Re: [Xen-devel] [PATCH 5/25] Xen/doc: Add Xen virtual IOMMU doc

[Julien Grall <julien.grall@xxxxxxx>]

On 05/07/17 04:15, Lan Tianyu wrote:
> Hi Julien:
Hi Tianyu Lan,

>         Thanks for your review.
>
> On 2017年07月04日 18:39, Julien Grall wrote:
> > > +vIOMMU hypercall
> > > +================
> > > +Introduce new domctl hypercall "xen_domctl_viommu_op" to create/destroy
> > > +vIOMMU and query vIOMMU capabilities that device model can support.
> > > +
> > > +* vIOMMU hypercall parameter structure
> > > +struct xen_domctl_viommu_op {
> > > +    uint32_t cmd;
> > > +#define XEN_DOMCTL_create_viommu          0
> > > +#define XEN_DOMCTL_destroy_viommu         1
> > > +#define XEN_DOMCTL_query_viommu_caps      2
> > I am a bit confused. This is only creating the vIOMMU. However, there
> > might be multiple host IOMMUs, how do you link them together?
> >
> > > +    union {
> > > +        struct {
> > > +            /* IN - vIOMMU type */
> > > +            uint64_t viommu_type;
> > This is a bit confusing, you don't define what should be the value of
> > viommu_type, ...
> >
> > > +            /* IN - MMIO base address of vIOMMU. */
> > > +            uint64_t base_address;
> > > +            /* IN - Length of MMIO region */
> > > +            uint64_t length; > +            /* IN - Capabilities with
> > > which we want to create */
> > > +            uint64_t capabilities;
> > ... capabilities ...
> Sorry. miss the type and capability definition here.
>
> /* VIOMMU type */
> #define VIOMMU_TYPE_INTEL_VTD     (1u << 0)
>
> /* VIOMMU capabilities*/
> #define VIOMMU_CAP_IRQ_REMAPPING  (1u << 0)
>
> "viommu_type" means vendor vIOMMU device model. So far, we just support
> virtual Intel VTD.
>
> "capabilities" means the feature that vIOMMU supports. We just add
> interrupt remapping for virtual VTD.
>
>
> > > +            /* OUT - vIOMMU identity */
> > > +            uint32_t viommu_id;
> > > +        } create_viommu; > +
> > > +        struct {
> > > +            /* IN - vIOMMU identity */
> > > +            uint32_t viommu_id;
> > > +        } destroy_viommu;
> > > +
> > > +        struct {
> > > +            /* IN - vIOMMU type */
> > > +            uint64_t viommu_type; > +            /* OUT - vIOMMU
> > > Capabilities */
> > > +            uint64_t caps;
> > ... and caps. I see you have defined them in a separate header
> > (viommu.h). But there are no way for the developer to know that they
> > should be used.
> Macros of "Capabilities" and "type" are defined under public directory
> in order to tool stack also can use them to pass vIOMMU type and
> capabilities.
My point was that if a developer read domctl.h first, he cannot guess 
that the value to be used in "capabilities" and "type" are defined in a 
separate header (viommu.h). You should at least write down a comment in 
the code explaining that.
> > > +        } query_caps;
> > > +    } u;
> > > +};
> > > +
> > > +- XEN_DOMCTL_query_viommu_caps
> > > +    Query capabilities of vIOMMU device model. vIOMMU_type specifies
> > > +which vendor vIOMMU device model(E,G Intel VTD) is targeted and
> > > hypervisor
> > "E,G" did you mean "e.g"?
> Yes. Will update.
>
> > > +returns capability bits(E,G interrupt remapping bit).
> > Ditto.
> >
> > A given platform may have multiple IOMMUs with different features. Are
> > we expecting
> So far, our patchset just supports VM with one vIOMMU as starter.
>
> Do you mean emulation of some vIOMMU capabilities rely on physical IOMMU
> and there are multiple IOMMUs with different feature?
>
> If yes, we need to emulate mult-vIOMMU for different assigned devices
> under different pIOMMU. Vendor vIOMMU device model needs to check
> whether the assigned device and support given capabilities passed by
> tool stack.
Hmmm, I think I was a bit confused with the domctl. You are querying the 
vIOMMU capabilities and they may be different from the physical IOMMU right?
> > > +
> > > +- XEN_DOMCTL_create_viommu
> > > +    Create vIOMMU device with vIOMMU_type, capabilities, MMIO
> > > +base address and length. Hypervisor returns viommu_id. Capabilities
> > > should
> > > +be in range of value returned by query_viommu_caps hypercall.
> > Can you explain what mmio and length are here for? Do you expect to trap
> > and emulate the MMIO region in Xen?
> Yes, we need to emulate VTD MMIO register in the Xen hypervisor and this
> is agreement under design stage. The MMIO base address is passed to
> guest via ACPI table which is built by tool stack and so tool stack
> manages vIOMMU MMIO region. When create vIOMMU, base address and length
> needs to be passed.
I am not yet sure we want to emulate an IOMMU for ARM. They are a bit 
complex to emulate and we have multiple one (SMMUv2, SMMUv3, 
IPMMU-VMSA,...). So PV might be the solution here. Though, it is too 
early to decide.
If we wanted to use emulation, an IOMMU may have multiple MMIO ranges 
and multiple interrupts (either legacy or MSI). Here you are assuming 
only one MMIO and no interrupt. This new interface is a DOMCTL so it 
might be ok to extend it in the future?
Furthermore, on ARM we would be able to create the vIOMMU but it would 
be unusable. Indeed, IOMMU are only used to protect devices. But you 
don't see any way to say "This device is protected by the IOMMU". Did I 
miss anything?
> For arm, the base address maybe passed by device tree?
Either Device Tree or ACPI. I don't think it matters here.

> > From just looking at the document. I am struggling to understand how
> > this is going to be useful.
> >
> > > +
> > > +- XEN_DOMCTL_destroy_viommu
> > > +    Destroy vIOMMU in Xen hypervisor with viommu_id as parameters.
> > > +
> > > +xl vIOMMU configuration
> > > +=======================
> > > +viommu="type=vtd,intremap=1,x2apic=1"
> > > +
> > > +"type" - Specify vIOMMU device model type. Currently only supports
> > > Intel vtd
> > > +device model.
> > > +"intremap" - Enable vIOMMU interrupt remapping function.
> > > +"x2apic" - Support x2apic mode with interrupt remapping function.
Cheers,

--
Julien Grall

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