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

[Lan Tianyu <tianyu.lan@xxxxxxxxx>]

Hi Julien:
        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.


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

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

For arm, the base address maybe passed by device tree?

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


-- 
Best regards
Tianyu Lan

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