RE: [PATCH 2/5] iommu / domctl: introduce XEN_DOMCTL_iommu_ctl

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 16 October 2020 16:47
> To: Paul Durrant <paul@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Paul Durrant <pdurrant@xxxxxxxxxx>; Daniel De Graaf 
> <dgdegra@xxxxxxxxxxxxx>; Ian Jackson
> <iwj@xxxxxxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Andrew Cooper 
> <andrew.cooper3@xxxxxxxxxx>; George Dunlap
> <george.dunlap@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Stefano 
> Stabellini
> <sstabellini@xxxxxxxxxx>
> Subject: Re: [PATCH 2/5] iommu / domctl: introduce XEN_DOMCTL_iommu_ctl
> 
> Hi Paul,
> 
> On 05/10/2020 10:49, Paul Durrant wrote:
> > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> > index 791f0a2592..75e855625a 100644
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -1130,6 +1130,18 @@ struct xen_domctl_vuart_op {
> >                                    */
> >   };
> >
> > +/*
> > + * XEN_DOMCTL_iommu_ctl
> > + *
> > + * Control of VM IOMMU settings
> > + */
> > +
> > +#define XEN_DOMCTL_IOMMU_INVALID 0
> 
> I can't find any user of XEN_DOMCTL_IOMMU_INVALID. What's the purpose
> for it?
> 

It's just a placeholder. I think it's generally safer that a zero opcode value 
is invalid.

> [...]
> 
> > diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> > index b21c3783d3..1a96d3502c 100644
> > --- a/xen/include/xsm/xsm.h
> > +++ b/xen/include/xsm/xsm.h
> > @@ -106,17 +106,19 @@ struct xsm_operations {
> >       int (*irq_permission) (struct domain *d, int pirq, uint8_t allow);
> >       int (*iomem_permission) (struct domain *d, uint64_t s, uint64_t e, 
> > uint8_t allow);
> >       int (*iomem_mapping) (struct domain *d, uint64_t s, uint64_t e, 
> > uint8_t allow);
> > -    int (*pci_config_permission) (struct domain *d, uint32_t machine_bdf, 
> > uint16_t start, uint16_t
> end, uint8_t access);
> > +    int (*pci_config_permission) (struct domain *d, uint32_t machine_bdf, 
> > uint16_t start, uint16_t
> end, uint8_t access);
> 
> I can't figure out what changed here. Is it an intended change?
> 

No, I'll check and get rid of it.

  Paul

> >
> > -#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI)
> > +#if defined(CONFIG_HAS_PASSTHROUGH)
> > +    int (*iommu_ctl) (struct domain *d, unsigned int op);
> > +#if defined(CONFIG_HAS_PCI)
> >       int (*get_device_group) (uint32_t machine_bdf);
> >       int (*assign_device) (struct domain *d, uint32_t machine_bdf);
> >       int (*deassign_device) (struct domain *d, uint32_t machine_bdf);
> >   #endif
> > -
> > -#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_DEVICE_TREE)
> > +#if defined(CONFIG_HAS_DEVICE_TREE)
> >       int (*assign_dtdevice) (struct domain *d, const char *dtpath);
> >       int (*deassign_dtdevice) (struct domain *d, const char *dtpath);
> > +#endif
> >   #endif
> 
> Cheers,
> 
> --
> Julien Grall