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

From: Jan Beulich <jbeulich@suse.com>
To: Paul Durrant <paul.durrant@citrix.com>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"George Dunlap" <George.Dunlap@eu.citrix.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"Tim Deegan" <tim@xen.org>, "Julien Grall" <julien.grall@arm.com>,
	"Anthony PERARD" <anthony.perard@citrix.com>,
	xen-devel@lists.xenproject.org,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v6 10/10] introduce a 'passthrough' configuration option to xl.cfg...
Date: Thu, 29 Aug 2019 16:07:24 +0200	[thread overview]
Message-ID: <6eaff8e6-126a-dffe-9051-4e3b31e3e240@suse.com> (raw)
In-Reply-To: <20190816172001.3905-11-paul.durrant@citrix.com>

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@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
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.

> @@ -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 << ?

Jan

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