| Message ID | 20190905093416.2955-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/libxl: choose a sane default for HAP | expand |
> -----Original Message----- [snip] > -void libxl__arch_domain_create_info_setdefault(libxl__gc *gc, > - libxl_domain_create_info *c_info) > +int libxl__arch_domain_create_info_setdefault(libxl__gc *gc, > + libxl_domain_create_info *c_info) > { > + libxl_physinfo pi; > + int rc = libxl_get_physinfo(CTX, &pi); > + > + if (rc) { > + LOG(ERROR, "unable to get physinfo"); > + return rc; > + } > + > + libxl_defbool_setdefault(&c_info->hap, pi.cap_hap); Is this going to work on ARM (where CDF_hap is required)? Because... > + > + return 0; > } > > void libxl__arch_domain_build_info_setdefault(libxl__gc *gc, > diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c > index c50d910a1c..74ea184087 100644 > --- a/xen/arch/x86/sysctl.c > +++ b/xen/arch/x86/sysctl.c > @@ -165,6 +165,8 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi) > pi->capabilities |= XEN_SYSCTL_PHYSCAP_pv; > if ( iommu_enabled ) > pi->capabilities |= XEN_SYSCTL_PHYSCAP_directio; > + if ( hvm_hap_supported() ) > + pi->capabilities |= XEN_SYSCTL_PHYSCAP_hap; ...this is x86-only code, and I don't see an equivalent hunk for ARM. Paul > } > > long arch_do_sysctl( > diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h > index 91c48dcae0..6c457625e9 100644 > --- a/xen/include/public/sysctl.h > +++ b/xen/include/public/sysctl.h > @@ -90,6 +90,10 @@ struct xen_sysctl_tbuf_op { > /* (x86) The platform supports direct access to I/O devices with IOMMU. */ > #define _XEN_SYSCTL_PHYSCAP_directio 2 > #define XEN_SYSCTL_PHYSCAP_directio (1u<<_XEN_SYSCTL_PHYSCAP_directio) > +/* (x86) The platform supports Hardware Assisted Paging. */ > +#define _XEN_SYSCTL_PHYSCAP_hap 3 > +#define XEN_SYSCTL_PHYSCAP_hap (1u<<_XEN_SYSCTL_PHYSCAP_hap) > + > struct xen_sysctl_physinfo { > uint32_t threads_per_core; > uint32_t cores_per_socket; > -- > 2.22.0
On 05.09.2019 11:34, Roger Pau Monne wrote: > Current libxl code will always enable Hardware Assisted Paging (HAP), > expecting that the hypervisor will fallback to shadow if HAP is not > available. With the changes to the domain builder that's not the case > any longer, and the hypervisor will raise an error if HAP is not > available instead of silently falling back to shadow. Would it really be much more involved than the change here to restore silent defaulting to shadow? > --- a/xen/include/public/sysctl.h > +++ b/xen/include/public/sysctl.h > @@ -90,6 +90,10 @@ struct xen_sysctl_tbuf_op { > /* (x86) The platform supports direct access to I/O devices with IOMMU. */ > #define _XEN_SYSCTL_PHYSCAP_directio 2 > #define XEN_SYSCTL_PHYSCAP_directio (1u<<_XEN_SYSCTL_PHYSCAP_directio) > +/* (x86) The platform supports Hardware Assisted Paging. */ > +#define _XEN_SYSCTL_PHYSCAP_hap 3 > +#define XEN_SYSCTL_PHYSCAP_hap (1u<<_XEN_SYSCTL_PHYSCAP_hap) I think this bit wants to be universal (i.e. "(x86)" dropped), and be set unconditionally on Arm. Irrespective of the question regarding an alternative solution I think it is quite sensible to expose availability of HAP to the tools. In fact I think "xl info" should show this alongside other properties. Jan
On Thu, Sep 05, 2019 at 11:40:19AM +0200, Paul Durrant wrote: > > -----Original Message----- > [snip] > > -void libxl__arch_domain_create_info_setdefault(libxl__gc *gc, > > - libxl_domain_create_info *c_info) > > +int libxl__arch_domain_create_info_setdefault(libxl__gc *gc, > > + libxl_domain_create_info *c_info) > > { > > + libxl_physinfo pi; > > + int rc = libxl_get_physinfo(CTX, &pi); > > + > > + if (rc) { > > + LOG(ERROR, "unable to get physinfo"); > > + return rc; > > + } > > + > > + libxl_defbool_setdefault(&c_info->hap, pi.cap_hap); > > Is this going to work on ARM (where CDF_hap is required)? Because... It should, libxl__arch_domain_create_info_setdefault for ARM sets hap to true unconditionally. > > + > > + return 0; > > } > > > > void libxl__arch_domain_build_info_setdefault(libxl__gc *gc, > > diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c > > index c50d910a1c..74ea184087 100644 > > --- a/xen/arch/x86/sysctl.c > > +++ b/xen/arch/x86/sysctl.c > > @@ -165,6 +165,8 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi) > > pi->capabilities |= XEN_SYSCTL_PHYSCAP_pv; > > if ( iommu_enabled ) > > pi->capabilities |= XEN_SYSCTL_PHYSCAP_directio; > > + if ( hvm_hap_supported() ) > > + pi->capabilities |= XEN_SYSCTL_PHYSCAP_hap; > > ...this is x86-only code, and I don't see an equivalent hunk for ARM. Yes, that flag is x86 only ATM (like all other capability flags). Thanks, Roger.
On 05/09/2019 10:52, Jan Beulich wrote: > On 05.09.2019 11:34, Roger Pau Monne wrote: >> Current libxl code will always enable Hardware Assisted Paging (HAP), >> expecting that the hypervisor will fallback to shadow if HAP is not >> available. With the changes to the domain builder that's not the case >> any longer, and the hypervisor will raise an error if HAP is not >> available instead of silently falling back to shadow. > Would it really be much more involved than the change here to restore > silent defaulting to shadow? We could, but I would object to doing so. It is not sensible behaviour to have, because the only thing it does is hide (mis)configuration issues. > >> --- a/xen/include/public/sysctl.h >> +++ b/xen/include/public/sysctl.h >> @@ -90,6 +90,10 @@ struct xen_sysctl_tbuf_op { >> /* (x86) The platform supports direct access to I/O devices with IOMMU. */ >> #define _XEN_SYSCTL_PHYSCAP_directio 2 >> #define XEN_SYSCTL_PHYSCAP_directio (1u<<_XEN_SYSCTL_PHYSCAP_directio) >> +/* (x86) The platform supports Hardware Assisted Paging. */ >> +#define _XEN_SYSCTL_PHYSCAP_hap 3 >> +#define XEN_SYSCTL_PHYSCAP_hap (1u<<_XEN_SYSCTL_PHYSCAP_hap) > I think this bit wants to be universal (i.e. "(x86)" dropped), and > be set unconditionally on Arm. Irrespective of the question regarding > an alternative solution I think it is quite sensible to expose > availability of HAP to the tools. In fact I think "xl info" should > show this alongside other properties. I agree. While terms like HVM and HAP may have come about in the x86 world, they are deliberately vendor and technology neutral. ARM *should* be claim to be, and behave as, HVM-only HAP-only system, per this nomenclature. ~Andrew
On Thu, Sep 05, 2019 at 11:52:59AM +0200, Jan Beulich wrote: > On 05.09.2019 11:34, Roger Pau Monne wrote: > > Current libxl code will always enable Hardware Assisted Paging (HAP), > > expecting that the hypervisor will fallback to shadow if HAP is not > > available. With the changes to the domain builder that's not the case > > any longer, and the hypervisor will raise an error if HAP is not > > available instead of silently falling back to shadow. > > Would it really be much more involved than the change here to restore > silent defaulting to shadow? But that would mean that a user having selected hap=1 on the config file would get silently defaulted to shadow, which is wrong IMO. > > --- a/xen/include/public/sysctl.h > > +++ b/xen/include/public/sysctl.h > > @@ -90,6 +90,10 @@ struct xen_sysctl_tbuf_op { > > /* (x86) The platform supports direct access to I/O devices with IOMMU. */ > > #define _XEN_SYSCTL_PHYSCAP_directio 2 > > #define XEN_SYSCTL_PHYSCAP_directio (1u<<_XEN_SYSCTL_PHYSCAP_directio) > > +/* (x86) The platform supports Hardware Assisted Paging. */ > > +#define _XEN_SYSCTL_PHYSCAP_hap 3 > > +#define XEN_SYSCTL_PHYSCAP_hap (1u<<_XEN_SYSCTL_PHYSCAP_hap) > > I think this bit wants to be universal (i.e. "(x86)" dropped), and > be set unconditionally on Arm. Irrespective of the question regarding > an alternative solution I think it is quite sensible to expose > availability of HAP to the tools. In fact I think "xl info" should > show this alongside other properties. I can indeed make this available to both x86 and ARM. AFAICT it's always going to be true on ARM. I can expand this a bit so it's also printed in `xl info` together with the rest of the virt_caps. Thanks, Roger.
On 05.09.2019 12:01, Roger Pau Monné wrote: > On Thu, Sep 05, 2019 at 11:52:59AM +0200, Jan Beulich wrote: >> On 05.09.2019 11:34, Roger Pau Monne wrote: >>> Current libxl code will always enable Hardware Assisted Paging (HAP), >>> expecting that the hypervisor will fallback to shadow if HAP is not >>> available. With the changes to the domain builder that's not the case >>> any longer, and the hypervisor will raise an error if HAP is not >>> available instead of silently falling back to shadow. >> >> Would it really be much more involved than the change here to restore >> silent defaulting to shadow? > > But that would mean that a user having selected hap=1 on the config > file would get silently defaulted to shadow, which is wrong IMO. Since you and Andrew agree, so be it. Personally I'd like it better if "hap=1" didn't prevent a domain from starting on a HAP-incapable host. I wouldn't mind a warning though. Jan
On 05/09/2019 11:32, Jan Beulich wrote: > On 05.09.2019 12:01, Roger Pau Monné wrote: >> On Thu, Sep 05, 2019 at 11:52:59AM +0200, Jan Beulich wrote: >>> On 05.09.2019 11:34, Roger Pau Monne wrote: >>>> Current libxl code will always enable Hardware Assisted Paging (HAP), >>>> expecting that the hypervisor will fallback to shadow if HAP is not >>>> available. With the changes to the domain builder that's not the case >>>> any longer, and the hypervisor will raise an error if HAP is not >>>> available instead of silently falling back to shadow. >>> Would it really be much more involved than the change here to restore >>> silent defaulting to shadow? >> But that would mean that a user having selected hap=1 on the config >> file would get silently defaulted to shadow, which is wrong IMO. > Since you and Andrew agree, so be it. Personally I'd like it better > if "hap=1" didn't prevent a domain from starting on a HAP-incapable > host. I wouldn't mind a warning though. Behaviour like that could be arranged in libxl, which now has all the requisite pieces. What I'm not happy with is xc_domain_create() asking for a HAP domain and getting silently getting a shadow one. ~Andrew
> On Sep 5, 2019, at 11:01 AM, Roger Pau Monne <roger.pau@citrix.com> wrote: > > On Thu, Sep 05, 2019 at 11:52:59AM +0200, Jan Beulich wrote: >> On 05.09.2019 11:34, Roger Pau Monne wrote: >>> Current libxl code will always enable Hardware Assisted Paging (HAP), >>> expecting that the hypervisor will fallback to shadow if HAP is not >>> available. With the changes to the domain builder that's not the case >>> any longer, and the hypervisor will raise an error if HAP is not >>> available instead of silently falling back to shadow. >> >> Would it really be much more involved than the change here to restore >> silent defaulting to shadow? > > But that would mean that a user having selected hap=1 on the config > file would get silently defaulted to shadow, which is wrong IMO. At the libxl layer, aren’t the options tristate? I.e., this would be “hap”, “shadow”, or “not specified”? The user needs to be able to specify “always use shadow”, “always use HAP”, or “use HAP if available, otherwise use shadow”. At the moment, leaving it empty should be “use HAP if available, otherwise use shadow”; so “hap = 1” should fail if HAP is not available. -George
On Thu, Sep 05, 2019 at 12:34:11PM +0200, George Dunlap wrote: > > > > On Sep 5, 2019, at 11:01 AM, Roger Pau Monne <roger.pau@citrix.com> wrote: > > > > On Thu, Sep 05, 2019 at 11:52:59AM +0200, Jan Beulich wrote: > >> On 05.09.2019 11:34, Roger Pau Monne wrote: > >>> Current libxl code will always enable Hardware Assisted Paging (HAP), > >>> expecting that the hypervisor will fallback to shadow if HAP is not > >>> available. With the changes to the domain builder that's not the case > >>> any longer, and the hypervisor will raise an error if HAP is not > >>> available instead of silently falling back to shadow. > >> > >> Would it really be much more involved than the change here to restore > >> silent defaulting to shadow? > > > > But that would mean that a user having selected hap=1 on the config > > file would get silently defaulted to shadow, which is wrong IMO. > > At the libxl layer, aren’t the options tristate? I.e., this would be “hap”, “shadow”, or “not specified”? > > The user needs to be able to specify “always use shadow”, “always use HAP”, or “use HAP if available, otherwise use shadow”. The "use HAP if available, otherwise use shadow" is currently only possibly expressed by not setting the hap option in the config file. > At the moment, leaving it empty should be “use HAP if available, otherwise use shadow”; so “hap = 1” should fail if HAP is not available. Right, this is what this patch is trying to accomplish. I have a v2 series which fills the capabilities field for ARM and also reports the hap capability in `xl info`. Thanks, Roger.
On 9/5/19 12:01 PM, Roger Pau Monné wrote: > On Thu, Sep 05, 2019 at 12:34:11PM +0200, George Dunlap wrote: >> >> >>> On Sep 5, 2019, at 11:01 AM, Roger Pau Monne <roger.pau@citrix.com> wrote: >>> >>> On Thu, Sep 05, 2019 at 11:52:59AM +0200, Jan Beulich wrote: >>>> On 05.09.2019 11:34, Roger Pau Monne wrote: >>>>> Current libxl code will always enable Hardware Assisted Paging (HAP), >>>>> expecting that the hypervisor will fallback to shadow if HAP is not >>>>> available. With the changes to the domain builder that's not the case >>>>> any longer, and the hypervisor will raise an error if HAP is not >>>>> available instead of silently falling back to shadow. >>>> >>>> Would it really be much more involved than the change here to restore >>>> silent defaulting to shadow? >>> >>> But that would mean that a user having selected hap=1 on the config >>> file would get silently defaulted to shadow, which is wrong IMO. >> >> At the libxl layer, aren’t the options tristate? I.e., this would be “hap”, “shadow”, or “not specified”? >> >> The user needs to be able to specify “always use shadow”, “always use HAP”, or “use HAP if available, otherwise use shadow”. > > The "use HAP if available, otherwise use shadow" is currently only > possibly expressed by not setting the hap option in the config file. > >> At the moment, leaving it empty should be “use HAP if available, otherwise use shadow”; so “hap = 1” should fail if HAP is not available. > > Right, this is what this patch is trying to accomplish. Right; I wasn't trying to contradict you so much as "weigh in" (and basically agree with you). -George
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index ec71574e99..5c0fcf320e 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -399,6 +399,7 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo) physinfo->cap_pv = !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_pv); physinfo->cap_hvm_directio = !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_directio); + physinfo->cap_hap = !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_hap); GC_FREE; return 0; diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h index d624159e53..3f59e790b7 100644 --- a/tools/libxl/libxl_arch.h +++ b/tools/libxl/libxl_arch.h @@ -65,8 +65,8 @@ _hidden int libxl__arch_domain_map_irq(libxl__gc *gc, uint32_t domid, int irq); _hidden -void libxl__arch_domain_create_info_setdefault(libxl__gc *gc, - libxl_domain_create_info *c_info); +int libxl__arch_domain_create_info_setdefault(libxl__gc *gc, + libxl_domain_create_info *c_info); _hidden void libxl__arch_domain_build_info_setdefault(libxl__gc *gc, diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c index 141e159043..b067869154 100644 --- a/tools/libxl/libxl_arm.c +++ b/tools/libxl/libxl_arm.c @@ -1114,8 +1114,8 @@ int libxl__arch_domain_map_irq(libxl__gc *gc, uint32_t domid, int irq) return xc_domain_bind_pt_spi_irq(CTX->xch, domid, irq, irq); } -void libxl__arch_domain_create_info_setdefault(libxl__gc *gc, - libxl_domain_create_info *c_info) +int libxl__arch_domain_create_info_setdefault(libxl__gc *gc, + libxl_domain_create_info *c_info) { /* * Arm guest are now considered as PVH by the toolstack. To allow @@ -1130,6 +1130,9 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc, c_info->type = LIBXL_DOMAIN_TYPE_PVH; /* All other fields can remain untouched */ } + libxl_defbool_setdefault(&c_info->hap, true); + + return 0; } void libxl__arch_domain_build_info_setdefault(libxl__gc *gc, diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 03ce166f4f..431ead2067 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -30,17 +30,19 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc, libxl_domain_create_info *c_info) { + int rc; + if (!c_info->type) { LOG(ERROR, "domain type unspecified"); return ERROR_INVAL; } - libxl__arch_domain_create_info_setdefault(gc, c_info); + rc = libxl__arch_domain_create_info_setdefault(gc, c_info); + if (rc) + return rc; - if (c_info->type != LIBXL_DOMAIN_TYPE_PV) { - libxl_defbool_setdefault(&c_info->hap, true); + if (c_info->type != LIBXL_DOMAIN_TYPE_PV) libxl_defbool_setdefault(&c_info->oos, true); - } libxl_defbool_setdefault(&c_info->run_hotplug_scripts, true); libxl_defbool_setdefault(&c_info->driver_domain, false); diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index b61399ce36..9e1f8515d3 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -1025,6 +1025,7 @@ libxl_physinfo = Struct("physinfo", [ ("cap_hvm", bool), ("cap_pv", bool), ("cap_hvm_directio", bool), # No longer HVM specific + ("cap_hap", bool), ], dir=DIR_OUT) libxl_connectorinfo = Struct("connectorinfo", [ diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c index c0f88a7eaa..bdc9d9adfe 100644 --- a/tools/libxl/libxl_x86.c +++ b/tools/libxl/libxl_x86.c @@ -620,9 +620,20 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc *gc, return rc; } -void libxl__arch_domain_create_info_setdefault(libxl__gc *gc, - libxl_domain_create_info *c_info) +int libxl__arch_domain_create_info_setdefault(libxl__gc *gc, + libxl_domain_create_info *c_info) { + libxl_physinfo pi; + int rc = libxl_get_physinfo(CTX, &pi); + + if (rc) { + LOG(ERROR, "unable to get physinfo"); + return rc; + } + + libxl_defbool_setdefault(&c_info->hap, pi.cap_hap); + + return 0; } void libxl__arch_domain_build_info_setdefault(libxl__gc *gc, diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c index c50d910a1c..74ea184087 100644 --- a/xen/arch/x86/sysctl.c +++ b/xen/arch/x86/sysctl.c @@ -165,6 +165,8 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi) pi->capabilities |= XEN_SYSCTL_PHYSCAP_pv; if ( iommu_enabled ) pi->capabilities |= XEN_SYSCTL_PHYSCAP_directio; + if ( hvm_hap_supported() ) + pi->capabilities |= XEN_SYSCTL_PHYSCAP_hap; } long arch_do_sysctl( diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h index 91c48dcae0..6c457625e9 100644 --- a/xen/include/public/sysctl.h +++ b/xen/include/public/sysctl.h @@ -90,6 +90,10 @@ struct xen_sysctl_tbuf_op { /* (x86) The platform supports direct access to I/O devices with IOMMU. */ #define _XEN_SYSCTL_PHYSCAP_directio 2 #define XEN_SYSCTL_PHYSCAP_directio (1u<<_XEN_SYSCTL_PHYSCAP_directio) +/* (x86) The platform supports Hardware Assisted Paging. */ +#define _XEN_SYSCTL_PHYSCAP_hap 3 +#define XEN_SYSCTL_PHYSCAP_hap (1u<<_XEN_SYSCTL_PHYSCAP_hap) + struct xen_sysctl_physinfo { uint32_t threads_per_core; uint32_t cores_per_socket;
Current libxl code will always enable Hardware Assisted Paging (HAP), expecting that the hypervisor will fallback to shadow if HAP is not available. With the changes to the domain builder that's not the case any longer, and the hypervisor will raise an error if HAP is not available instead of silently falling back to shadow. In order to keep the previous functionality report whether HAP is available or not in XEN_SYSCTL_physinfo, so that the toolstack can select a sane default if there's no explicit user selection of whether HAP should be used. Fixes: d0c0ba7d3de ('x86/hvm/domain: remove the 'hap_enabled' flag') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Paul Durrant <Paul.Durrant@citrix.com> --- tools/libxl/libxl.c | 1 + tools/libxl/libxl_arch.h | 4 ++-- tools/libxl/libxl_arm.c | 7 +++++-- tools/libxl/libxl_create.c | 10 ++++++---- tools/libxl/libxl_types.idl | 1 + tools/libxl/libxl_x86.c | 15 +++++++++++++-- xen/arch/x86/sysctl.c | 2 ++ xen/include/public/sysctl.h | 4 ++++ 8 files changed, 34 insertions(+), 10 deletions(-)