Re: [PATCH v4 2/2] tools: don't expose XENFEAT_hvm_pirqs by default

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Jan 25, 2024 at 10:30:41AM +0100, Roger Pau Monne wrote:
> The HVM pirq feature allows routing interrupts from both physical and emulated
> devices over event channels, this was done a performance improvement.  However
> its usage is fully undocumented, and the only reference implementation is in
> Linux.  It defeats the purpose of local APIC hardware virtualization, because
> when using it interrupts avoid the usage of the local APIC altogether.
> 
> It has also been reported to not work properly with certain devices, at least
> when using some AMD GPUs Linux attempts to route interrupts over event
> channels, but Xen doesn't correctly detect such routing, which leads to the
> hypervisor complaining with:
> 
> (XEN) d15v0: Unsupported MSI delivery mode 7 for Dom15
> 
> When MSIs are attempted to be routed over event channels the entry delivery
> mode is set to ExtINT, but Xen doesn't detect such routing and attempts to
> inject the interrupt following the native MSI path, and the ExtINT delivery
> mode is not supported.
> 
> Disable HVM PIRQs by default and provide a per-domain option in xl.cfg to
> enable such feature.  Also for backwards compatibility keep the feature 
> enabled
> for any resumed domains that don't have an explicit selection.
> 
> Note that the only user of the feature (Linux) is also able to handle native
> interrupts fine, as the feature was already not used if Xen reported local 
> APIC
> hardware virtualization active.
> 
> Link: https://github.com/QubesOS/qubes-issues/issues/7971
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Reviewed-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

For the python part - it's a bit unfortunate there is no knob to control
the value, but in practice I doubt it would be useful (and especially
for python bindings users), so:

Acked-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>

