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

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

On Wed, Feb 02, 2022 at 03:19:13PM +0000, Jane Malalane wrote:
> On 01/02/2022 10:02, Roger Pau Monné wrote:
> > On Thu, Jan 27, 2022 at 04:01:33PM +0000, Jane Malalane wrote:
> >> 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
> >> @@ -819,11 +825,44 @@ 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)
> >>   {
> >> +    int rc;
> >> +    bool assisted_xapic;
> >> +    bool assisted_x2apic;
> >> +
> >>       libxl_defbool_setdefault(&b_info->acpi, true);
> >>       libxl_defbool_setdefault(&b_info->arch_x86.msr_relaxed, false);
> >> +
> >> +    libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, false);
> >> +    libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, false);
> >> +
> >> +    assisted_xapic = libxl_defbool_val(b_info->arch_x86.assisted_xapic);
> >> +    assisted_x2apic = libxl_defbool_val(b_info->arch_x86.assisted_x2apic);
> >> +
> >> +    if ((assisted_xapic || assisted_x2apic) &&
> >> +        b_info->type == LIBXL_DOMAIN_TYPE_PV)
> >> +    {
> >> +        LOG(ERROR, "Interrupt Controller Virtualization not supported for 
> >> PV");
> >> +        rc = ERROR_INVAL;
> >> +        goto out;
> >> +    }
> >> +
> >> +    if ((assisted_xapic && !physinfo->cap_assisted_xapic) ||
> >> +         (assisted_x2apic && !physinfo->cap_assisted_x2apic))
> >> +    {
> >> +        LOG(ERROR, "x%sAPIC hardware supported emulation not available",
> >> +            assisted_xapic && !physinfo->cap_assisted_xapic ? "" : "2");
> >> +        rc =  ERROR_INVAL;
> >> +        goto out;
> >> +    }
> > 
> > I think the logic here is slightly wrong, as you are setting the
> > default value of assisted_x{2}apic to false, and we would instead like
> > to set it to the current value supported by the hardware in order to
> > keep current behavior.
> > 
> > Also the options are HVM/PVH only, so having them set for PV should
> > result in an error regardless of the set value, ie:
> > 
> > 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)))
> >       ERROR
> > 
> > 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);
> > 
> > I don't think you need the local assisted_x{2}apic variables.
> 
> Makes sense. In that case, could I instead just have this?
> 
> if (b_info->type != LIBXL_DOMAIN_TYPE_PV)
> {
>      if (physinfo->cap_assisted_xapic)
>          libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, true);
>      if (physinfo->cap_assisted_x2apic)
>          libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, true);

I think you actively need to set assisted_x{2}apic if they are using
default values, or else a later call to libxl_defbool_val will cause
an assert to trigger.

libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic,
                         physinfo->cap_assisted_xapic);


assisted_x{2}apic need to either resolve to true or false past this
point, but must not be left using it's default (uninitialized) value.

> }
> 
> Or do i still need to also check that assisted_x{2}apic hasn't been set 
> elsewhere for PV domains, in which case, I'm happy to add the code you 
> proposed above with this code I have here too.

I would prefer if we actively rejected options that don't make
sense.

It's wrong to try to set assisted_x{2}apic for PV because there's no
APIC at all in that case. I will defer to the maintainer, but I would
prefer if an error was reported in that case. I know we are not
consistent in that regard, so I'm not going to block what you propose.

Thanks, Roger.