Re: [PATCH 06/16] target/arm: Add sve infrastructure for page lookup

[Richard Henderson]

On 4/16/20 6:03 AM, Peter Maydell wrote:
>> +#ifdef CONFIG_USER_ONLY
>> +    memset(&info->attrs, 0, sizeof(info->attrs));
> 
> Could just write "info->attrs = {};" ?

Not quite.  Correct syntax would be attrs = (MemTxAttrs){ }.
I don't see that as an improvement over memset though.

>> +    int16_t mem_off_first[2];
>> +    int16_t reg_off_first[2];
>> +    int16_t reg_off_last[2];
> 
> It would be helpful to document what these actually are,
> and in particular what the behaviour is if the whole thing
> fits in a single page. (Judging by the code, the elements
> at index 1 for the 2nd page are set to -1 ?)

Yes, that's right.  I've added some more detail here.

>> +    intptr_t reg_off_first = -1, reg_off_last = -1, reg_off_split;
>> +    intptr_t mem_off_last, mem_off_split;
>> +    intptr_t page_split, elt_split;
>> +    intptr_t i;
> 
> intptr_t seems like a funny type to be using here, since these
> aren't actually related to pointers as far as I can tell.

They're used as array indexes.  If we use "int", the compiler keeps
re-extending the value.

>> +    memset(info, -1, offsetof(SVEContLdSt, page));
> 
> I guess this isn't conceptually much different from zeroing
> out integer struct fields, but it feels a bit less safe somehow.

It seems easier than setting 9 fields separately...

>> +    page_split = -(addr | TARGET_PAGE_MASK);
> 
> What is the negation for ?

Computation of remaining bytes in the page:

    -(x | TARGET_PAGE_MASK)
  = -(x | -TARGET_PAGE_SIZE)
  = TARGET_PAGE_SIZE - (x & -TARGET_PAGE_SIZE)

We use this all over qemu.


r~