Re: [PATCH v3 06/24] xen/domain: introduce hardware emulation flags

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.01.2025 02:58, Denis Mukhin via B4 Relay wrote:
> @@ -8,7 +9,9 @@ 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);
> +        config->arch.emulation_flags = XEN_X86_EMU_ALL;
> +        config->arch.emulation_flags &= ~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;

You're merely writing the same thing differently here, aren't you? Why
is this needed?

> @@ -159,9 +160,7 @@ 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 |
> -                                        XEN_X86_EMU_USE_PIRQ);
> +        config.arch.emulation_flags = XEN_X86_EMU_BASELINE;

While less direct here, same question as above.

> --- a/tools/tests/paging-mempool/test-paging-mempool.c
> +++ b/tools/tests/paging-mempool/test-paging-mempool.c
> @@ -9,6 +9,7 @@
>  #include <xenforeignmemory.h>
>  #include <xengnttab.h>
>  #include <xen-tools/common-macros.h>
> +#include <xen/virtdev.h>
>  
>  static unsigned int nr_failures;
>  #define fail(fmt, ...)                          \
> --- a/tools/tests/resource/test-resource.c
> +++ b/tools/tests/resource/test-resource.c
> @@ -8,6 +8,7 @@
>  #include <xenforeignmemory.h>
>  #include <xengnttab.h>
>  #include <xen-tools/common-macros.h>
> +#include <xen/virtdev.h>
>  
>  static unsigned int nr_failures;
>  #define fail(fmt, ...)                          \
> --- a/tools/tests/tsx/test-tsx.c
> +++ b/tools/tests/tsx/test-tsx.c
> @@ -29,6 +29,7 @@
>  #include <xenctrl.h>
>  #include <xenguest.h>
>  #include <xen-tools/common-macros.h>
> +#include <xen/virtdev.h>
>  
>  #include "xg_private.h"
>  

Throughout these in particular - it's not really nice to require the extra
#include everywhere now.

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -753,9 +753,7 @@ static bool emulation_flags_ok(const struct domain *d, 
> uint32_t emflags)
>               emflags != (X86_EMU_VPCI | X86_EMU_LAPIC | X86_EMU_IOAPIC) )
>              return false;
>          if ( !is_hardware_domain(d) &&
> -             /* HVM PIRQ feature is user-selectable. */
> -             (emflags & ~X86_EMU_USE_PIRQ) !=
> -             (X86_EMU_ALL & ~(X86_EMU_VPCI | X86_EMU_USE_PIRQ)) &&
> +             xen_emflags_allowable(emflags) != XEN_X86_EMU_BASELINE &&

What is or is not allowable doesn't depend on just the flags. Either the
name needs to be more specific, or the domain needs passing in.

> @@ -456,7 +457,7 @@ struct arch_domain
>      /* Don't unconditionally inject #GP for unhandled MSRs. */
>      bool msr_relaxed;
>  
> -    /* Emulated devices enabled bitmap. */
> +    /* Hardware emulation flags. */
>      uint32_t emulation_flags;
>  } __cacheline_aligned;

The original comment isn't good enough because of what?

> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -15,6 +15,7 @@ headers-y := \
>      compat/sched.h \
>      compat/vcpu.h \
>      compat/version.h \
> +    compat/virtdev.h \
>      compat/xen.h \
>      compat/xlat.h

This shouldn't be needed, as ...

> --- /dev/null
> +++ b/xen/include/public/virtdev.h
> @@ -0,0 +1,61 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef XEN__PUBLIC_VIRTDEV_H
> +#define XEN__PUBLIC_VIRTDEV_H

... this should in no case be generally exposed: You moved the flags
from a tools-only section of arch-x86/xen.h, and hence they should
remain tools-only.

> +/*
> + * Domain hardware emulation flags.
> + */
> +enum {
> +    VIRTDEV_LAPIC      = 1U << 0,
> +    VIRTDEV_HPET       = 1U << 1,
> +    VIRTDEV_PM         = 1U << 2,
> +    VIRTDEV_RTC        = 1U << 3,
> +    VIRTDEV_IOAPIC     = 1U << 4,
> +    VIRTDEV_PIC        = 1U << 5,
> +    VIRTDEV_VGA        = 1U << 6,
> +    VIRTDEV_IOMMU      = 1U << 7,
> +    VIRTDEV_PIT        = 1U << 8,
> +    VIRTDEV_PIRQ       = 1U << 9,
> +    VIRTDEV_PCI        = 1U << 10,
> +};
> +
> +#if defined(__i386__) || defined(__x86_64__)

Why does this conditional live only here? Almost the entire enum above
is x86-specific, too.

Bottom line: I remain yet to be convinced of the need for the new header.

Jan