Re: [Xen-devel] [PATCH v2 2/2] sysctl/libxl: choose a sane default for HAP

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Sep 06, 2019 at 03:54:10PM +0200, Paul Durrant wrote:
> > -----Original Message-----
> > From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Paul 
> > Durrant
> > Sent: 05 September 2019 14:52
> > To: Roger Pau Monne <roger.pau@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; 
> > Konrad Rzeszutek Wilk
> > <konrad.wilk@xxxxxxxxxx>; Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Tim 
> > (Xen.org) <tim@xxxxxxx>;
> > George Dunlap <George.Dunlap@xxxxxxxxxx>; Julien Grall 
> > <julien.grall@xxxxxxx>; Jan Beulich
> > <jbeulich@xxxxxxxx>; Anthony Perard <anthony.perard@xxxxxxxxxx>; Ian 
> > Jackson <Ian.Jackson@xxxxxxxxxx>;
> > Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>; Roger Pau Monne 
> > <roger.pau@xxxxxxxxxx>
> > Subject: Re: [Xen-devel] [PATCH v2 2/2] sysctl/libxl: choose a sane default 
> > for HAP
> > 
> > > -----Original Message-----
> > > From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> > > Sent: 05 September 2019 14:27
> > > To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> > > Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx>; Ian Jackson 
> > > <Ian.Jackson@xxxxxxxxxx>; Wei Liu
> > > <wl@xxxxxxx>; Anthony Perard <anthony.perard@xxxxxxxxxx>; Andrew Cooper 
> > > <Andrew.Cooper3@xxxxxxxxxx>;
> > > George Dunlap <George.Dunlap@xxxxxxxxxx>; Jan Beulich 
> > > <jbeulich@xxxxxxxx>; Julien Grall
> > > <julien.grall@xxxxxxx>; Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; 
> > > Stefano Stabellini
> > > <sstabellini@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>; Volodymyr Babchuk
> > <Volodymyr_Babchuk@xxxxxxxx>;
> > > Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> > > Subject: [PATCH v2 2/2] sysctl/libxl: choose a sane default for HAP
> > >
> > > 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.
> > >
> > > Note that on ARM hardware HAP capability is always reported since it's
> > > a required feature in order to run Xen.
> > >
> > > Fixes: d0c0ba7d3de ('x86/hvm/domain: remove the 'hap_enabled' flag')
> > > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > 
> > LGTM
> > 
> > Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > 
> > > ---
> > > Cc: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> > > ---
> > > Changes since v1:
> > >  - Also report HAP capability for ARM.
> > >  - Print hap capability in xl info.
> > > ---
> > >  tools/libxl/libxl.c         | 1 +
> > >  tools/libxl/libxl_create.c  | 8 +++++++-
> > >  tools/libxl/libxl_types.idl | 1 +
> > >  tools/xl/xl_info.c          | 5 +++--
> > >  xen/arch/arm/sysctl.c       | 2 +-
> > >  xen/arch/x86/sysctl.c       | 2 ++
> > >  xen/include/public/sysctl.h | 4 ++++
> > >  7 files changed, 19 insertions(+), 4 deletions(-)
> > >
> > > 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_create.c b/tools/libxl/libxl_create.c
> > > index 03ce166f4f..6a556dea8f 100644
> > > --- a/tools/libxl/libxl_create.c
> > > +++ b/tools/libxl/libxl_create.c
> > > @@ -38,7 +38,13 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
> > >      libxl__arch_domain_create_info_setdefault(gc, c_info);
> > >
> > >      if (c_info->type != LIBXL_DOMAIN_TYPE_PV) {
> > > -        libxl_defbool_setdefault(&c_info->hap, true);
> > > +        libxl_physinfo info;
> > > +        int rc = libxl_get_physinfo(CTX, &info);
> > > +
> > > +        if (rc)
> > > +            return rc;
> > > +
> > > +        libxl_defbool_setdefault(&c_info->hap, info.cap_hap);
> > >          libxl_defbool_setdefault(&c_info->oos, true);
> > >      }
> > >
> > > 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),
> 
> Actually Julien's review of one of my patches points out that this idl change 
> should be accompanied by an associated LIBXL_HAVE_CAP_HAP definition.

Ouch, yes, I always forget those. I will add now, and keep your RB and
Jan's Ack unless any of you tell me otherwise.

Thanks, Roger.

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