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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Tue, 2015-06-02 at 11:46 +0100, Julien Grall wrote:
> 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.

I'm afraid you are going to have to say more explicitly what you find
strange here.

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

Right, and this needs to be explained in the document as an assumption
upon which other things can draw, so that the document is (so far as
possible) a coherent whole...

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

... and therefore things like this would become apparent.

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

This is the only thing which I think is currently problematic (the rest
we know).

AIUI the intention for PCI devices was that we would learn this around
the time dom0 makes the PHYSDEVOP_pci_device_add call (or perhaps
XEN_DOMCTL_assign_device or some other function for non-PCI devices).

The doc at the moment says:

- Find number of vectors (nrvecs) for this device by querying PCI
  helper function

I don't recall if this was in draftC or is based on feedback since then.
In any case I've changed it to say

- Find number of vectors from `its_device` struct.

and added a note:

XXX Number of vectors contains in `struct its_device` needs to have
been communicated from dom0 at some point, e..g by
PHYSDEVOP_pci_device_add call or XEN_DOMCTL_assign_device or some
other scheme.

>       - Compute the theorical size of the ITT
>       - Validate the ITT IPA and the ITT size

Do we need to validate against the theoretical size, or just again the
amount of memory the guest has or?

If we don't actually store anything in the ITT then we may be able just
not care about the size.



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