| Message ID | 20191010151111.22125-10-ian.jackson@eu.citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libxl memkb & pt defaulting | expand |
On Thu, Oct 10, 2019 at 04:11:11PM +0100, Ian Jackson wrote: > LIBXL_PASSTHROUGH_UNKNOWN (aka "ENABLED" in an earlier uncommitted > version of this code) is doing double duty. We actually need all of > the following to be specificable: > * default ("unknown"): enable PT iff we have devices to > pass through specified in the initial config file. > * "enabled" (and fail if the platform doesn't support it). > * "disabled" (and reject future PT hotplug). > * "share_pt"/"sync_pt": enable PT and set a specific PT mode. > > Defaulting and error checking should be done in libxl. So, we make > several changes here. > > We introduce "enabled". (And we document "unknown".) > > We move all of the error checking and defaulting code from xl into > libxl. Now, libxl__domain_config_setdefault has all of the necessary > information to get this right. So we can do it all there, in one > place. > > We can also arrange to have only one place each which calculates > (i) whether passthrough needs to be enabled because pt devices were > specified (ii) whether pt_share can be used. > > xl now only has to parse the enum in the same way as it parses all > other enums. > > This change fixes a regression from earlier 4.13-pre: until recent > changes, passthrough was only enabled by default if passthrough > devices were specified. We restore this behaviour. > > This will hide, from the point of view of libvirt tests in osstest, a > separate hypervisor regression which prevents migration of a domain > with passthrough enabled but without actual PT devices. I think Jan committed a patch to fix that, so this may need deleting. I will let Jan and Paul confirm this. Wei.
On 10/10/2019 16:11, Ian Jackson wrote: > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > index 69971c97b6..fccb6a6271 100644 > --- a/tools/libxl/libxl_create.c > +++ b/tools/libxl/libxl_create.c > @@ -968,6 +957,50 @@ int libxl__domain_config_setdefault(libxl__gc *gc, > goto error_out; > } > > + bool need_pt = d_config->num_pcidevs || d_config->num_dtdevs; > + if (c_info->passthrough == LIBXL_PASSTHROUGH_UNKNOWN) { > + c_info->passthrough = need_pt > + ? LIBXL_PASSTHROUGH_ENABLED : LIBXL_PASSTHROUGH_DISABLED; > + } > + > + bool iommu_enabled = physinfo.cap_hvm_directio; > + if (c_info->passthrough != LIBXL_PASSTHROUGH_DISABLED && !iommu_enabled) { > + LOGD(ERROR, domid, > + "ERROR: passthrough not supported on this platform\n"); > + ret = ERROR_INVAL; > + goto error_out; > + } > + > + if (c_info->passthrough == LIBXL_PASSTHROUGH_DISABLED && need_pt) { > + LOGD(ERROR, domid, > + "passthrough disabled but devices are specified"); This is the only log message which isn't prefixed with ERROR: > + ret = ERROR_INVAL; > + goto error_out; > + } > + > + const char *whynot_pt_share = > + c_info->type == LIBXL_DOMAIN_TYPE_PV ? "not valid for PV domain" : > + !physinfo.cap_iommu_hap_pt_share ? "not supported on this platform" : > + NULL; This is a little more complicated. For ARM, doesn't libxl treat guests as PV, or has that been fixed now? ARM's only passthrough mode is PT_SHARE. On x86 for PVH, passthrough doesn't work yet. This may not be an argument against constructing the guest suitably, but we should check that things don't explode in new and interesting ways from this change. For x86 HVM, PT_SHARE is only available for HAP guests, so shadow guests must use PT_SYNC. > + > + if (c_info->passthrough == LIBXL_PASSTHROUGH_ENABLED) { > + assert(iommu_enabled); > + c_info->passthrough = whynot_pt_share > + ? LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT; > + } > + > + if (c_info->passthrough == LIBXL_PASSTHROUGH_SHARE_PT && whynot_pt_share) { > + LOGD(ERROR, domid, > + "ERROR: passthrough=\"share_pt\" %s\n", > + whynot_pt_share); > + ret = ERROR_INVAL; > + goto error_out; > + } > + > + /* An explicit setting should now have been chosen */ > + assert(c_info->passthrough != LIBXL_PASSTHROUGH_UNKNOWN); > + assert(c_info->passthrough != LIBXL_PASSTHROUGH_ENABLED); This is confusing. I think it would help if ... > + > /* If target_memkb is smaller than max_memkb, the subsequent call > * to libxc when building HVM domain will enable PoD mode. > */ > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index 3ac9494b80..2441c0c233 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -266,8 +266,9 @@ libxl_vkb_backend = Enumeration("vkb_backend", [ > libxl_passthrough = Enumeration("passthrough", [ > (0, "unknown"), > (1, "disabled"), > - (2, "sync_pt"), > - (3, "share_pt"), > + (2, "enabled"), > + (3, "sync_pt"), > + (4, "share_pt"), ... this had a comment explaining enabled is just interim value. (2, "enabled"), # becomes {sync,share}_pt once defaults are evaluated ~Andrew
Andrew Cooper writes ("Re: [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> On 10/10/2019 16:11, Ian Jackson wrote:
> > + if (c_info->passthrough == LIBXL_PASSTHROUGH_DISABLED && need_pt) {
> > + LOGD(ERROR, domid,
> > + "passthrough disabled but devices are specified");
>
> This is the only log message which isn't prefixed with ERROR:
I will strip the ERROR: out of the others. I think I inherited them
from when this code was in xl, where it's just fprintf stderr. Here
the priority is an argument to LOGD.
> > + const char *whynot_pt_share =
> > + c_info->type == LIBXL_DOMAIN_TYPE_PV ? "not valid for PV domain" :
> > + !physinfo.cap_iommu_hap_pt_share ? "not supported on this platform" :
> > + NULL;
>
> This is a little more complicated.
I aimed to replicate the logic prior to my series. FTAOD I think this
means this was already broken in xl ? Anyway:
> For ARM, doesn't libxl treat guests as PV, or has that been fixed now?
> ARM's only passthrough mode is PT_SHARE.
I think this means that I need to move the calculation of
whynot_pt_share into arch-specific code. I'll wait and see what ARM
folks say.
> On x86 for PVH, passthrough doesn't work yet. This may not be an
> argument against constructing the guest suitably, but we should check
> that things don't explode in new and interesting ways from this change.
If we know it doesn't work, it's not a good idea to accept it. I will
arrange to reject it.
> For x86 HVM, PT_SHARE is only available for HAP guests, so shadow guests
> must use PT_SYNC.
I will add a check for c_info->hap.
> > + /* An explicit setting should now have been chosen */
> > + assert(c_info->passthrough != LIBXL_PASSTHROUGH_UNKNOWN);
> > + assert(c_info->passthrough != LIBXL_PASSTHROUGH_ENABLED);
>
> This is confusing. I think it would help if ...
...
> ... this had a comment explaining enabled is just interim value.
>
> (2, "enabled"), # becomes {sync,share}_pt once defaults are evaluated
Good idea, thanks.
Ian.
Hi, On 11/10/2019 10:47, Andrew Cooper wrote: > On 10/10/2019 16:11, Ian Jackson wrote: >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c >> index 69971c97b6..fccb6a6271 100644 >> --- a/tools/libxl/libxl_create.c >> +++ b/tools/libxl/libxl_create.c >> @@ -968,6 +957,50 @@ int libxl__domain_config_setdefault(libxl__gc *gc, >> goto error_out; >> } >> >> + bool need_pt = d_config->num_pcidevs || d_config->num_dtdevs; >> + if (c_info->passthrough == LIBXL_PASSTHROUGH_UNKNOWN) { >> + c_info->passthrough = need_pt >> + ? LIBXL_PASSTHROUGH_ENABLED : LIBXL_PASSTHROUGH_DISABLED; >> + } >> + >> + bool iommu_enabled = physinfo.cap_hvm_directio; >> + if (c_info->passthrough != LIBXL_PASSTHROUGH_DISABLED && !iommu_enabled) { >> + LOGD(ERROR, domid, >> + "ERROR: passthrough not supported on this platform\n"); >> + ret = ERROR_INVAL; >> + goto error_out; >> + } >> + >> + if (c_info->passthrough == LIBXL_PASSTHROUGH_DISABLED && need_pt) { >> + LOGD(ERROR, domid, >> + "passthrough disabled but devices are specified"); > > This is the only log message which isn't prefixed with ERROR: > >> + ret = ERROR_INVAL; >> + goto error_out; >> + } >> + >> + const char *whynot_pt_share = >> + c_info->type == LIBXL_DOMAIN_TYPE_PV ? "not valid for PV domain" : >> + !physinfo.cap_iommu_hap_pt_share ? "not supported on this platform" : >> + NULL; > > This is a little more complicated. > > For ARM, doesn't libxl treat guests as PV, or has that been fixed now? libxl treats Arm guest as PVH now. Note that we seamlessly convert PV to PVH in libxl__arch_domain_{build, create}_info_setdefault(). So as long as this is called after any of those calls, then we should be fine. > ARM's only passthrough mode is PT_SHARE. Correct. Cheers,
On Thu, Oct 10, 2019 at 4:12 PM Ian Jackson <ian.jackson@eu.citrix.com> wrote: > > LIBXL_PASSTHROUGH_UNKNOWN (aka "ENABLED" in an earlier uncommitted > version of this code) is doing double duty. We actually need all of > the following to be specificable: > * default ("unknown"): enable PT iff we have devices to > pass through specified in the initial config file. I realize this may be a bit late, but I find "unknown" to be a very strange word to use to indicate, "please choose the best option for me". For USB device type I used "auto", meaning, "automatically choose the best option for me". Paul didn't like "auto", which is fair enough, but I really don't see how "unknown" is better. Anyway, not meaning to block, just a suggestion. -George
George Dunlap writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> On Thu, Oct 10, 2019 at 4:12 PM Ian Jackson <ian.jackson@eu.citrix.com> wrote:
> > LIBXL_PASSTHROUGH_UNKNOWN (aka "ENABLED" in an earlier uncommitted
> > version of this code) is doing double duty. We actually need all of
> > the following to be specificable:
> > * default ("unknown"): enable PT iff we have devices to
> > pass through specified in the initial config file.
>
> I realize this may be a bit late, but I find "unknown" to be a very
> strange word to use to indicate, "please choose the best option for
> me". For USB device type I used "auto", meaning, "automatically
> choose the best option for me". Paul didn't like "auto", which is
> fair enough, but I really don't see how "unknown" is better.
>
> Anyway, not meaning to block, just a suggestion.
I do not have a strong opinion about this. I would be happy with
"auto" (or "default" maybe).
None of this was in 4.12 so we are still free to change it. Now would
be the time!
Ian.
Julien Grall writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> libxl treats Arm guest as PVH now. Note that we seamlessly convert
> PV to PVH in libxl__arch_domain_{build, create}_info_setdefault().
>
> So as long as this is called after any of those calls, then we
> should be fine.
I'll check this.
This means that the algorithm is quite different on ARM to on x86:
ARM guests are always PVH and need PT_SHARE; on x86, PVH is not
compatible with passthrough at all.
So I need to put this logic in an arch-specific function.
Ian.
On 11.10.19 15:31, Ian Jackson wrote: > George Dunlap writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"): >> On Thu, Oct 10, 2019 at 4:12 PM Ian Jackson <ian.jackson@eu.citrix.com> wrote: >>> LIBXL_PASSTHROUGH_UNKNOWN (aka "ENABLED" in an earlier uncommitted >>> version of this code) is doing double duty. We actually need all of >>> the following to be specificable: >>> * default ("unknown"): enable PT iff we have devices to >>> pass through specified in the initial config file. >> >> I realize this may be a bit late, but I find "unknown" to be a very >> strange word to use to indicate, "please choose the best option for >> me". For USB device type I used "auto", meaning, "automatically >> choose the best option for me". Paul didn't like "auto", which is >> fair enough, but I really don't see how "unknown" is better. >> >> Anyway, not meaning to block, just a suggestion. > > I do not have a strong opinion about this. I would be happy with > "auto" (or "default" maybe). "unspecified"? Juergen
Jürgen Groß writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> On 11.10.19 15:31, Ian Jackson wrote:
> > I do not have a strong opinion about this. I would be happy with
> > "auto" (or "default" maybe).
>
> "unspecified"?
That is IMO the best suggestion so far so I will go with that in my
v3.
Ian.
On Fri, 11 Oct 2019 at 17:34, Ian Jackson <ian.jackson@citrix.com> wrote: > > Jürgen Groß writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"): > > On 11.10.19 15:31, Ian Jackson wrote: > > > I do not have a strong opinion about this. I would be happy with > > > "auto" (or "default" maybe). > > > > "unspecified"? > > That is IMO the best suggestion so far so I will go with that in my > v3. Seems odd to specify a parameter with a value of 'unspecified' ;-) Paul > > Ian.
Paul Durrant writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> On Fri, 11 Oct 2019 at 17:34, Ian Jackson <ian.jackson@citrix.com> wrote:
> >
> > Jürgen Groß writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> > > On 11.10.19 15:31, Ian Jackson wrote:
> > > > I do not have a strong opinion about this. I would be happy with
> > > > "auto" (or "default" maybe).
> > >
> > > "unspecified"?
> >
> > That is IMO the best suggestion so far so I will go with that in my
> > v3.
>
> Seems odd to specify a parameter with a value of 'unspecified' ;-)
I have tried to infer +1/-1/0 numbers from the mail thread. I have
also looked at libxl_types.idl to see how many times we are using
what name to represent roughly this concept:
Bikeshed colour Paul Juergen George Ian Anthony Wei #already
unknown ? ? -1 +2 ? ? 17
default ? ? ? 0 ? ? 2
auto -1 ? +1 0 ? ? 1
unspecified -1 +1 ? 0 ? ? 0
^^^^^^^^^^^^^^^^
libxl maintainers
On this basis IMO clearly this should go back to "unknown".
I will do that in a respin (or maybe on commit) but right now I think
I am still awaiting a review for this patch.
Thanks,
Ian.
On Mon, Oct 14, 2019 at 05:09:24PM +0100, Ian Jackson wrote: > Paul Durrant writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"): > > On Fri, 11 Oct 2019 at 17:34, Ian Jackson <ian.jackson@citrix.com> wrote: > > > > > > Jürgen Groß writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"): > > > > On 11.10.19 15:31, Ian Jackson wrote: > > > > > I do not have a strong opinion about this. I would be happy with > > > > > "auto" (or "default" maybe). > > > > > > > > "unspecified"? > > > > > > That is IMO the best suggestion so far so I will go with that in my > > > v3. > > > > Seems odd to specify a parameter with a value of 'unspecified' ;-) > > I have tried to infer +1/-1/0 numbers from the mail thread. I have > also looked at libxl_types.idl to see how many times we are using > what name to represent roughly this concept: > > Bikeshed colour Paul Juergen George Ian Anthony Wei #already > > unknown ? ? -1 +2 ? ? 17 > default ? ? ? 0 ? ? 2 > auto -1 ? +1 0 ? ? 1 > unspecified -1 +1 ? 0 ? ? 0 > > ^^^^^^^^^^^^^^^^ > libxl maintainers +1 to "unknown". I prefer consistency. 0 to all others. > > On this basis IMO clearly this should go back to "unknown". > I will do that in a respin (or maybe on commit) but right now I think > I am still awaiting a review for this patch. > I think a respin is required -- in one of the mails your said you would need to put some logic into arch-specific function. Wei. > Thanks, > Ian.
Wei Liu writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"):
> On Mon, Oct 14, 2019 at 05:09:24PM +0100, Ian Jackson wrote:
> > Bikeshed colour Paul Juergen George Ian Anthony Wei #already
> >
> > unknown ? ? -1 +2 ? ? 17
> > default ? ? ? 0 ? ? 2
> > auto -1 ? +1 0 ? ? 1
> > unspecified -1 +1 ? 0 ? ? 0
> >
> > ^^^^^^^^^^^^^^^^
> > libxl maintainers
>
> +1 to "unknown". I prefer consistency.
>
> 0 to all others.
Thanks.
> > On this basis IMO clearly this should go back to "unknown".
> > I will do that in a respin (or maybe on commit) but right now I think
> > I am still awaiting a review for this patch.
>
> I think a respin is required -- in one of the mails your said you would
> need to put some logic into arch-specific function.
Yes. Already done.
This is confusing because we are in the thread re v2, which is where
this bikeshed conversation is happening.
But there is also a v3, see:
Subject: Re: [Xen-devel] [XEN PATCH for-4.13 v3 10/10] libxl/xl: Overhaul
passthrough setting logic
That's been R-B Anthony but I have now changed "unspecified" back to
"unknown"...
Ian.
On Mon, Oct 14, 2019 at 05:09:24PM +0100, Ian Jackson wrote: > Paul Durrant writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"): > > On Fri, 11 Oct 2019 at 17:34, Ian Jackson <ian.jackson@citrix.com> wrote: > > > > > > Jürgen Groß writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"): > > > > On 11.10.19 15:31, Ian Jackson wrote: > > > > > I do not have a strong opinion about this. I would be happy with > > > > > "auto" (or "default" maybe). > > > > > > > > "unspecified"? > > > > > > That is IMO the best suggestion so far so I will go with that in my > > > v3. > > > > Seems odd to specify a parameter with a value of 'unspecified' ;-) > > I have tried to infer +1/-1/0 numbers from the mail thread. I have > also looked at libxl_types.idl to see how many times we are using > what name to represent roughly this concept: > > Bikeshed colour Paul Juergen George Ian Anthony Wei #already > > unknown ? ? -1 +2 ? ? 17 > default ? ? ? 0 ? ? 2 > auto -1 ? +1 0 ? ? 1 > unspecified -1 +1 ? 0 ? ? 0 > > ^^^^^^^^^^^^^^^^ > libxl maintainers Maybe "unknown" is more used in the API, but when I look at the manpage "unknown" value as never been used before. On the other hand "default" as been used twice in the man page. (and one "defaults" and two other "default" that I'm not sure if they can be in the config file.) So I would say +2 for default and +1 for unknown.
On Mon, Oct 14, 2019 at 05:59:26PM +0100, Anthony PERARD wrote: > On Mon, Oct 14, 2019 at 05:09:24PM +0100, Ian Jackson wrote: > > Paul Durrant writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"): > > > On Fri, 11 Oct 2019 at 17:34, Ian Jackson <ian.jackson@citrix.com> wrote: > > > > > > > > Jürgen Groß writes ("Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic"): > > > > > On 11.10.19 15:31, Ian Jackson wrote: > > > > > > I do not have a strong opinion about this. I would be happy with > > > > > > "auto" (or "default" maybe). > > > > > > > > > > "unspecified"? > > > > > > > > That is IMO the best suggestion so far so I will go with that in my > > > > v3. > > > > > > Seems odd to specify a parameter with a value of 'unspecified' ;-) > > > > I have tried to infer +1/-1/0 numbers from the mail thread. I have > > also looked at libxl_types.idl to see how many times we are using > > what name to represent roughly this concept: > > > > Bikeshed colour Paul Juergen George Ian Anthony Wei #already > > > > unknown ? ? -1 +2 ? ? 17 > > default ? ? ? 0 ? ? 2 > > auto -1 ? +1 0 ? ? 1 > > unspecified -1 +1 ? 0 ? ? 0 > > > > ^^^^^^^^^^^^^^^^ > > libxl maintainers > > Maybe "unknown" is more used in the API, but when I look at the manpage > "unknown" value as never been used before. On the other hand "default" > as been used twice in the man page. (and one "defaults" and two other > "default" that I'm not sure if they can be in the config file.) > > So I would say +2 for default and +1 for unknown. Either is fine as long as it is clearly documented. If you feel strongly about "default", so be it. :-) Wei. > > -- > Anthony PERARD
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 69971c97b6..fccb6a6271 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -57,18 +57,6 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc, if (!c_info->ssidref) c_info->ssidref = SECINITSID_DOMU; - if (info->cap_hvm_directio && - (c_info->passthrough == LIBXL_PASSTHROUGH_UNKNOWN)) { - c_info->passthrough = ((c_info->type == LIBXL_DOMAIN_TYPE_PV) || - !info->cap_iommu_hap_pt_share) ? - LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT; - } else if (!info->cap_hvm_directio) { - c_info->passthrough = LIBXL_PASSTHROUGH_DISABLED; - } - - /* An explicit setting should now have been chosen */ - assert(c_info->passthrough != LIBXL_PASSTHROUGH_UNKNOWN); - return 0; } @@ -904,6 +892,7 @@ int libxl__domain_config_setdefault(libxl__gc *gc, libxl_ctx *ctx = libxl__gc_owner(gc); int ret; bool pod_enabled = false; + libxl_domain_create_info *c_info = &d_config->c_info; libxl_physinfo physinfo; ret = libxl_get_physinfo(CTX, &physinfo); @@ -968,6 +957,50 @@ int libxl__domain_config_setdefault(libxl__gc *gc, goto error_out; } + bool need_pt = d_config->num_pcidevs || d_config->num_dtdevs; + if (c_info->passthrough == LIBXL_PASSTHROUGH_UNKNOWN) { + c_info->passthrough = need_pt + ? LIBXL_PASSTHROUGH_ENABLED : LIBXL_PASSTHROUGH_DISABLED; + } + + bool iommu_enabled = physinfo.cap_hvm_directio; + if (c_info->passthrough != LIBXL_PASSTHROUGH_DISABLED && !iommu_enabled) { + LOGD(ERROR, domid, + "ERROR: passthrough not supported on this platform\n"); + ret = ERROR_INVAL; + goto error_out; + } + + if (c_info->passthrough == LIBXL_PASSTHROUGH_DISABLED && need_pt) { + LOGD(ERROR, domid, + "passthrough disabled but devices are specified"); + ret = ERROR_INVAL; + goto error_out; + } + + const char *whynot_pt_share = + c_info->type == LIBXL_DOMAIN_TYPE_PV ? "not valid for PV domain" : + !physinfo.cap_iommu_hap_pt_share ? "not supported on this platform" : + NULL; + + if (c_info->passthrough == LIBXL_PASSTHROUGH_ENABLED) { + assert(iommu_enabled); + c_info->passthrough = whynot_pt_share + ? LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT; + } + + if (c_info->passthrough == LIBXL_PASSTHROUGH_SHARE_PT && whynot_pt_share) { + LOGD(ERROR, domid, + "ERROR: passthrough=\"share_pt\" %s\n", + whynot_pt_share); + ret = ERROR_INVAL; + goto error_out; + } + + /* An explicit setting should now have been chosen */ + assert(c_info->passthrough != LIBXL_PASSTHROUGH_UNKNOWN); + assert(c_info->passthrough != LIBXL_PASSTHROUGH_ENABLED); + /* If target_memkb is smaller than max_memkb, the subsequent call * to libxc when building HVM domain will enable PoD mode. */ diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index 3ac9494b80..2441c0c233 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -266,8 +266,9 @@ libxl_vkb_backend = Enumeration("vkb_backend", [ libxl_passthrough = Enumeration("passthrough", [ (0, "unknown"), (1, "disabled"), - (2, "sync_pt"), - (3, "share_pt"), + (2, "enabled"), + (3, "sync_pt"), + (4, "share_pt"), ]) # diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c index 79871c22d0..112f8ee026 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -1222,7 +1222,6 @@ void parse_config_data(const char *config_source, int pci_seize = 0; int i, e; char *kernel_basename; - bool iommu_enabled, iommu_hap_pt_share; libxl_domain_create_info *c_info = &d_config->c_info; libxl_domain_build_info *b_info = &d_config->b_info; @@ -1234,8 +1233,6 @@ void parse_config_data(const char *config_source, exit(EXIT_FAILURE); } - iommu_enabled = physinfo.cap_hvm_directio; - iommu_hap_pt_share = physinfo.cap_iommu_hap_pt_share; libxl_physinfo_dispose(&physinfo); config= xlu_cfg_init(stderr, config_source); @@ -1509,67 +1506,13 @@ void parse_config_data(const char *config_source, } } - if (xlu_cfg_get_string(config, "passthrough", &buf, 0)) { - c_info->passthrough = - (d_config->num_pcidevs || d_config->num_dtdevs) - ? LIBXL_PASSTHROUGH_UNKNOWN : LIBXL_PASSTHROUGH_DISABLED; - } else { - if (!strcasecmp("enabled", buf)) - c_info->passthrough = LIBXL_PASSTHROUGH_UNKNOWN; - else { - libxl_passthrough o; - - e = libxl_passthrough_from_string(buf, &o); - if (e || !strcasecmp("unknown", buf)) { - fprintf(stderr, - "ERROR: unknown passthrough option '%s'\n", - buf); - exit(-ERROR_FAIL); - } - - c_info->passthrough = o; - } - } - - switch (c_info->passthrough) { - case LIBXL_PASSTHROUGH_UNKNOWN: - /* - * Choose a suitable default. libxl would also do this but - * choosing here allows the code calculating 'iommu_memkb' - * below make an informed decision. - */ - c_info->passthrough = - (c_info->type == LIBXL_DOMAIN_TYPE_PV) || !iommu_hap_pt_share - ? LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT; - break; - - case LIBXL_PASSTHROUGH_DISABLED: - if (d_config->num_pcidevs || d_config->num_dtdevs) { + if (!xlu_cfg_get_string(config, "passthrough", &buf, 0)) { + if (libxl_passthrough_from_string(buf, &c_info->passthrough)) { fprintf(stderr, - "ERROR: passthrough disabled but devices are specified\n"); - exit(-ERROR_FAIL); - } - break; - case LIBXL_PASSTHROUGH_SHARE_PT: - if (c_info->type == LIBXL_DOMAIN_TYPE_PV) { - fprintf(stderr, - "ERROR: passthrough=\"share_pt\" not valid for PV domain\n"); - exit(-ERROR_FAIL); - } else if (!iommu_hap_pt_share) { - fprintf(stderr, - "ERROR: passthrough=\"share_pt\" not supported on this platform\n"); - exit(-ERROR_FAIL); + "ERROR: unknown passthrough option '%s'\n", + buf); + exit(1); } - break; - case LIBXL_PASSTHROUGH_SYNC_PT: - break; - } - - if ((c_info->passthrough != LIBXL_PASSTHROUGH_DISABLED) && - !iommu_enabled) { - fprintf(stderr, - "ERROR: passthrough not supported on this platform\n"); - exit(-ERROR_FAIL); } if (!xlu_cfg_get_long(config, "shadow_memory", &l, 0))
LIBXL_PASSTHROUGH_UNKNOWN (aka "ENABLED" in an earlier uncommitted version of this code) is doing double duty. We actually need all of the following to be specificable: * default ("unknown"): enable PT iff we have devices to pass through specified in the initial config file. * "enabled" (and fail if the platform doesn't support it). * "disabled" (and reject future PT hotplug). * "share_pt"/"sync_pt": enable PT and set a specific PT mode. Defaulting and error checking should be done in libxl. So, we make several changes here. We introduce "enabled". (And we document "unknown".) We move all of the error checking and defaulting code from xl into libxl. Now, libxl__domain_config_setdefault has all of the necessary information to get this right. So we can do it all there, in one place. We can also arrange to have only one place each which calculates (i) whether passthrough needs to be enabled because pt devices were specified (ii) whether pt_share can be used. xl now only has to parse the enum in the same way as it parses all other enums. This change fixes a regression from earlier 4.13-pre: until recent changes, passthrough was only enabled by default if passthrough devices were specified. We restore this behaviour. This will hide, from the point of view of libvirt tests in osstest, a separate hypervisor regression which prevents migration of a domain with passthrough enabled but without actual PT devices. Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> --- v2: New patch in this version of the series. --- tools/libxl/libxl_create.c | 57 ++++++++++++++++++++++++++++++-------- tools/libxl/libxl_types.idl | 5 ++-- tools/xl/xl_parse.c | 67 ++++----------------------------------------- 3 files changed, 53 insertions(+), 76 deletions(-)