Re: Xen on ARM vITS Handling Draft B (Was Re: Xen/arm: Virtual ITS command queue handling)

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

Hi,

On 25/05/2015 12:40, Vijay Kilari wrote:
> On Mon, May 25, 2015 at 3:02 PM, Julien Grall
> <julien.grall.oss@gmail.com> wrote:
>>
>>
>> On 25/05/2015 11:06, Vijay Kilari wrote:
>>>
>>> On Sun, May 24, 2015 at 4:05 PM, Julien Grall <julien.grall@citrix.com>
>>> wrote:
>>>>>
>>>>> 1) Command translation:
>>>>> -----------------------------------
>>>>>
>>>>>     - ITS commands contains device ID, Event ID (vID), Collection ID
>>>>> (vCID), Target Address (vTA)
>>>>>        parameters
>>>>>     - All these parameters should be validated
>>>>>     - These parameters should be translated from Virtual to Physical
>>>>>
>>>>> Of the existing GICv3 ITS commands, MAPC, MAPD, MAPVI/MAPI are the time
>>>>> consuming commands as these commands creates entry in the Xen ITS
>>>>> structures,
>>>>> which are used to validate other ITS commands.
>>>>>
>>>>> 1.1 MAPC command translation
>>>>> -----------------------------------------------
>>>>>       Format: MAPC vCID, vTA
>>>>>
>>>>>       -  vTA is validated against Re-distributor address by searching
>>>>> Redistributor region /
>>>>>           CPU number based on GITS_TYPER.PAtype and Physical Collection
>>>>> ID & Physical
>>>>>           Target address are retrieved
>>>>>       -  Each vITS will have cid_map (struct cid_mapping) which holds
>>>>> mapping of
>>>>>          Virtual Collection ID, Virtual Targets address and Physical
>>>>> Collection ID.
>>>>>       -  MAPC pCID, pTA physical ITS command is generated
>>>>>
>>>>>       Here there is no overhead, the cid_map entries (approx 32 entries)
>>>>> are preallocated when
>>>>>       vITS is created.
>>>>
>>>>
>>>>
>>>> How did you decide the 32 entries? The ITS must at least provide N + 1
>>>> collection when N is the number of processors.
>>>
>>>
>>> It should be MAX_VIRT_VCPUS.
>>
>>
>> Why not allocating dynamically rather than wasting memory?
>>
>>>>
>>>> Also, how do you handle collection re-mapping?
>>>
>>>
>>> There is one collection per cpu. The vTA of MAPC should fall within
>>> vcpus range (GITS_TYPE.PTAtype is 0).
>>
>>
>> It's not what I asked...
>>
>>> In case of remapping,  if the vCID does not exists in cid_map,
>>> then new entry is made (vCID, pCID, vTA)
>>>
>>> If vCID exists, the existing entry is updated with pCID, vTA
>>>
>>> However this cid_map should be used to inject to right pCPU where
>>> vCPU is running.
>>
>>
>> What do you mean by injecting? The MAPC should never be injected to the
>> physical CPU. As I said earlier, the collection is shared with all the vCPU
>> and Xen.
>>
>
> It does not mean MAPC is sent to physical CPU,
>
> All interrupts mapped to collection are taken on cpus 0 to nr_vcpus.
> when vCID is mapped to pCID, all pCID fall in the range of 0 to nr_vcpus

vCID can be higher than the number of VCPUs (the vITS has to support 
nr_vcpus + 1 collection).

Also, the number of physical collection may be lower than the virtual 
collection because the user created a guest with num vCPUs > num pCPU.

> So, irrespective of vcpus running on physical cpus all interrupts are routed
> to pCPU 0 to nr_vcpus
>
> Similar to below patch done for SPIs. LPIs should also be injected.

I know that LPIs should be injected...

>
> http://lists.xen.org/archives/html/xen-devel/2014-09/msg04176.html
>
> Correct me if I have not understood your question correctly.

AFAIU your proposal, the function mapping(vCID) will always return the 
same pCID, right?

[..]

>>>> What about device remapping?
>>>
>>>
>>> IMO, device cannot be remapped. It has to removed (MAPD with valid bit 0)
>>> so that ITS HW can remove the entries and added with new MAPD command.
>>
>>
>> Your opinion is not the spec...
>>
>> Device remapping is allowed by the spec (see 4.9.18 "Re-mapping and
>> Un-mapping devices in PRD03-GENC-010745 24.0). So even it's not possible
>> (with a spec ref in proof), you have to protect it...
>
> I am no saying that is my opinion, I mean the same as told in 4.9.18,

IMO === In My Opinion... I can't guess that you were talking about 4.9.18.

> To unmap the device, the MAPD should be sent with valid bit 0, which will

s/unmap/re-map/ ?

> remove the device from the list and added again on MAPD with valid bit 1

I can't see where the spec says that 2 MAPD (one with V=1 and the other 
with V=0) is required. The section 4.9.18 contains an 'or':

"Issue a mapping command (MAPD; see section 5.13.11) or an un-mapping 
command"

This is related to "Interrupts can be re-mapped or un-mapped".

4.9.18 and 5.13.11 (PRD03-GENC-010745 24.0) are only speaking about a 
single MAPD:

"Note: software might issue a MAPD command to re-map an already mapped 
device and the ITS must invalidate all cached data for that device."

>>
>> new pID may not be re-generated but there is some care to take when an vID
>> is remapped. (see 4.9.17 "Re-mapping and Un-mapping Interrupts" in
>> PRD03-GENC-010745 24.0).
>>
>>> If vCID is changed, a new pCID is generated based on MAPC command
>>
>>
>> Which is wrong...
>
> When you say vID is remapped, then vCID should be different right?

Yes. I was confuse by "MAPC command" at the end.

Regards,

-- 
Julien Grall