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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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.

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.

> >    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.

> >> - `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.


> 
> >           - 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?


> >     - 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?

> 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.

Ian.


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