> ---
> Changes since v2:
>  - Add changelog entry.
> 
> Changes since v1:
>  - Fix libxl for PV guests.
> ---
>  CHANGELOG.md                      |  2 ++
>  docs/man/xl.cfg.5.pod.in          |  7 +++++++
>  tools/include/libxl.h             |  7 +++++++
>  tools/libs/light/libxl_create.c   |  7 +++++--
>  tools/libs/light/libxl_types.idl  |  1 +
>  tools/libs/light/libxl_x86.c      | 12 +++++++++---
>  tools/python/xen/lowlevel/xc/xc.c |  4 +++-
>  tools/xl/xl_parse.c               |  1 +
>  8 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index 723d06425431..ddb3ab8db4e7 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -9,6 +9,8 @@ The format is based on [Keep a 
> Changelog](https://keepachangelog.com/en/1.0.0/)
>  ### Changed
>   - Changed flexible array definitions in public I/O interface headers to not
>     use "1" as the number of array elements.
> + - On x86:
> +   - HVM PIRQs are disabled by default.
>  
>  ### Added
>   - On x86:
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 2e234b450efb..ea8d41727d8e 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -2460,6 +2460,13 @@ The viridian option can be specified as a boolean. A 
> value of true (1)
>  is equivalent to the list [ "defaults" ], and a value of false (0) is
>  equivalent to an empty list.
>  
> +=item B<hvm_pirq=BOOLEAN>
> +
> +Select whether the guest is allowed to route interrupts from devices (either
> +emulated or passed through) over event channels.
> +
> +This option is disabled by default.
> +
>  =back
>  
>  =head3 Emulated VGA Graphics Device
> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> index 907aa0a3303a..f1652b1664f0 100644
> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
> @@ -608,6 +608,13 @@
>   * executable in order to not run it as the same user as libxl.
>   */
>  
> +/*
> + * LIBXL_HAVE_HVM_PIRQ indicates the presence of the u.hvm.pirq filed in
> + * libxl_domain_build_info that signals whether an HVM guest has accesses to
> + * the XENFEAT_hvm_pirqs feature.
> + */
> +#define LIBXL_HAVE_HVM_PIRQ 1
> +
>  /*
>   * libxl memory management
>   *
> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> index ce1d43110336..0008fac607e3 100644
> --- a/tools/libs/light/libxl_create.c
> +++ b/tools/libs/light/libxl_create.c
> @@ -376,6 +376,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>          libxl_defbool_setdefault(&b_info->u.hvm.usb,                false);
>          libxl_defbool_setdefault(&b_info->u.hvm.vkb_device,         true);
>          libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci,   true);
> +        libxl_defbool_setdefault(&b_info->u.hvm.pirq,               false);
>  
>          libxl_defbool_setdefault(&b_info->u.hvm.spice.enable, false);
>          if (!libxl_defbool_val(b_info->u.hvm.spice.enable) &&
> @@ -2375,10 +2376,12 @@ int libxl_domain_create_restore(libxl_ctx *ctx, 
> libxl_domain_config *d_config,
>  
>      /*
>       * When restoring (either from a save file or for a migration domain) set
> -     * the MSR relaxed mode for compatibility with older Xen versions if the
> -     * option is not set as part of the original configuration.
> +     * the MSR relaxed mode and HVM PIRQs for compatibility with older Xen
> +     * versions if the options are not set as part of the original
> +     * configuration.
>       */
>      libxl_defbool_setdefault(&d_config->b_info.arch_x86.msr_relaxed, true);
> +    libxl_defbool_setdefault(&d_config->b_info.u.hvm.pirq, true);
>  
>      return do_domain_create(ctx, d_config, domid, restore_fd, send_back_fd,
>                              params, ao_how, aop_console_how);
> diff --git a/tools/libs/light/libxl_types.idl 
> b/tools/libs/light/libxl_types.idl
> index 7d8bd5d21667..899ad3096926 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -692,6 +692,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>                                         ("rdm", libxl_rdm_reserve),
>                                         ("rdm_mem_boundary_memkb", MemKB),
>                                         ("mca_caps",         uint64),
> +                                       ("pirq",             libxl_defbool),
>                                         ])),
>                   ("pv", Struct(None, [("kernel", string, {'deprecated_by': 
> 'kernel'}),
>                                        ("slack_memkb", MemKB),
> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
> index d16573e72cd4..a50ec37eb3eb 100644
> --- a/tools/libs/light/libxl_x86.c
> +++ b/tools/libs/light/libxl_x86.c
> @@ -9,6 +9,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>      switch(d_config->c_info.type) {
>      case LIBXL_DOMAIN_TYPE_HVM:
>          config->arch.emulation_flags = (XEN_X86_EMU_ALL & ~XEN_X86_EMU_VPCI);
> +        if (!libxl_defbool_val(d_config->b_info.u.hvm.pirq))
> +            config->arch.emulation_flags &= ~XEN_X86_EMU_USE_PIRQ;
>          break;
>      case LIBXL_DOMAIN_TYPE_PVH:
>          config->arch.emulation_flags = XEN_X86_EMU_LAPIC;
> @@ -864,15 +866,19 @@ void libxl__arch_update_domain_config(libxl__gc *gc,
>                                        const libxl_domain_config *src)
>  {
>      /*
> -     * Force MSR relaxed to be set (either to true or false) so it's part of
> -     * the domain configuration when saving or performing a live-migration.
> +     * Force MSR relaxed and HVM pirq to be set (either to true or false) so
> +     * it's part of the domain configuration when saving or performing a
> +     * live-migration.
>       *
> -     * Doing so allows the recovery side to figure out whether the flag 
> should
> +     * Doing so allows the recovery side to figure out whether the flags 
> should
>       * be set to true in order to keep backwards compatibility with already
>       * started domains.
>       */
>      libxl_defbool_setdefault(&dst->b_info.arch_x86.msr_relaxed,
>                      libxl_defbool_val(src->b_info.arch_x86.msr_relaxed));
> +    if (src->c_info.type == LIBXL_DOMAIN_TYPE_HVM )
> +        libxl_defbool_setdefault(&dst->b_info.u.hvm.pirq,
> +                                 libxl_defbool_val(src->b_info.u.hvm.pirq));
>  }
>  
>  /*
> diff --git a/tools/python/xen/lowlevel/xc/xc.c 
> b/tools/python/xen/lowlevel/xc/xc.c
> index d3ea350e07b9..9feb12ae2b16 100644
> --- a/tools/python/xen/lowlevel/xc/xc.c
> +++ b/tools/python/xen/lowlevel/xc/xc.c
> @@ -159,7 +159,9 @@ static PyObject *pyxc_domain_create(XcObject *self,
>  
>  #if defined (__i386) || defined(__x86_64__)
>      if ( config.flags & XEN_DOMCTL_CDF_hvm )
> -        config.arch.emulation_flags = (XEN_X86_EMU_ALL & ~XEN_X86_EMU_VPCI);
> +        config.arch.emulation_flags = XEN_X86_EMU_ALL &
> +                                      ~(XEN_X86_EMU_VPCI |
> +                                        XEN_X86_EMU_USE_PIRQ);
>  #elif defined (__arm__) || defined(__aarch64__)
>      config.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
>  #else
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index ed983200c3f8..9b358f11b88e 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1801,6 +1801,7 @@ void parse_config_data(const char *config_source,
>          xlu_cfg_get_defbool(config, "hpet", &b_info->u.hvm.hpet, 0);
>          xlu_cfg_get_defbool(config, "vpt_align", &b_info->u.hvm.vpt_align, 
> 0);
>          xlu_cfg_get_defbool(config, "apic", &b_info->apic, 0);
> +        xlu_cfg_get_defbool(config, "hvm_pirq", &b_info->u.hvm.pirq, 0);
>  
>          switch (xlu_cfg_get_list(config, "viridian",
>                                   &viridian, &num_viridian, 1))
> -- 
> 2.43.0
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature