On 31 October 2017 at 01:23, Andrew Baumann
<andrew.baum...@microsoft.com> wrote:
>> From: Peter Maydell [mailto:peter.mayd...@linaro.org]
>> 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
