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

[Jane Malalane <Jane.Malalane@citrix.com>]

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@citrix.com>
>>>> Signed-off-by: Jane Malalane <jane.malalane@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.
>>
>> 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, 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.

Thanks,

Jane.