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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Aug 24, 2017 at 03:03:49PM +0100, Julien Grall wrote:
> Hi,
> 
> On 23/08/17 15:05, Roger Pau Monné wrote:
> > On Wed, Aug 23, 2017 at 11:19:01AM +0100, Julien Grall wrote:
> > > Hi Roger,
> > > 
> > > On 23/08/17 08:22, Roger Pau Monné wrote:
> > > > 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.
> > > 
> > > To me, it makes more sense to care both the base and the size rather than
> > > only the former.
> > > 
> > > The toolstack is in charge of the address space and should be aware of the
> > > size of everything. This address space may not be static and it makes 
> > > sense
> > > to give this information to Xen and verify we had the same assumption.
> > 
> > Does this imply that we will have variable size vIOMMU MMIO regions?
> 
> There are existing IOMMU with multiple MMIO regions. This is the case of the
> Nvidia SMMU. Whether we will emulate then is another question. But for
> completeness, I would use address/size.

The proposed implementation does not support multiple MMIO regions
anyway. I'm not going to oppose to this anymore, but I don't see much
value on implementing things just for completeness without having a
real use case, specially when this is a domctl interface that is not
stable, ie: we can always modify it later on without any issues.

Roger.

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