Re: [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC

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

On Thu, Jan 27, 2022 at 04:01:33PM +0000, Jane Malalane wrote:
> Introduce a new per-domain creation x86 specific flag to
> select whether hardware assisted virtualization should be used for
> x{2}APIC.
> 
> A per-domain option is added to xl in order to select the usage of
> x{2}APIC hardware assisted vitualization, as well as a global
> configuration option.
> 
> Having all APIC interaction exit to Xen for emulation is slow and can
> induce much overhead. Hardware can speed up x{2}APIC by running APIC
> read/write accesses without taking a VM exit.
> 
> Signed-off-by: Jane Malalane <jane.malalane@xxxxxxxxxx>
> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Wei Liu <wl@xxxxxxx>
> CC: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> CC: Juergen Gross <jgross@xxxxxxxx>
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: George Dunlap <george.dunlap@xxxxxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Christian Lindig <christian.lindig@xxxxxxxxxx>
> CC: David Scott <dave@xxxxxxxxxx>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> CC: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> ---
>  docs/man/xl.cfg.5.pod.in              | 10 ++++++++
>  docs/man/xl.conf.5.pod.in             | 12 ++++++++++
>  tools/golang/xenlight/helpers.gen.go  | 12 ++++++++++
>  tools/libs/light/libxl_arch.h         |  5 ++--
>  tools/libs/light/libxl_arm.c          |  5 ++--
>  tools/libs/light/libxl_create.c       | 21 ++++++++++-------
>  tools/libs/light/libxl_types.idl      |  2 ++
>  tools/libs/light/libxl_x86.c          | 43 
> +++++++++++++++++++++++++++++++++--
>  tools/ocaml/libs/xc/xenctrl.ml        |  2 ++
>  tools/ocaml/libs/xc/xenctrl.mli       |  2 ++
>  tools/xl/xl.c                         |  8 +++++++
>  tools/xl/xl.h                         |  2 ++
>  tools/xl/xl_parse.c                   | 14 ++++++++++++
>  xen/arch/x86/domain.c                 | 27 +++++++++++++++++++++-
>  xen/arch/x86/hvm/vmx/vmcs.c           |  4 ++++
>  xen/arch/x86/hvm/vmx/vmx.c            | 13 +++++++----
>  xen/arch/x86/include/asm/hvm/domain.h |  6 +++++
>  xen/arch/x86/traps.c                  |  6 +++--
>  xen/include/public/arch-x86/xen.h     |  2 ++
>  19 files changed, 174 insertions(+), 22 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index b98d161398..974fe7d2d8 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -1862,6 +1862,16 @@ firmware tables when using certain older guest 
> Operating
>  Systems. These tables have been superseded by newer constructs within
>  the ACPI tables.
>  
> +=item B<assisted_xapic=BOOLEAN>
> +B<(x86 only)> Enables or disables hardware assisted virtualization for xapic.
> +This allows accessing APIC registers without a VM-exit.
> +The default is settable via L<xl.conf(5)>.
> +
> +=item B<assisted_x2apic=BOOLEAN>
> +B<(x86 only)> Enables or disables hardware assisted virtualization for 
> x2apic.
> +This allows accessing APIC registers without a VM-exit.
> +The default is settable via L<xl.conf(5)>.

Like you do below I would capitalize xAPIC and x2APIC in the option
text.

> +
>  =item B<nx=BOOLEAN>
>  
>  B<(x86 only)> Hides or exposes the No-eXecute capability. This allows a guest
> diff --git a/docs/man/xl.conf.5.pod.in b/docs/man/xl.conf.5.pod.in
> index df20c08137..2d0a59d019 100644
> --- a/docs/man/xl.conf.5.pod.in
> +++ b/docs/man/xl.conf.5.pod.in
> @@ -107,6 +107,18 @@ Sets the default value for the C<max_grant_version> 
> domain config value.
>  
>  Default: maximum grant version supported by the hypervisor.
>  
> +=item B<assisted_xapic=BOOLEAN>
> +
> +If enabled, domains will use xAPIC hardware assisted emulation by default.
> +
> +Default: enabled.
> +
> +=item B<assisted_x2apic=BOOLEAN>
> +
> +If enabled, domains will use x2APIC hardware assisted emulation by default.
> +
> +Default: enabled.

I think for both options this should be:

Default: enabled if supported.

> +
>  =item B<vif.default.script="PATH">
>  
>  Configures the default hotplug script used by virtual network devices.
> diff --git a/tools/golang/xenlight/helpers.gen.go 
> b/tools/golang/xenlight/helpers.gen.go
> index dd4e6c9f14..90e7b9b205 100644
> --- a/tools/golang/xenlight/helpers.gen.go
> +++ b/tools/golang/xenlight/helpers.gen.go
> @@ -636,6 +636,12 @@ x.Passthrough = Passthrough(xc.passthrough)
>  if err := 
> x.XendSuspendEvtchnCompat.fromC(&xc.xend_suspend_evtchn_compat);err != nil {
>  return fmt.Errorf("converting field XendSuspendEvtchnCompat: %v", err)
>  }
> +if err := x.ArchX86.AssistedXapic.fromC(&xc.arch_x86.assisted_xapic);err != 
> nil {
> +return fmt.Errorf("converting field ArchX86.AssistedXapic: %v", err)
> +}
> +if err := x.ArchX86.AssistedX2Apic.fromC(&xc.arch_x86.assisted_x2apic);err 
> != nil {
> +return fmt.Errorf("converting field ArchX86.AssistedX2Apic: %v", err)
> +}
>  
>   return nil}
>  
> @@ -679,6 +685,12 @@ xc.passthrough = C.libxl_passthrough(x.Passthrough)
>  if err := x.XendSuspendEvtchnCompat.toC(&xc.xend_suspend_evtchn_compat); err 
> != nil {
>  return fmt.Errorf("converting field XendSuspendEvtchnCompat: %v", err)
>  }
> +if err := x.ArchX86.AssistedXapic.toC(&xc.arch_x86.assisted_xapic); err != 
> nil {
> +return fmt.Errorf("converting field ArchX86.AssistedXapic: %v", err)
> +}
> +if err := x.ArchX86.AssistedX2Apic.toC(&xc.arch_x86.assisted_x2apic); err != 
> nil {
> +return fmt.Errorf("converting field ArchX86.AssistedX2Apic: %v", err)
> +}
>  
>   return nil
>   }
> diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h
> index 00cc50394d..2eaff45526 100644
> --- a/tools/libs/light/libxl_arch.h
> +++ b/tools/libs/light/libxl_arch.h
> @@ -71,8 +71,9 @@ void 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,
> -                                              libxl_domain_build_info 
> *b_info);
> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> +                                             libxl_domain_build_info *b_info,
> +                                             const libxl_physinfo *physinfo);
>  
>  _hidden
>  int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 52f2545498..4d422bef96 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -1384,8 +1384,9 @@ void 
> libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
>      }
>  }
>  
> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> -                                              libxl_domain_build_info 
> *b_info)
> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> +                                             libxl_domain_build_info *b_info,
> +                                             const libxl_physinfo *physinfo)
>  {
>      /* ACPI is disabled by default */
>      libxl_defbool_setdefault(&b_info->acpi, false);
> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> index d7a40d7550..2bae6fef62 100644
> --- a/tools/libs/light/libxl_create.c
> +++ b/tools/libs/light/libxl_create.c
> @@ -264,7 +264,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>      if (!b_info->event_channels)
>          b_info->event_channels = 1023;
>  
> -    libxl__arch_domain_build_info_setdefault(gc, b_info);
>      libxl_defbool_setdefault(&b_info->dm_restrict, false);
>  
>      if (b_info->iommu_memkb == LIBXL_MEMKB_DEFAULT)
> @@ -456,15 +455,21 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>          libxl_defbool_setdefault(&b_info->nested_hvm,               false);
>      }
>  
> -    if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
> -        libxl_physinfo info;
> +    libxl_physinfo info;
>  
> -        rc = libxl_get_physinfo(CTX, &info);
> -        if (rc) {
> -            LOG(ERROR, "failed to get hypervisor info");
> -            return rc;
> -        }
> +    rc = libxl_get_physinfo(CTX, &info);
> +    if (rc) {
> +        LOG(ERROR, "failed to get hypervisor info");
> +        return rc;
> +    }
>  
> +    rc = libxl__arch_domain_build_info_setdefault(gc, b_info, &info);
> +    if (rc) {
> +        LOG(ERROR, "unable to set domain arch build info defaults");
> +        return rc;
> +    }
> +
> +    if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
>          if (info.cap_gnttab_v2)
>              b_info->max_grant_version = 2;
>          else if (info.cap_gnttab_v1)
> diff --git a/tools/libs/light/libxl_types.idl 
> b/tools/libs/light/libxl_types.idl
> index 42ac6c357b..db5eb0a0b3 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -648,6 +648,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
>                                 ("vuart", libxl_vuart_type),
>                                ])),
>      ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
> +                               ("assisted_xapic", libxl_defbool),
> +                               ("assisted_x2apic", libxl_defbool),
>                                ])),
>      # Alternate p2m is not bound to any architecture or guest type, as it is
>      # supported by x86 HVM and ARM support is planned.
> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
> index 33da51fe89..b257fca756 100644
> --- a/tools/libs/light/libxl_x86.c
> +++ b/tools/libs/light/libxl_x86.c
> @@ -23,6 +23,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>      if (libxl_defbool_val(d_config->b_info.arch_x86.msr_relaxed))
>          config->arch.misc_flags |= XEN_X86_MSR_RELAXED;
>  
> +    if(libxl_defbool_val(d_config->b_info.arch_x86.assisted_xapic))
> +        config->arch.misc_flags |= XEN_X86_ASSISTED_XAPIC;
> +
> +    if(libxl_defbool_val(d_config->b_info.arch_x86.assisted_x2apic))
> +        config->arch.misc_flags |= XEN_X86_ASSISTED_X2APIC;
> +
>      return 0;
>  }
>  
> @@ -819,11 +825,44 @@ void 
> libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
>  {
>  }
>  
> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> -                                              libxl_domain_build_info 
> *b_info)
> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> +                                             libxl_domain_build_info *b_info,
> +                                             const libxl_physinfo *physinfo)
>  {
> +    int rc;
> +    bool assisted_xapic;
> +    bool assisted_x2apic;
> +
>      libxl_defbool_setdefault(&b_info->acpi, true);
>      libxl_defbool_setdefault(&b_info->arch_x86.msr_relaxed, false);
> +
> +    libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, false);
> +    libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, false);
> +
> +    assisted_xapic = libxl_defbool_val(b_info->arch_x86.assisted_xapic);
> +    assisted_x2apic = libxl_defbool_val(b_info->arch_x86.assisted_x2apic);
> +
> +    if ((assisted_xapic || assisted_x2apic) &&
> +        b_info->type == LIBXL_DOMAIN_TYPE_PV)
> +    {
> +        LOG(ERROR, "Interrupt Controller Virtualization not supported for 
> PV");
> +        rc = ERROR_INVAL;
> +        goto out;
> +    }
> +
> +    if ((assisted_xapic && !physinfo->cap_assisted_xapic) ||
> +         (assisted_x2apic && !physinfo->cap_assisted_x2apic))
> +    {
> +        LOG(ERROR, "x%sAPIC hardware supported emulation not available",
> +            assisted_xapic && !physinfo->cap_assisted_xapic ? "" : "2");
> +        rc =  ERROR_INVAL;
> +        goto out;
> +    }

I think the logic here is slightly wrong, as you are setting the
default value of assisted_x{2}apic to false, and we would instead like
to set it to the current value supported by the hardware in order to
keep current behavior.

Also the options are HVM/PVH only, so having them set for PV should
result in an error regardless of the set value, ie:

if (b_info->type == LIBXL_DOMAIN_TYPE_PV &&
    (!libxl_defbool_is_default(&b_info->arch_x86.assisted_xapic) ||
     !libxl_defbool_is_default(&b_info->arch_x86.assisted_x2apic)))
     ERROR

libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic,
                         physinfo->cap_assisted_xapic);
libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic,
                         physinfo->cap_assisted_x2apic);

I don't think you need the local assisted_x{2}apic variables.

> +
> +    rc = 0;
> +out:
> +    return rc;

The out label is not really needed here and makes the code longer.
Just 'return ERROR_INVAL' in the error paths or 0 at the end of the
function. You can then also drop the local rc variable.

> +
>  }
>  
>  int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
> index 7ce832d605..cce30d8731 100644
> --- a/tools/ocaml/libs/xc/xenctrl.ml
> +++ b/tools/ocaml/libs/xc/xenctrl.ml
> @@ -50,6 +50,8 @@ type x86_arch_emulation_flags =
>  
>  type x86_arch_misc_flags =
>       | X86_MSR_RELAXED
> +     | X86_ASSISTED_XAPIC
> +     | X86_ASSISTED_X2APIC
>  
>  type xen_x86_arch_domainconfig =
>  {
> diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
> index a2b15130ee..67a22ec15c 100644
> --- a/tools/ocaml/libs/xc/xenctrl.mli
> +++ b/tools/ocaml/libs/xc/xenctrl.mli
> @@ -44,6 +44,8 @@ type x86_arch_emulation_flags =
>  
>  type x86_arch_misc_flags =
>    | X86_MSR_RELAXED
> +  | X86_ASSISTED_XAPIC
> +  | X86_ASSISTED_X2APIC
>  
>  type xen_x86_arch_domainconfig = {
>    emulation_flags: x86_arch_emulation_flags list;
> diff --git a/tools/xl/xl.c b/tools/xl/xl.c
> index 2d1ec18ea3..b97e491c9c 100644
> --- a/tools/xl/xl.c
> +++ b/tools/xl/xl.c
> @@ -57,6 +57,8 @@ int max_grant_frames = -1;
>  int max_maptrack_frames = -1;
>  int max_grant_version = LIBXL_MAX_GRANT_DEFAULT;
>  libxl_domid domid_policy = INVALID_DOMID;
> +int assisted_xapic = 0;
> +int assisted_x2apic = 0;

This should be initialized to -1, in order to denote the values are
unset...

>  
>  xentoollog_level minmsglevel = minmsglevel_default;
>  
> @@ -201,6 +203,12 @@ static void parse_global_config(const char *configfile,
>      if (!xlu_cfg_get_long (config, "claim_mode", &l, 0))
>          claim_mode = l;
>  
> +    if (!xlu_cfg_get_long (config, "assisted_xapic", &l, 0))
> +        assisted_xapic = l;
> +
> +    if (!xlu_cfg_get_long (config, "assisted_x2apic", &l, 0))
> +        assisted_x2apic = l;
> +
>      xlu_cfg_replace_string (config, "remus.default.netbufscript",
>          &default_remus_netbufscript, 0);
>      xlu_cfg_replace_string (config, "colo.default.proxyscript",
> diff --git a/tools/xl/xl.h b/tools/xl/xl.h
> index c5c4bedbdd..528deb3feb 100644
> --- a/tools/xl/xl.h
> +++ b/tools/xl/xl.h
> @@ -286,6 +286,8 @@ extern libxl_bitmap global_vm_affinity_mask;
>  extern libxl_bitmap global_hvm_affinity_mask;
>  extern libxl_bitmap global_pv_affinity_mask;
>  extern libxl_domid domid_policy;
> +extern int assisted_xapic;
> +extern int assisted_x2apic;
>  
>  enum output_format {
>      OUTPUT_FORMAT_JSON,
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 117fcdcb2b..16ff9e76bc 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1681,6 +1681,20 @@ void parse_config_data(const char *config_source,
>          xlu_cfg_get_defbool(config, "vpt_align", &b_info->u.hvm.vpt_align, 
> 0);
>          xlu_cfg_get_defbool(config, "apic", &b_info->apic, 0);
>  
> +        e = xlu_cfg_get_defbool(config, "assisted_xapic",
> +                                &b_info->arch_x86.assisted_xapic, 0);
> +        if (e == ESRCH) /* not specified */
> +            libxl_defbool_set(&b_info->arch_x86.assisted_xapic, 
> assisted_xapic);

...because here you only want to use the global values if they have
actually been set by the user (assisted_x{2}apic != -1):

e = xlu_cfg_get_defbool(config, "assisted_xapic",
                        &b_info->arch_x86.assisted_xapic, 0);
if (e == ESRCH && assisted_xapic != -1) /* use global default if present */
    libxl_defbool_set(&b_info->arch_x86.assisted_xapic, assisted_xapic);
else if (e)
    exit(1);

> +        else if (e)
> +            exit(1);
> +
> +        e = xlu_cfg_get_defbool(config, "assisted_x2apic",
> +                                &b_info->arch_x86.assisted_x2apic, 0);
> +        if (e == ESRCH) /* not specified */
> +            libxl_defbool_set(&b_info->arch_x86.assisted_x2apic, 
> assisted_x2apic);
> +        else if (e)
> +            exit(1);
> +
>          switch (xlu_cfg_get_list(config, "viridian",
>                                   &viridian, &num_viridian, 1))
>          {
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index ef1812dc14..d08f51e28b 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -619,6 +619,8 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>      bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
>      bool hap = config->flags & XEN_DOMCTL_CDF_hap;
>      bool nested_virt = config->flags & XEN_DOMCTL_CDF_nested_virt;
> +    bool assisted_xapic = config->arch.misc_flags & XEN_X86_ASSISTED_XAPIC;
> +    bool assisted_x2apic = config->arch.misc_flags & XEN_X86_ASSISTED_X2APIC;
>      unsigned int max_vcpus;
>  
>      if ( hvm ? !hvm_enabled : !IS_ENABLED(CONFIG_PV) )
> @@ -685,13 +687,30 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>          }
>      }
>  
> -    if ( config->arch.misc_flags & ~XEN_X86_MSR_RELAXED )
> +    if ( config->arch.misc_flags & ~(XEN_X86_MSR_RELAXED |
> +                                     XEN_X86_ASSISTED_XAPIC |
> +                                     XEN_X86_ASSISTED_X2APIC) )
>      {
>          dprintk(XENLOG_INFO, "Invalid arch misc flags %#x\n",
>                  config->arch.misc_flags);
>          return -EINVAL;
>      }
>  
> +    if ( (assisted_xapic || assisted_x2apic) && !hvm )
> +    {
> +        dprintk(XENLOG_INFO,
> +                "Interrupt Controller Virtualization not supported for 
> PV\n");
> +        return -EINVAL;
> +    }
> +
> +    if ( (assisted_xapic && !assisted_xapic_available) ||
> +         (assisted_x2apic && !assisted_x2apic_available) )
> +    {
> +        dprintk(XENLOG_INFO, "x%sAPIC requested but not available\n",

This should be a little bit more concise, as Xen does always offer
a fully software virtualized x{2}APIC.

"hardware assisted x%sAPIC requested but not available\n"

Thanks, Roger.