On 08/02/2022 16:17, Roger Pau Monné wrote:
> On Mon, Feb 07, 2022 at 06:21:01PM +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.
>>
>> Being able to disable x{2}APIC hardware assisted vitualization can be
>> useful for testing and debugging purposes.
>
> I think you have agreed with Jan some changes to the description
> regarding the purpose of the commit.
Yes.
>
>>
>> Signed-off-by: Jane Malalane <jane.malal...@citrix.com>
>> Suggested-by: Andrew Cooper <andrew.coop...@citrix.com>
Will swap the ordering, forgot to do this previously, apologies.
>> ---
>> CC: Wei Liu <w...@xen.org>
>> CC: Anthony PERARD <anthony.per...@citrix.com>
>> CC: Juergen Gross <jgr...@suse.com>
>> CC: Andrew Cooper <andrew.coop...@citrix.com>
>> CC: George Dunlap <george.dun...@citrix.com>
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Julien Grall <jul...@xen.org>
>> CC: Stefano Stabellini <sstabell...@kernel.org>
>> CC: Christian Lindig <christian.lin...@citrix.com>
>> CC: David Scott <d...@recoil.org>
>> CC: Volodymyr Babchuk <volodymyr_babc...@epam.com>
>> CC: "Roger Pau Monné" <roger....@citrix.com>
>>
>> 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 | 10 ++++++++++
>> docs/man/xl.conf.5.pod.in | 12 ++++++++++++
>> tools/golang/xenlight/helpers.gen.go | 12 ++++++++++++
>> 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 | 23 ++++++++++++++---------
>> tools/libs/light/libxl_types.idl | 2 ++
>> tools/libs/light/libxl_x86.c | 31 +++++++++++++++++++++++++++++--
>> 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 | 14 +++++---------
>> xen/arch/x86/include/asm/hvm/domain.h | 6 ++++++
>> xen/arch/x86/traps.c | 8 ++++----
>> xen/include/public/arch-x86/xen.h | 2 ++
>> 21 files changed, 173 insertions(+), 30 deletions(-)
>>
>> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
>> index b98d161398..1d98bbd182 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)>.
>> +
>> =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..30993827e5 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.
>
> We don't capitalize xl options, so I would suggest to lowercase APIC
> for all the option names.
Okay.
>
>> +
>> =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/include/libxl.h b/tools/include/libxl.h
>> index 924e142628..83944c17ae 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_x{2}apic 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 d7a40d7550..843e523df9 100644
>> --- a/tools/libs/light/libxl_create.c
>> +++ b/tools/libs/light/libxl_create.c
>> @@ -264,7 +264,20 @@ 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_physinfo info;
>
> The definition of info needs to be at the top of the function,
> together with the rest of the variable definitions.
Okay.
>
>> +
>> + 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 +470,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..f0fa0ceea2 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)
>> + {
>
> Coding style for libxl is to place the bracket in the same line as the
> if.
Okay.
>
>> + 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,30 @@ 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);
>> + }
>> +
>> + 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)))
>
> You could just do:
>
> 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);
> } else if (!libxl_defbool_is_default(b_info->arch_x86.assisted_xapic) ||
> !libxl_defbool_is_default(b_info->arch_x86.assisted_x2apic))
> /* ERROR */
>
>> + {
>> + LOG(ERROR, "Interrupt Controller Virtualization not supported for
>> PV");
>> + return ERROR_INVAL;
>> + }
>> +
>> + return 0;
True.
>> }
>>
>> 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/ocaml/libs/xc/xenctrl_stubs.c
>> b/tools/ocaml/libs/xc/xenctrl_stubs.c
>> index 5b4fe72c8d..0aa957d379 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 */
>
> We would usually define an XEN_X86_MISC_MAX that would point to
> XEN_X86_ASSISTED_X2APIC currently.
>
>> (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 */
> ^ no need for the extra parentheses here and below.
>
>> + 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 ef1812dc14..9033a0e181 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;
>> + }
>> +
>> return 0;
>> }
>>
>> @@ -863,6 +883,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 4060aef1bd..614db5c4a4 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 ( !v->domain->arch.hvm.assisted_xapic )
>> + 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 36c8a12cfe..3c9ff60154 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -3333,16 +3333,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 ( ! v->domain->arch.hvm.assisted_xapic &&
>> + ! v->domain->arch.hvm.assisted_x2apic )
> ^ extra space.
>> return;
>>
>> vmx_vmcs_enter(v);
>> @@ -3352,7 +3347,8 @@ 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 ( v->domain->arch.hvm.assisted_x2apic
>> + && vlapic_x2apic_mode(vlapic) )
>> {
>> v->arch.hvm.vmx.secondary_exec_control |=
>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
>> @@ -3373,7 +3369,7 @@ void vmx_vlapic_msr_changed(struct vcpu *v)
>> vmx_clear_msr_intercept(v, MSR_X2APIC_SELF, VMX_MSR_W);
>> }
>> }
>> - else
>> + else if ( v->domain->arch.hvm.assisted_xapic )
>> 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/traps.c b/xen/arch/x86/traps.c
>> index 485bd66971..33694acc99 100644
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -1115,7 +1115,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v,
>> uint32_t leaf,
>> if ( !is_hvm_domain(d) || subleaf != 0 )
>> break;
>>
>> - if ( cpu_has_vmx_apic_reg_virt )
>> + if ( cpu_has_vmx_apic_reg_virt &&
>
> You can drop the cpu_has_vmx_apic_reg_virt check here, if
> cpu_has_vmx_apic_reg_virt is false assisted_xapic won't be set to true.
Oh, but assisted_xapic_available is only set to depend on
cpu_has_vmx_virtualize_apic_accesses, unless I should correct this, but
I might be missing something...
>
>> + v->domain->arch.hvm.assisted_xapic )
>> res->a |= XEN_HVM_CPUID_APIC_ACCESS_VIRT;
>>
>> /*
>> @@ -1124,9 +1125,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v,
>> uint32_t leaf,
>> * and wrmsr in the guest will run without VMEXITs (see
>> * vmx_vlapic_msr_changed()).
>> */
>> - if ( cpu_has_vmx_virtualize_x2apic_mode &&
>> - cpu_has_vmx_apic_reg_virt &&
>> - cpu_has_vmx_virtual_intr_delivery )
>> + if ( (cpu_has_vmx_apic_reg_virt &&
>> cpu_has_vmx_virtual_intr_delivery) &&
> ^ unneeded parentheses
>
Okay.
> Thanks, Roger.
Thank you,
Jane.
