Re: [PATCH v3 7/9] xen/arm: vgic: Optimize the way to store the target vCPU in the rank

[Julien Grall <julien.grall@citrix.com>]

On 08/10/15 10:39, Ian Campbell wrote:
> On Wed, 2015-10-07 at 20:13 +0100, Julien Grall wrote:
>> On 07/10/15 17:29, Julien Grall wrote:
>>> TBH, I don't see how I can justify that what we are doing is fine except
>>> by saying "for simplicity".
> 
> I think it is also fine to say that we currently expect that in reality
> OSes won't rely on being able to read-back from this register.
> 
>> I though a bit more about it. We are both interpreting the spec
>> differently and I think both are valid. So hardware people and OS
>> developer may also lean toward one of them.
> 
> TBH I don't agree, your definition essentially turns any RW register which
> does not specify precisely otherwise into a WO register.
> 
>> Mine is more restrictive than yours, as you said on v1, it would hit
>> picky OS that follow your interpretation and read back the register to
>> check either the value is exactly the one written and/or the previous
>> value. Although it make the implementation simpler and save memory.
>>
>> How about:
>>
>> "Note that with these changes, any read to those registers will list
>> only the target vCPU used by Xen. As the spec is not clear whether this
>> is a valid choice or not, picky OSes (i.e which read back register to
>> check the value written) which have a different interpretation of the
>> spec may not boot anymore on Xen. Although, I think this is a fair trade
>> between memory usage in Xen (1KB on a domain using 4 vCPUs with no SPIs)
>> and a strict interpretation of the spec (though all the cases are not
>> clearly defined)".
> 
> That's OK. I'd prefer to drop the "picky" and to make the "(i.e. ....)" in
> the middle to say something like "(i.e. OSes which perform read-modify
> -write operations on these registers"), which is not a picky thing to want
> to do even if we don't expect any OS to actually to it.

I will update the commit message.

> BTW, IHI 0069A says 8.9.17:
> 
>     When affinity routing is not enabled, holds the list of target PEs for
>     the interrupt. That is, it holds the list of CPU interfaces to which the
>     Distributor forwards the interrupt if it is asserted and has sufficient
>     priority.

I read multiple time the ITARGETSR section and somehow miss this paragraph.

> 
> Which isn't actually inconsistent with the register reading back the set of
> CPUs to which the interrupt is actually going to end up being routed (i.e.
> the behaviour of this patch).
> 
> However I think this has gone on long enough and some text which says it
> isn't clear is still a reasonable thing to write.

I will resend a new version after I got an answer from Stefano on [1].

Regards,

[1] http://lists.xen.org/archives/html/xen-devel/2015-10/msg00893.html

-- 
Julien Grall