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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.