On 01.03.2022 15:19, Roger Pau Monné wrote:
> 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.coop...@citrix.com>
>>>>>>>>> Signed-off-by: Jane Malalane <jane.malal...@citrix.com>
>>>>>>>>> ---
>>>>>>>>> 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?
The leaf number doesn't matter. The FEATURESET_* constants are part of
the ABI (just that the names we give them aren't in the public headers).
There would simply be a new FEATURESET_x4a.
> 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.
I may well be underestimating the amount of work involved. I wanted to
put this up as a possible alternative. Seeing that it's not liked, I'm
not going to insist to further pursue this.
Jan
