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

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

On Tue, Mar 01, 2022 at 08:51:59AM +0100, Jan Beulich wrote:
> On 28.02.2022 17:31, Roger Pau Monné wrote:
> > 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).
> 
> I did say before that once (if ever) finer grained controls are wanted,
> a sysctl like suggested would indeed look to be the way to report the
> capability. But we aren't at that point.

So we would expose everything in the 0x40000000 range, and caller
would figure out the position of the Xen leaves doing a signature
check until the Xen leaf is found?

Would we represent the max policy as having Viridian enabled (so Xen
leaves starting at 0x40000100)?

I would agree with this if the hypervisor leaves where already part of
the managed CPUID data, but the work required to expose the leaves in
the policy/featureset doesn't seem trivial. Making those leaves part
of the policy will likely be done at some point, and then we can
decide to drop the sysctl bits.

Thanks, Roger.