Re: [Qemu-arm] [PATCH v2] arm: implement cache/shareability attribute bits for PAR registers

[Peter Maydell]

On 31 October 2017 at 01:23, Andrew Baumann
<address@hidden> wrote:
>> From: Peter Maydell [mailto:address@hidden
>> Hi. This is looking pretty good, but I have a few comments below,
>> and we're pretty much at the softfreeze date (KVM Forum last week
>> meant I didn't get much code review done, unfortunately). Would
>> you be too sad if this missed 2.11 ?
>
> No worries. It would be nice to have a stable release that we can tell people 
> supports arm64 Windows, but I recognise that this is a non-trivial change, so 
> if we have to wait for 2.12 to get that, then fair enough.

Is this the only missing piece, then? If we squint a bit we can
call it a bug fix as long as we can get it in early in the softfreeze
rather than later...


>> The v8A ARM ARM pseudocode CombineS1S2AttrHints() says that the hint
>> bits always come from s1 regardless of whose attrs won.
>
> Aha, I was wondering about this. Thanks for the pointer to the pseudocode... 
> it isn't referenced anywhere in the relevant section! It's reassuring to see 
> that, aside from the hints (where the English was ambiguous IIRC), I seem to 
> have got the rest of the translation correct.

I didn't find anything in the English text about hints at all.
(I'll have to go and have another look.)

>> You can play bit games with the format here, because
>> what we're effectively implementing is "whichever is last in
>> the order '0, 3, 2' wins", which is
>>    ret.shareability = MIN(s1.shareability ^ 1, s2.shareability ^ 1) ^ 1;
>> (since the xor with 1 transforms (0,3,2) to (1,2,3) and is self-inverse).
>> Is that better than the if ladder above? Not entirely sure :-)
>
> I've no confidence in my ability to get that bit-logic right, or
> even to check that you did. I'd rather leave it to the compiler to
> figure out how to play those optimisation games :)

Mmm. This isn't on a fast path I think so we may as well write
it clearly.

>> > +    /* Combine memory type and cacheability attributes */
>> > +    if (s1hi == 0 || s2hi == 0) {
>> > +        /* Device has precedence over normal */
>> > +        if (s1lo == 0 || s2lo == 0) {
>> > +            /* nGnRnE has precedence over anything */
>> > +            ret.attrs = 0;
>> > +        } else if (s1lo == 4 || s2lo == 4) {
>> > +            /* non-Reordering has precedence over Reordering */
>> > +            ret.attrs = 4;  /* nGnRE */
>> > +        } else if (s1lo == 8 || s2lo == 8) {
>> > +            /* non-Gathering has precedence over Gathering */
>> > +            ret.attrs = 8;  /* nGRE */
>> > +        } else {
>> > +            ret.attrs = 0xc; /* GRE */
>> > +        }
>>
>> Isn't this if-ladder equivalent to just "ret.attrs = MIN(s1lo, s2lo);" ?
>
> Assuming both s1lo and s2lo avoid undefined encodings, yes it is. I tend to 
> prefer the if-ladder just because it's a more direct translation of the spec 
> (e.g. CombineS1S2Device() pseudocode). But in this case if you prefer I could 
> change it to the MIN version for brevity, since it's fairly straightforward.

Your choice either way, then.

thanks
-- PMM