[Skiboot] [PATCH 05/15] libc/string: Add memcpy_ci

[Oliver O'Halloran]

On Fri, Sep 2, 2016 at 2:59 PM, Claudio Carvalho
<cclaudio at linux.vnet.ibm.com> wrote:
>
>
> On 09/01/2016 06:34 AM, Stewart Smith wrote:
>> Claudio Carvalho <cclaudio at linux.vnet.ibm.com> writes:
>>> --- /dev/null
>>> +++ b/libc/string/memcpy_ci.c
>>> @@ -0,0 +1,50 @@
>>> +/* Copyright 2013-2016 IBM Corp
>>> + *
>>> + * Licensed under the Apache License, Version 2.0 (the "License");
>>> + * you may not use this file except in compliance with the License.
>>> + * You may obtain a copy of the License at
>>> + *
>>> + *      http://www.apache.org/licenses/LICENSE-2.0
>>> + *
>>> + * Unless required by applicable law or agreed to in writing, software
>>> + * distributed under the License is distributed on an "AS IS" BASIS,
>>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
>>> + * implied.
>>> + * See the License for the specific language governing permissions and
>>> + * limitations under the License.
>>> + */
>>> +
>>> +#include <string.h>
>>> +#include <io.h>
>>> +
>>> +const void* memcpy_ci(const void *destpp, const void *srcpp, size_t len)

I really don't like this name.  If I encoutered a function called
memcpy_ci() I would assume it performs the copy with CI loads *and*
stores. This function copies from CI memory to cachable memory so it
should be called memcpy_from_ci() or similar. This might seem a little
nitpicky, but mixing cached and CI accesses to the same memory can
cause a checkstop (even in real mode) so it's important to be clear
about how memory is being accessed.

>>> +{
>>> +    size_t i;
>>> +    const size_t block = sizeof(uint32_t);
>>> +
>>> +    unsigned long int destp = (long int) destpp;
>>> +    unsigned long int srcp = (long int) srcpp;
>>> +
>>> +    /* copy just a few bytes to make dst aligned */
>>> +    i = len % block;
>>> +    while (i > 0)
>>> +    {
>>> +            *((uint8_t*) destp) = in_8((uint8_t*)srcp);
>>> +            srcp += 1;
>>> +            destp += 1;
>>> +            i--;
>>> +    }
>>> +
>>> +    len -= len % block;
>>> +
>>> +    /* copy the aligned bytes */
>>> +    while(len > 0)
>>> +    {

Nitpick: In skiboot we use the Kernel's brace style. Functions should
have their brace on a new line, but control flow statements should
have the brace on the same line.

>>> +            *((uint32_t*) destp) = in_be32((uint32_t*)srcp);
>>> +            srcp += block;
>>> +            destp += block;
>>> +            len -= block;
>>> +    }
>>> +
>>> +    return destpp;
>>> +}
>>
>> I think it would be good to add a unit test for this function.
>>
>
> Makes sense. I will figure out how to add a unit test in skiboot for
> this function.
>
>
>> It also (currently) assumes that src is a multiple of uint32_t, although
>> nothing checks that and so would silently fail if called for, say 33 bytes.
>>
>
> Not sure if I understand your suggestion here.
>
> This function doesn't check if srcpp and dstpp are multiple of uint32_t,
> but it assumes that the processor is able to read a char and a uint32_t
> from src. Thus, the first loop copies at most len % sizeof(uint32_t) to
> optimize the number of reads

The length checking is fine, the bigger issue here is that the first
loop will leave srcp unaligned.  If we wanted to copy 7 bytes starting
from 0x0 the first loop would move srcp to 0x3 before attempting to
finish the copy with a single 32 bit load. On P8 unaligned CI loads
will cause an alignment interrupt so the assumption that the processor
can read a uint32_t from srcp is faulty. If you want to keep this
optimisation the first loop needs to align srcp so that (srcp % block)
== 0. The bulk of the copy can then be done with with 32 (or 64bit)
loads and finished with a byte-by-byte copy. You could move the byte
copies into a helper function to eliminate all the int <-> pointer
casting too.

>
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot