Re: [Xen-devel] [Draft C] Xen on ARM vITS Handling

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Ian,

On 01/06/15 14:36, Ian Campbell wrote:
> On Fri, 2015-05-29 at 15:06 +0100, Julien Grall wrote:
> > Hi Vijay,
> >
> > On 27/05/15 17:44, Vijay Kilari wrote:
> > > > ## Command Translation
> > > >
> > > > Of the existing GICv3 ITS commands, `MAPC`, `MAPD`, `MAPVI`/`MAPI` are
> > > > potentially time consuming commands as these commands creates entry in
> > > > the Xen ITS structures, which are used to validate other ITS commands.
> > > >
> > > > `INVALL` and `SYNC` are global and potentially disruptive to other
> > > > guests and so need consideration.
> > > >
> > > > All other ITS command like `MOVI`, `DISCARD`, `INV`, `INT`, `CLEAR`
> > > > just validate and generate physical command.
> > > >
> > > > ### `MAPC` command translation
> > > >
> > > > Format: `MAPC vCID, vTA`
> > >    -  The GITS_TYPER.PAtype is emulated as 0. Hence vTA is always represents
> > >       vcpu number. Hence vTA is validated against physical Collection
> > > IDs by querying
> > >       ITS driver and corresponding Physical Collection ID is retrieved.
> > >    -  Each vITS will have cid_map (struct cid_mapping) which holds mapping of
> > Why do you speak about each vITS? The emulation is only related to one
> > vITS and not shared...
> And each vITS will have a cid_map, which is used. This seems like a
> reasonable way to express this concept in the context.
This is rather strange when everything in the command emulation is per-vits.

> Perhaps there is a need to include discussion of some of the secondary
> data structures alongside the defintion `cits_cq`. In which case we
> could talk about "its associated `cid_map`" and things.
>
> > >       Virtual Collection ID(vCID), Virtual Target address(vTA) and
> > >       Physical Collection ID (pCID).
> > >       If vCID entry already exists in cid_map, then that particular
> > > mapping is updated with
> > >       the new pCID and vTA else new entry is made in cid_map
> > When you move a collection, you also have to make sure that all the
> > interrupts associated to it will be delivered to the new target.
> >
> > I'm not sure what you are suggesting for that...
> This is going to be rather painful I fear.
>
> > >    -  MAPC pCID, pTA physical ITS command is generated
> > We should not send any MAPC command to the physical ITS. The collection
> > is already mapped during Xen boot and the guest should not be able to
> > move the physical collection (they are shared between all the guests and
> > Xen).
> This needs discussion in the background section, to describe the
> physical setup which the virtual stuff can make assumption of.
I don't think this is a background section. The physical number of
collection is limited (the mandatory number of collections is nr_cpus +
1). Those collection will likely be shared between Xen and the different
guests.

If we let the guest moving the physical collection we will also move all
the interrupts which is wrong.

> > >    Here there is no overhead, the cid_map entries are preallocated
> > > with size of nr_cpus
> > >    in the platform.
> > As said the number of collection should be at least nr_cpus + 1.
> FWIW I read this as "with size appropriate for nr_cpus", which leaves
> the +1 as implicit. I added the +1 nevertheless.
I wanted to make clear. His implementation was only considering nr_cpus
collections.

> > > > - `MAPC pCID, pTA` physical ITS command is generated
> > > >
> > > > ### `MAPD` Command translation
> > > >
> > > > Format: `MAPD device, Valid, ITT IPA, ITT Size`
> > > >
> > > > `MAPD` is sent with `Valid` bit set if device needs to be added and reset
> > > > when device is removed.
> > > >
> > > > If `Valid` bit is set:
> > > >
> > > > - Allocate memory for `its_device` struct
> > > > - Validate ITT IPA & ITT size and update its_device struct
> > > > - Find number of vectors(nrvecs) for this device by querying PCI
> > > >   helper function
> > > > - Allocate nrvecs number of LPI XXX nrvecs is a function of `ITT Size`?
> > > > - Allocate memory for `struct vlpi_map` for this device. This
> > > >   `vlpi_map` holds mapping of Virtual LPI to Physical LPI and ID.
> > > > - Find physical ITS node with which this device is associated
> > > > - Call `p2m_lookup` on ITT IPA addr and get physical ITT address
> > > > - Validate ITT Size
> > > > - Generate/format physical ITS command: `MAPD, ITT PA, ITT Size`
> > > >
> > > > Here the overhead is with memory allocation for `its_device` and `vlpi_map`
> > > >
> > > > XXX Suggestion was to preallocate some of those at device passthrough
> > > > setup time?
> > > If Validation bit is set:
> > >    - Query its_device tree and get its_device structure for this device.
> > >    - (XXX: If pci device is hidden from dom0, does this device is added
> > >        with PHYSDEVOP_pci_device_add hypercall?)
> > >    - If device does not exists return
> > >    - If device exists in RB-tree then
> > >           - Validate ITT IPA & ITT size and update its_device struct
> > To validate the ITT size you need to know the number of interrupt ID.
> Please could you get into the habit of making concrete suggestions for
> changes to the text. I've no idea what change I should make based on
> this observation. If not concrete suggestions please try and make the
> implications of what you are saying clear.
The size of the ITT is based on the number of Interrupt supported by the
device.

The only way to validate the size getting the number of Interrupt
before. i.e

        - Find the number of MSI for this device
        - Compute the theorical size of the ITT
        - Validate the ITT IPA and the ITT size
        - Update the its_device struct

Note: this is based on this conversation and doesn't take into account
the other threads.

> > >           - Check if device is already assigned to the domain,
> > >             if not then
> > >                - Find number of vectors(nrvecs) for this device.
> > >                - Allocate nrvecs number of LPI
> > >                - Fetch vlpi_map for this device (preallocated at the
> > > time of adding
> > >                  this device to Xen). This vlpi_map holds mapping of
> > > Virtual LPI to
> > >                  Physical LPI and ID.
> > >                - Call p2m_lookup on ITT IPA addr and get physical ITT address
> > >                - Assign this device to this domain and mark as enabled
> > >           - If this device already exists with the domain (Domain is
> > > remapping the device)
> > >                - Validate ITT IPA & ITT size and update its_device struct
> > >                - Call p2m_lookup on ITT IPA addr and get physical ITT address
> > >                - Disable all the LPIs of this device by searching
> > > through vlpi_map and LPI
> > >                  configuration table
> > Disabling all the LPIs associated to a device can be time consuming
> > because you have to unroute them and make sure that the physical ITS
> > effectively disabled it before sending the MAPD command.
> >
> > Given that the software would be buggy if it send a MAPD command without
> > releasing all the associated interrupt we could ignore the command if
> > any interrupt is still enabled.
> Releasing how? Did you mean disable?
I think disable is enough (see 4.9.18).

> > >     - Clear all vlpis mapping for this device
> > >     - Remove this device from the domain
> > >
> > > > ### `MAPVI`/`MAPI` Command translation
> > > >
> > > > Format: `MAPVI device, ID, vID, vCID`
> > > >
> > > > - Validate if the device exits by checking vITS device list
> > > > - Validate vCID and get pCID by searching cid_map
> > > >
> > > > - if vID does not have entry in `vlpi_entries` of this device allocate
> > > >   a new pID from `vlpi_map` of this device and update `vlpi_entries`
> > > >   with new pID
> > > > - Allocate irq descriptor and add to RB tree
> > > > - call `route_irq_to_guest()` for this pID
> > > > - Generate/format physical ITS command: `MAPVI device ID, pID, pCID`
> > > - Validate if the device exists by checking vITS device RB-tree.
> > > - Validate vCID and get pCID by searching cid_map
> > > - if vID does not have entry in vlpi_entries of this device
> > >       -  Allot pID from vlpi_map of this device and update
> > > vlpi_entries with new pID.
> > >       - Allocate irq descriptor and add to RB tree
> > >       - call route_irq_to_guest() for this pID
> > >   If exists,
> > >      - If vCID is different ( remapping interrupts to differnt collection ),
> > >             - Disable LPI
> > You have to ensure the the LPI is disabled with an INV/SYNC.
> Can we not rely on the subsequent INV/SYNC from the guest relating to
> the MAPI here?
I think you meant MAPVI here.

It's not mandatory that MAPVI is followed by an INV. INV is only used 
when the software modified the LPI configuration table.
Furthermore, it's not clear what would be the behavior of the ITS if the 
interrupt is not correctly disabled (i.e not having an INV/SYNC before 
MAPI).
Although, I think more about it and Xen should not disable/enable any 
IRQ without the explicit request of the guest. Based on 4.9.17, I think 
we can ignore the command if the IRQ has not been disabled (i.e LPI 
disabled in the LPI conf table + INV/INVALL sent).
> > Although as suggested on the previous commands, we may want to deny the
> > command if the interrupt is not disabled.
> >
> > >             - Update the vlpi_map
> > >              (XXX: Enable LPI on guest request?)
> > Read the spec...
> If you know the answer then please share it rather than making people
> guess what you are talking about.
The Spec is pretty clear on it (see 4.9.17). The guest is managing the
interrupt itself via the LPI configuration table, we should not try to
do more than the guest ask.

The question to enable/disable the IRQ will depend on the virtual LPI
configuration table and if we already replicated the value to the
physical one.

Although as we don't have any mapping vLPI -> pLPI at this time, it 
likely means that we have to check the virtual LPI configuration table, 
replicate the value in the physical table and then send an INV/SYNC (if 
the value has been toggled) to ensure the completion.
This is because it would be valid, even if it's pointless, to enable an 
LPI before mapping a (DevID, ID).
Regards,

--
Julien Grall

--
Julien Grall

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