Re: [Xen-devel] [PATCH v2 08/15] xen/arm: vgic-v3: Emulate correctly the re-distributor

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Ian,

On 02/02/15 17:38, Ian Campbell wrote:
> On Mon, 2015-02-02 at 17:05 +0000, Julien Grall wrote:
>> On 02/02/15 16:47, Ian Campbell wrote:
>>> On Mon, 2015-02-02 at 16:33 +0000, Julien Grall wrote:
>>>> Hi Ian,
>>>>
>>>> On 02/02/15 15:59, Ian Campbell wrote:
>>>>> On Thu, 2015-01-29 at 18:25 +0000, Julien Grall wrote:
>>>>>> There is a one-to-one mapping between each re-distributors and 
>>>>>> processors.
>>>>>> Each re-distributors can be accessed by any processor at any time. For
>>>>>> instance during the initialization of the GIC, the drivers will browse
>>>>>> the re-distributor to find the one associated to the current processor
>>>>>> (via GICR_TYPER). So each re-distributor has its own MMIO region.
>>>>>>
>>>>>> The current implementation of the vGICv3 emulation assumes that the
>>>>>> re-distributor region is banked. Therefore, the processor won't be able
>>>>>> to access other re-distributor. While this is working fine for Linux, a
>>>>>> processor will only access GICR_TYPER to find the associated 
>>>>>> re-distributor,
>>>>>> we should have a correct implementation for the other operating system.
>>>>>
>>>>> You could state something stronger here, which is that it is wrong and
>>>>> should be fixed as a matter of principal. The fact that we happen to get
>>>>> away with it for some versions of Linux is an aside (although worth
>>>>> mentioning)
>>>>
>>>> I will rework the paragraph.
>>>>
>>>>>>
>>>>>> All emulated registers of the re-distributors take a vCPU in parameter
>>>>>> and necessary lock. Therefore concurrent access is already properly 
>>>>>> handled.
>>>>>>
>>>>>> The missing bit is retrieving the right vCPU following the region 
>>>>>> accessed.
>>>>>> Retrieving the right vCPU could be slow, so it has been divided in 2 
>>>>>> paths:
>>>>>>     - fast path: The current vCPU is accessing its own re-distributor
>>>>>>     - slow path: The current vCPU is accessing an other re-distributor
>>>>>
>>>>> "another".
>>>>>
>>>>>>
>>>>>> As the processor needs to initialize itself, the former case is very
>>>>>> common. To handle the access quickly, the base address of the
>>>>>> re-distributor is computed and stored per-vCPU during the vCPU 
>>>>>> initialization.
>>>>>>
>>>>>> The latter is less common and more complicate to handle. The 
>>>>>> re-distributors
>>>>>> can be spread accross multiple regions in the memory.
>>>>>
>>>>> "across"
>>>>>
>>>>>> +    /*
>>>>>> +     * Find the region where the re-distributor lives. For this purpose,
>>>>>> +     * we look one region ahead as only MMIO range for redistributors
>>>>>> +     * traps here.
>>>>>> +     * Note: We assume that the region are ordered.
>>>>>
>>>>> You could also check base+size vs gpa to avoid this limitation.
>>>>
>>>> IHMO, this limitation is harmless. This would happen for DOM0 if the
>>>> device tree doesn't sort the region.
>>>
>>> If it can happen then it isn't harmless, and it's easy to avoid so why
>>> not do so.
>>
>> The code is looking one region a-head to avoid check in the case there
>> is only one big re-distributors region.
>>
>>>> AFAICT, we have a similar limitation for the memory region. Although I
>>>> could sort them when we build DOM0.
>>>
>>> I'm not sure we do these days, but in any case two wrongs don't make a
>>> right.
>>
>> See consider_modules. We implicitly assume memory ordering.
>>
>> Anyway, if we do that, we should also check the overlapping between
>> regions, the size of the region is valid (i.e aligned to 64K and
>> correctly sized...),...
> 
> I don't see how all that follows, certainly not at run time. Those are
> the sorts of things which should be checked at initialisation time and
> cause us to complain loudly.

My point was, we have to trust a bit the device tree given by the
platform. If the device tree is buggy, then bare-metal linux will
unlikely boot.

But I agree, that we have to sort the region of re-distributors at init.

>> The main drawbacks here would be DOM0 access the wrong vCPU or receive a
>> data abort. It's not too bad compare to "slowing down" the lookup.
> 
>> If you really want to support non-ordered access, I could do at Domain
>> initialization.
> 
> If we are at liberty to sort the list at init time then that would be
> fine.

I don't think it's matter if we don't expose exactly the same layout for
the re-distributors as the physical hardware.

So, we can do whatever we want :).

> We probably ought to do the same with the memory regions
> 
>>> I'd not be all that surprised to see systems with more rdist region
>>> space than they have physical processors e.g. sku's with different
>>> numbers of cores, but otherwise the same platform.
>>
>> So the current check would be enough? Even though, having more vCPUs
>> than physical CPUs for DOM0 sounds silly.
>>
> 
> I expect so, yes.

Great, thanks. I will update the comment in the next version.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel