On Thu, Mar 17, 2022 at 09:58:15AM +0100, Roger Pau Monné wrote:
> On Wed, Mar 16, 2022 at 09:13:15AM +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.
>
> Have you tested migration of guests with this patch applied?
>
> We need to be careful so that a guest that doesn't have
> assisted_x{2}apic set in the config file can be migrated between hosts
> that have different support for hardware assisted x{2}APIC
> virtualization.
>
> Ie: we need to make sure the selection of arch_x86.assisted_x{2}apic
> is only present in the migration stream when explicitly set in the
> configuration file.
I've tested this myself, and the behavior seems to be correct. The
selection is only present on the stream when explicitly set by the
user.
> > 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..f118dc7e97 100644
> > --- a/tools/xl/xl_parse.c
> > +++ b/tools/xl/xl_parse.c
> > @@ -2761,6 +2761,24 @@ skip_usbdev:
> >
> > xlu_cfg_get_defbool(config, "vpmu", &b_info->vpmu, 0);
> >
> > + if (b_info->type != LIBXL_DOMAIN_TYPE_PV) {
> > + 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);
>
> Indentation seems wrong in the line above.
The chunk above is also logically wrong, because it will exit(1) if
no assisted_x{2}apic option is provided on either the guest or the
global config files.
I think:
e = xlu_cfg_get_long(config, "assisted_xapic", &l , 0);
if (!e)
libxl_defbool_set(&b_info->arch_x86.assisted_xapic, l);
else if (e != ESRCH)
exit(1);
else if (assisted_xapic != -1) /* use global default if present */
libxl_defbool_set(&b_info->arch_x86.assisted_xapic, assisted_xapic);
e = xlu_cfg_get_long(config, "assisted_x2apic", &l, 0);
if (!e)
libxl_defbool_set(&b_info->arch_x86.assisted_x2apic, l);
else if (e != ESRCH)
exit(1);
else if (assisted_x2apic != -1) /* use global default if present */
libxl_defbool_set(&b_info->arch_x86.assisted_x2apic,
assisted_x2apic);
Is better.
Thanks, Roger.
