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

[Jane Malalane <Jane.Malalane@xxxxxxxxxx>]

On 08/03/2022 11:38, Roger Pau Monné wrote:
> On Mon, Mar 07, 2022 at 03:06:09PM +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 virtualization, 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 decoding the
>> APIC access and providing a VM exit with a more specific exit reason
>> than a regular EPT fault or by altogether avoiding a VM exit.
>>
>> On the other hand, being able to disable x{2}APIC hardware assisted
>> virtualization can be useful for testing and debugging purposes.
>>
>> Note: vmx_install_vlapic_mapping doesn't require modifications
>> regardless of whether the guest has "Virtualize APIC accesses" enabled
>> or not, i.e., setting the APIC_ACCESS_ADDR VMCS field is fine so long
>> as virtualize_apic_accesses is supported by the CPU.
>>
>> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Signed-off-by: Jane Malalane <jane.malalane@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>
>>
>> v5:
>> * Revert v4 changes in vmx_vlapic_msr_changed(), preserving the use of
>>    the has_assisted_x{2}apic macros
>> * Following changes in assisted_x{2}apic_available definitions in
>>    patch 1, retighten conditionals for setting
>>    XEN_HVM_CPUID_APIC_ACCESS_VIRT and XEN_HVM_CPUID_X2APIC_VIRT in
>>    cpuid_hypervisor_leaves()
>>
>> v4:
>>   * Add has_assisted_x{2}apic macros and use them where appropriate
>>   * Replace CPU checks with per-domain assisted_x{2}apic control
>>     options in vmx_vlapic_msr_changed() and cpuid_hypervisor_leaves(),
>>     following edits to assisted_x{2}apic_available definitions in
>>     patch 1
>>     Note: new assisted_x{2}apic_available definitions make later
>>     cpu_has_vmx_apic_reg_virt and cpu_has_vmx_virtual_intr_delivery
>>     checks redundant in vmx_vlapic_msr_changed()
>>
>> v3:
>>   * Change info in xl.cfg to better express reality and fix
>>     capitalization of x{2}apic
>>   * Move "physinfo" variable definition to the beggining of
>>     libxl__domain_build_info_setdefault()
>>   * Reposition brackets in if statement to match libxl coding style
>>   * Shorten logic in libxl__arch_domain_build_info_setdefault()
>>   * Correct dprintk message in arch_sanitise_domain_config()
>>   * Make appropriate changes in vmx_vlapic_msr_changed() and
>>     cpuid_hypervisor_leaves() for amended "assisted_x2apic" bit
>>   * Remove unneeded parantheses
>>
>> v2:
>>   * Add a LIBXL_HAVE_ASSISTED_APIC macro
>>   * Pass xcpyshinfo as a pointer in libxl__arch_get_physinfo
>>   * Add a return statement in now "int"
>>     libxl__arch_domain_build_info_setdefault()
>>   * Preserve libxl__arch_domain_build_info_setdefault 's location in
>>     libxl_create.c
>>   * Correct x{2}apic default setting logic in
>>     libxl__arch_domain_prepare_config()
>>   * Correct logic for parsing assisted_x{2}apic host/guest options in
>>     xl_parse.c and initialize them to -1 in xl.c
>>   * Use guest options directly in vmx_vlapic_msr_changed
>>   * Fix indentation of bool assisted_x{2}apic in struct hvm_domain
>>   * Add a change in xenctrl_stubs.c to pass xenctrl ABI checks
>> ---
>>   docs/man/xl.cfg.5.pod.in                | 19 +++++++++++++++++++
>>   docs/man/xl.conf.5.pod.in               | 12 ++++++++++++
>>   tools/golang/xenlight/helpers.gen.go    | 12 ++++++++++++
>>   tools/golang/xenlight/types.gen.go      |  2 ++
>>   tools/include/libxl.h                   |  7 +++++++
>>   tools/libs/light/libxl_arch.h           |  5 +++--
>>   tools/libs/light/libxl_arm.c            |  7 +++++--
>>   tools/libs/light/libxl_create.c         | 22 +++++++++++++---------
>>   tools/libs/light/libxl_types.idl        |  2 ++
>>   tools/libs/light/libxl_x86.c            | 28 ++++++++++++++++++++++++++--
>>   tools/ocaml/libs/xc/xenctrl.ml          |  2 ++
>>   tools/ocaml/libs/xc/xenctrl.mli         |  2 ++
>>   tools/ocaml/libs/xc/xenctrl_stubs.c     |  2 +-
>>   tools/xl/xl.c                           |  8 ++++++++
>>   tools/xl/xl.h                           |  2 ++
>>   tools/xl/xl_parse.c                     | 16 ++++++++++++++++
>>   xen/arch/x86/domain.c                   | 28 +++++++++++++++++++++++++++-
>>   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/include/asm/hvm/vmx/vmcs.h |  3 +++
>>   xen/arch/x86/traps.c                    |  9 +++++----
>>   xen/include/public/arch-x86/xen.h       |  2 ++
>>   23 files changed, 183 insertions(+), 30 deletions(-)
>>
>> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
>> index b98d161398..dcca564a23 100644
>> --- a/docs/man/xl.cfg.5.pod.in
>> +++ b/docs/man/xl.cfg.5.pod.in
>> @@ -1862,6 +1862,25 @@ 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. With this option enabled, a memory-mapped APIC access will be
>> +decoded by hardware and either issue a more specific VM exit than just
>> +an EPT fault, or altogether avoid a VM exit. Notice full
>> +virtualization for xAPIC can only be achieved if hardware supports
>> +“APIC-register virtualization” and “virtual-interrupt delivery”.
> 
> You shouldn't mention “APIC-register virtualization” or
> “virtual-interrupt delivery”, as those are Intel specific options. I
> would just remove that sentence (same below).
> 
>> 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. With this option enabled, an MSR-Based APIC access will
>> +either issue a VM exit or altogether avoid one.
> 
> "With this option enabled, certain accesses to MSR APIC registers will
> avoid a VM exit into the hypervisor."
> 
>> Notice full
>> +virtualization for x2APIC can only be achieved if hardware supports
>> +“APIC-register virtualization” and “virtual-interrupt delivery”. The
>> +default is settable via L<xl.conf(5)>.
>> +
>>   =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..95d136d1ea 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 virtualization by 
>> default.
>> +
>> +Default: enabled if supported.
>> +
>> +=item B<assisted_x2apic=BOOLEAN>
>> +
>> +If enabled, domains will use x2APIC hardware assisted virtualization by 
>> default.
>> +
>> +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..dece545ee0 100644
>> --- a/tools/golang/xenlight/helpers.gen.go
>> +++ b/tools/golang/xenlight/helpers.gen.go
>> @@ -1120,6 +1120,12 @@ x.ArchArm.Vuart = VuartType(xc.arch_arm.vuart)
>>   if err := x.ArchX86.MsrRelaxed.fromC(&xc.arch_x86.msr_relaxed);err != nil {
>>   return fmt.Errorf("converting field ArchX86.MsrRelaxed: %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)
>> +}
>>   x.Altp2M = Altp2MMode(xc.altp2m)
>>   x.VmtraceBufKb = int(xc.vmtrace_buf_kb)
>>   if err := x.Vpmu.fromC(&xc.vpmu);err != nil {
>> @@ -1605,6 +1611,12 @@ xc.arch_arm.vuart = 
>> C.libxl_vuart_type(x.ArchArm.Vuart)
>>   if err := x.ArchX86.MsrRelaxed.toC(&xc.arch_x86.msr_relaxed); err != nil {
>>   return fmt.Errorf("converting field ArchX86.MsrRelaxed: %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)
>> +}
>>   xc.altp2m = C.libxl_altp2m_mode(x.Altp2M)
>>   xc.vmtrace_buf_kb = C.int(x.VmtraceBufKb)
>>   if err := x.Vpmu.toC(&xc.vpmu); err != nil {
>> diff --git a/tools/golang/xenlight/types.gen.go 
>> b/tools/golang/xenlight/types.gen.go
>> index 87be46c745..253c9ad93d 100644
>> --- a/tools/golang/xenlight/types.gen.go
>> +++ b/tools/golang/xenlight/types.gen.go
>> @@ -520,6 +520,8 @@ Vuart VuartType
>>   }
>>   ArchX86 struct {
>>   MsrRelaxed Defbool
>> +AssistedXapic Defbool
>> +AssistedX2Apic Defbool
>>   }
>>   Altp2M Altp2MMode
>>   VmtraceBufKb int
>> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
>> index 94e6355822..cdcccd6d01 100644
>> --- a/tools/include/libxl.h
>> +++ b/tools/include/libxl.h
>> @@ -535,6 +535,13 @@
>>   #define LIBXL_HAVE_PHYSINFO_ASSISTED_APIC 1
>>   
>>   /*
>> + * LIBXL_HAVE_ASSISTED_APIC indicates that libxl_domain_build_info has
>> + * assisted_xapic and assisted_x2apic fields for enabling hardware
>> + * assisted virtualization for x{2}apic per domain.
>> + */
>> +#define LIBXL_HAVE_ASSISTED_APIC 1
>> +
>> +/*
>>    * libxl ABI compatibility
>>    *
>>    * The only guarantee which libxl makes regarding ABI compatibility
>> diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h
>> index 207ceac6a1..03b89929e6 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 39fdca1b49..ba5b8f433f 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);
>> @@ -1399,6 +1400,8 @@ void 
>> libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>>       memset(&b_info->u, '\0', sizeof(b_info->u));
>>       b_info->type = LIBXL_DOMAIN_TYPE_INVALID;
>>       libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PVH);
>> +
>> +    return 0;
>>   }
>>   
>>   int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
>> diff --git a/tools/libs/light/libxl_create.c 
>> b/tools/libs/light/libxl_create.c
>> index 15ed021f41..88d08d7277 100644
>> --- a/tools/libs/light/libxl_create.c
>> +++ b/tools/libs/light/libxl_create.c
>> @@ -75,6 +75,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>>                                           libxl_domain_build_info *b_info)
>>   {
>>       int i, rc;
>> +    libxl_physinfo info;
>>   
>>       if (b_info->type != LIBXL_DOMAIN_TYPE_HVM &&
>>           b_info->type != LIBXL_DOMAIN_TYPE_PV &&
>> @@ -264,7 +265,18 @@ 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);
>> +    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;
>> +    }
>> +
>>       libxl_defbool_setdefault(&b_info->dm_restrict, false);
>>   
>>       if (b_info->iommu_memkb == LIBXL_MEMKB_DEFAULT)
>> @@ -457,14 +469,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>>       }
>>   
>>       if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
>> -        libxl_physinfo info;
>> -
>> -        rc = libxl_get_physinfo(CTX, &info);
>> -        if (rc) {
>> -            LOG(ERROR, "failed to get hypervisor info");
>> -            return rc;
>> -        }
>> -
>>           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 e0a06ecfe3..c377d13b19 100644
>> --- a/tools/libs/light/libxl_x86.c
>> +++ b/tools/libs/light/libxl_x86.c
>> @@ -23,6 +23,14 @@ 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 (d_config->c_info.type != LIBXL_DOMAIN_TYPE_PV)
>> +    {
>> +        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 +827,27 @@ 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)
>>   {
>>       libxl_defbool_setdefault(&b_info->acpi, true);
>>       libxl_defbool_setdefault(&b_info->arch_x86.msr_relaxed, false);
>> +
>> +    if (b_info->type != LIBXL_DOMAIN_TYPE_PV) {
>> +        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);
>> +    }
>> +
> 
> Extra newline? 'else if' should be one space after the closing
> bracket.
> 
>> +    else if (!libxl_defbool_is_default(b_info->arch_x86.assisted_xapic) ||
>> +             !libxl_defbool_is_default(b_info->arch_x86.assisted_x2apic)) {
>> +        LOG(ERROR, "Interrupt Controller Virtualization not supported for 
>> PV");
>> +        return ERROR_INVAL;
>> +    }
>> +
>> +    return 0;
>>   }
>>   
>>   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 21783d3622..672a11ceb6 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 af6ba3d1a0..f9a6aa3a0f 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/ocaml/libs/xc/xenctrl_stubs.c 
>> b/tools/ocaml/libs/xc/xenctrl_stubs.c
>> index e0d49b18d2..ecfc7125d5 100644
>> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
>> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
>> @@ -239,7 +239,7 @@ CAMLprim value stub_xc_domain_create(value xch, value 
>> wanted_domid, value config
>>   
>>              cfg.arch.misc_flags = ocaml_list_to_c_bitmap
>>                      /* ! x86_arch_misc_flags X86_ none */
>> -                    /* ! XEN_X86_ XEN_X86_MSR_RELAXED all */
>> +                    /* ! XEN_X86_ XEN_X86_ASSISTED_X2APIC max */
>>                      (VAL_MISC_FLAGS);
>>   
>>   #undef VAL_MISC_FLAGS
>> diff --git a/tools/xl/xl.c b/tools/xl/xl.c
>> index 2d1ec18ea3..31eb223309 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 = -1;
>> +int assisted_x2apic = -1;
>>   
>>   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..0ab9b145fe 100644
>> --- a/tools/xl/xl_parse.c
>> +++ b/tools/xl/xl_parse.c
>> @@ -1681,6 +1681,22 @@ 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_long(config, "assisted_xapic", &l , 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)
>> +            libxl_defbool_set(&b_info->arch_x86.assisted_xapic, l);
>> +        else
>> +            exit(1);
>> +
>> +        e = xlu_cfg_get_long(config, "assisted_x2apic", &l, 0);
>> +        if ((e == ESRCH && assisted_x2apic != -1)) /* use global default if 
>> present */
>> +            libxl_defbool_set(&b_info->arch_x86.assisted_x2apic, 
>> assisted_x2apic);
>> +        else if (!e)
>> +            libxl_defbool_set(&b_info->arch_x86.assisted_x2apic, l);
>> +        else
>> +            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 a5048ed654..bcca0dc900 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,31 @@ 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,
>> +                "Hardware assisted x%sAPIC requested but not available\n",
>> +                assisted_xapic && !assisted_xapic_available ? "" : "2");
>> +        return -EINVAL;
> 
> I think for those two you could return -ENODEV if others agree.
> 
>> +    }
>> +
>>       return 0;
>>   }
>>   
>> @@ -864,6 +884,12 @@ int arch_domain_create(struct domain *d,
>>   
>>       d->arch.msr_relaxed = config->arch.misc_flags & XEN_X86_MSR_RELAXED;
>>   
>> +    d->arch.hvm.assisted_xapic =
>> +        config->arch.misc_flags & XEN_X86_ASSISTED_XAPIC;
>> +
>> +    d->arch.hvm.assisted_x2apic =
>> +        config->arch.misc_flags & XEN_X86_ASSISTED_X2APIC;
>> +
>>       return 0;
>>   
>>    fail:
>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>> index 06831099ed..e4503a02a7 100644
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -1157,6 +1157,10 @@ static int construct_vmcs(struct vcpu *v)
>>           __vmwrite(PLE_WINDOW, ple_window);
>>       }
>>   
>> +    if ( !has_assisted_xapic(v->domain) )
>> +        v->arch.hvm.vmx.secondary_exec_control &=
>> +            ~SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
>> +
>>       if ( cpu_has_vmx_secondary_exec_control )
>>           __vmwrite(SECONDARY_VM_EXEC_CONTROL,
>>                     v->arch.hvm.vmx.secondary_exec_control);
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index c075370f64..949ddd684c 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -3344,16 +3344,11 @@ static void vmx_install_vlapic_mapping(struct vcpu 
>> *v)
>>   
>>   void vmx_vlapic_msr_changed(struct vcpu *v)
>>   {
>> -    int virtualize_x2apic_mode;
>>       struct vlapic *vlapic = vcpu_vlapic(v);
>>       unsigned int msr;
>>   
>> -    virtualize_x2apic_mode = ( (cpu_has_vmx_apic_reg_virt ||
>> -                                cpu_has_vmx_virtual_intr_delivery) &&
>> -                               cpu_has_vmx_virtualize_x2apic_mode );
>> -
>> -    if ( !cpu_has_vmx_virtualize_apic_accesses &&
>> -         !virtualize_x2apic_mode )
>> +    if ( !has_assisted_xapic(v->domain) &&
>> +         !has_assisted_x2apic(v->domain) )
>>           return;
>>   
>>       vmx_vmcs_enter(v);
>> @@ -3363,7 +3358,7 @@ void vmx_vlapic_msr_changed(struct vcpu *v)
>>       if ( !vlapic_hw_disabled(vlapic) &&
>>            (vlapic_base_address(vlapic) == APIC_DEFAULT_PHYS_BASE) )
>>       {
>> -        if ( virtualize_x2apic_mode && vlapic_x2apic_mode(vlapic) )
>> +        if ( has_assisted_x2apic(v->domain) && vlapic_x2apic_mode(vlapic) )
>>           {
>>               v->arch.hvm.vmx.secondary_exec_control |=
>>                   SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
>> @@ -3384,7 +3379,7 @@ void vmx_vlapic_msr_changed(struct vcpu *v)
>>                   vmx_clear_msr_intercept(v, MSR_X2APIC_SELF, VMX_MSR_W);
>>               }
>>           }
>> -        else
>> +        else if ( has_assisted_xapic(v->domain) )
>>               v->arch.hvm.vmx.secondary_exec_control |=
>>                   SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
>>       }
>> diff --git a/xen/arch/x86/include/asm/hvm/domain.h 
>> b/xen/arch/x86/include/asm/hvm/domain.h
>> index 698455444e..92bf53483c 100644
>> --- a/xen/arch/x86/include/asm/hvm/domain.h
>> +++ b/xen/arch/x86/include/asm/hvm/domain.h
>> @@ -117,6 +117,12 @@ struct hvm_domain {
>>   
>>       bool                   is_s3_suspended;
>>   
>> +    /* xAPIC hardware assisted virtualization. */
>> +    bool                   assisted_xapic;
>> +
>> +    /* x2APIC hardware assisted virtualization. */
>> +    bool                   assisted_x2apic;
>> +
>>       /* hypervisor intercepted msix table */
>>       struct list_head       msixtbl_list;
>>   
>> diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h 
>> b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
>> index 9119aa8536..5b7d662ed7 100644
>> --- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
>> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h
>> @@ -220,6 +220,9 @@ void vmx_vmcs_reload(struct vcpu *v);
>>   #define CPU_BASED_ACTIVATE_SECONDARY_CONTROLS 0x80000000
>>   extern u32 vmx_cpu_based_exec_control;
>>   
>> +#define has_assisted_xapic(d)   ((d)->arch.hvm.assisted_xapic)
>> +#define has_assisted_x2apic(d)  ((d)->arch.hvm.assisted_x2apic)
> 
> Those macros should not be in an Intel specific header,
> arch/x86/include/asm/hvm/domain.h is likely a better place.

Actually do you think hvm.h could be better?

diff --git a/xen/arch/x86/include/asm/hvm/hvm.h 
b/xen/arch/x86/include/asm/hvm/hvm.h
index 5b7ec0cf69..65a978f670 100644
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -373,6 +373,12 @@ int hvm_get_param(struct domain *d, uint32_t index, 
uint64_t *value);
  #define hvm_tsc_scaling_ratio(d) \
      ((d)->arch.hvm.tsc_scaling_ratio)

+#define has_assisted_xapic(d) \
+    ((d)->arch.hvm.assisted_xapic)
+
+#define has_assisted_x2apic(d) \
+    ((d)->arch.hvm.assisted_x2apic)
+
  #define hvm_get_guest_time(v) hvm_get_guest_time_fixed(v, 0)

  #define hvm_paging_enabled(v) \

Thank you,

Jane.