Re: [PATCH v3 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86

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

On Mon, Feb 28, 2022 at 05:14:26PM +0100, Jan Beulich wrote:
> On 28.02.2022 16:36, Roger Pau Monné wrote:
> > On Mon, Feb 28, 2022 at 02:11:04PM +0100, Jan Beulich wrote:
> >> On 28.02.2022 11:59, Roger Pau Monné wrote:
> >>> On Thu, Feb 24, 2022 at 03:08:41PM +0100, Jan Beulich wrote:
> >>>> On 18.02.2022 18:29, Jane Malalane wrote:
> >>>>> Add XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_xapic and
> >>>>> XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_x2apic to report accelerated xapic
> >>>>> and x2apic, on x86 hardware.
> >>>>> No such features are currently implemented on AMD hardware.
> >>>>>
> >>>>> For that purpose, also add an arch-specific "capabilities" parameter
> >>>>> to struct xen_sysctl_physinfo.
> >>>>>
> >>>>> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >>>>> Signed-off-by: Jane Malalane <jane.malalane@xxxxxxxxxx>
> >>>>> ---
> >>>>> v3:
> >>>>>  * Define XEN_SYSCTL_PHYSCAP_ARCH_MAX for ABI checking and actually
> >>>>>    set arch_capbilities, via a call to c_bitmap_to_ocaml_list()
> >>>>>  * Have assisted_x2apic_available only depend on
> >>>>>    cpu_has_vmx_virtualize_x2apic_mode
> >>>>
> >>>> I understand this was the result from previous discussion, but this
> >>>> needs justifying in the description. Not the least because it differs
> >>>> from when XEN_HVM_CPUID_X2APIC_VIRT would be set as well as from what
> >>>> vmx_vlapic_msr_changed() does. The difference between those two is
> >>>> probably intended (judging from a comment there), but the further
> >>>> difference to what you add isn't obvious.
> >>>>
> >>>> Which raises another thought: If that hypervisor leaf was part of the
> >>>> HVM feature set, the tool stack could be able to obtain the wanted
> >>>> information without altering sysctl (assuming the conditions to set
> >>>> the respective bits were the same). And I would view it as generally
> >>>> reasonable for there to be a way for tool stacks to know what
> >>>> hypervisor leaves guests are going to get to see (at the maximum and
> >>>> by default).
> >>>
> >>> I'm not sure using CPUID would be appropriate for this. Those fields
> >>> are supposed to be used by a guest to decide whether it should prefer
> >>> the x{2}APIC over PV alternatives for certain operations (ie: IPIs for
> >>> example), but the level of control we can provide with the sysctl is
> >>> more fine grained.
> >>>
> >>> The current proposal is limited to the exposure and control of the
> >>> usage of APIC virtualization, but we could also expose availability
> >>> and per-domain enablement of APIC register virtualization and posted
> >>> interrupts.
> >>
> >> But then I would still like to avoid duplication of information
> >> exposure and expose through the featureset what can be exposed there
> >> and limit sysctl to what cannot be expressed otherwise.
> > 
> > So you would rather prefer to expose this information in a synthetic
> > CPUID leaf?
> 
> Depends on what you mean by "synthetic leaf". We already have a leaf.
> What I'm suggesting to consider to the give that hypervisor leaf a
> representation in the featureset.

Hm, but then we won't be able to expose more fine grained controls,
ie: separate between basic APIC virtualization support, APIC register
virtualization and interrupt virtualization. We would need to keep the
meaning of XEN_HVM_CPUID_APIC_ACCESS_VIRT / XEN_HVM_CPUID_X2APIC_VIRT
(and exposing more fine grained features to guests make no sense).

Thanks, Roger.