Re: [PATCH V2 1/25] DOMCTL: Introduce new DOMCTL commands for vIOMMU support

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, Aug 23, 2017 at 02:06:17PM +0800, Lan Tianyu wrote:
> Hi Roger:
> 	Thanks for your review.
> 
> On 2017年08月22日 22:32, Roger Pau Monné wrote:
> > On Wed, Aug 09, 2017 at 04:34:02PM -0400, Lan Tianyu wrote:
> >> +
> >> +/* vIOMMU capabilities */
> >> +#define VIOMMU_CAP_IRQ_REMAPPING  (1u << 0)
> >> +
> >> +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
> >> +    union {
> >> +        struct {
> >> +            /* IN - vIOMMU type */
> >> +            uint64_t viommu_type;
> >> +            /* 
> >> +             * IN - MMIO base address of vIOMMU. vIOMMU device models
> >> +             * are in charge of to check base_address and length.
> >> +             */
> >> +            uint64_t base_address;
> >> +            /* IN - Length of MMIO region */
> >> +            uint64_t length;
> > 
> > It seems weird that you can specify the length, is that something
> > that a user would like to set? Isn't the length of the IOMMU MMIO
> > region fixed by the hardware spec?
> 
> Different vendor may have different IOMMU register region sizes. (e.g,
> VTD has one page size for register region). The length field is to make
> vIOMMU device model not to abuse address space. Some registers' offsets
> are reported by other register and these offsets are emulated by vIOMMU
> device model. If it's not necessary, we can remove it and add it when
> there is real such requirement.

So from my understanding the size of the IOMMU MMIO region is implicit
in the IOMMU type that the user chooses. I don't think this field is
needed.

> > 
> >> +            /* IN - Capabilities with which we want to create */
> >> +            uint64_t capabilities;
> >> +            /* 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 capabilities;
> >> +        } query_caps;
> > 
> > This also seems weird, shouldn't you query the capabilities of an
> > already created vIOMMU, rather than a vIOMMU type? Shouldn't the first
> > field be viommu_id?
> > 
> 
> Query interface here is to check what capabilities the vIOMMU device
> model specified by viommu_type can support before create vIOMMU (suppose
> user may select different capabilities). If capabilities returned by
> query interface doesn't meet user configuration, tool stack should
> return error. So it just accepts viommu_type.

I don't think that's needed, if the chosen capabilities are not
supported by the selected IOMMU type simply return error in
XEN_DOMCTL_create_viommu.

The capabilities of each IOMMU type should be listed in the man page,
and the user should select a supported set or else
XEN_DOMCTL_create_viommu will fail. Doing the checks both in the
toolstack and in XEN_DOMCTL_create_viommu seems pointless and prone to
errors.

Roger.

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