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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.02.2022 13:09, Jane Malalane wrote:
> On 28/02/2022 07:32, Jan Beulich wrote:
>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments 
>> unless you have verified the sender and know the content is safe.
>>
>> On 25.02.2022 17:02, Jane Malalane wrote:
>>> On 24/02/2022 14:08, 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.
>>>
>>> Okay, I will make that explicit.
>>>
>>>> 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).
>>>
>>> Like the "cpuid" xtf test allows us to?
>>
>> I don't think I understand the question. That xtf test is concerned
>> about checking the CPUID output it gets to see itself. It doesn't care
>> about what other guests might get to see, nor the maximum and default.
>>
>>> Makes sense to me. I'm happy to take that up after.
>>
>> "After" what?
> So I meant to say that I could add the Xen CPUID leaves (40000x...) to 
> the policy so that toolstacks could know what hypervisor leaves guests 
> are going to see - in a future patch, as this wouldn't just expose 
> XEN_HVM_CPUID_APIC_ACCESS_VIRT and XEN_HVM_CPUID_X2APIC_VIRT 
> (0x40000x04) but other features too.

But doing this in a future patch (i.e. subsequent to this one) would
mean to first introduce the sysctl just to then rip it out again.
Hence my desire to consider the alternative before we settle on the
sysctl.

> But, at the same time, w.r.t. this patch in particular, using 
> XEN_HVM_CPUID_APIC_ACCESS_VIRT and XEN_HVM_CPUID_X2APIC_VIRT to detect 
> assisted APIC gives us less flexibility to add more fine grained 
> controls in the future.

I'm afraid I can't follow: All I'm talking about is how to expose the
same kind of information to the tool stack. I don't see how the
mechanism chosen would limit flexibility going forward.

Jan