Re: [Xen-devel] [PATCH v6 10/10] introduce a 'passthrough' configuration option to xl.cfg...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 29 August 2019 15:07
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Julien Grall <julien.grall@xxxxxxx>; 
> Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Anthony Perard <anthony.perard@xxxxxxxxxx>; 
> Roger Pau Monne
> <roger.pau@xxxxxxxxxx>; Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>; 
> George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Stefano 
> Stabellini
> <sstabellini@xxxxxxxxxx>; Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Tim 
> (Xen.org) <tim@xxxxxxx>;
> Wei Liu <wl@xxxxxxx>
> Subject: Re: [PATCH v6 10/10] introduce a 'passthrough' configuration option 
> to xl.cfg...
> 
> On 16.08.2019 19:20, Paul Durrant wrote:
> > ...and hence the ability to disable IOMMU mappings, and control EPT
> > sharing.
> >
> > This patch introduces a new 'libxl_passthrough' enumeration into
> > libxl_domain_create_info. The value will be set by xl either when it parses
> > a new 'passthrough' option in xl.cfg, or implicitly if there is passthrough
> > hardware specified for the domain.
> >
> > If the value of the passthrough configuration option is 'disabled' then
> > the XEN_DOMCTL_CDF_iommu flag will be clear in the xen_domctl_createdomain
> > flags, thus allowing the toolstack to control whether the domain gets
> > IOMMU mappings or not (where previously they were globally set).
> >
> > If the value of the passthrough configuration option is 'sync_pt' then
> > a new 'iommu_opts' field in xen_domctl_createdomain will be set with the
> > value XEN_DOMCTL_IOMMU_no_sharept. This will override the global default
> > set in iommu_hap_pt_share, thus allowing the toolstack to control whether
> > EPT sharing is used for the domain.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

> with a question/suggestion and a nit:
> 
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -308,6 +308,13 @@ static int sanitise_domain_config(struct 
> > xen_domctl_createdomain *config)
> >          return -EINVAL;
> >      }
> >
> > +    if ( !(config->flags & XEN_DOMCTL_CDF_iommu) && config->iommu_opts )
> > +    {
> > +        dprintk(XENLOG_INFO,
> > +                "IOMMU options specified but IOMMU not enabled\n");
> > +        return -EINVAL;
> > +    }
> 
> Seeing this I wonder whether XEN_DOMCTL_CDF_iommu wouldn't better be
> bit 0 of iommu_opts.
> 

I debated this with myself and I prefer to stick with separate flag and options.

> > @@ -70,6 +70,10 @@ struct xen_domctl_createdomain {
> >
> >      uint32_t flags;
> >
> > +#define _XEN_DOMCTL_IOMMU_no_sharept  0
> > +#define XEN_DOMCTL_IOMMU_no_sharept   (1U<<_XEN_DOMCTL_IOMMU_no_sharept)
> 
> Please can you add the missing blanks around << ?
> 

Sure.

  Paul

> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel