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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.03.2022 14:16, Jane Malalane wrote:
> On 07/03/2022 12:31, 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 07.03.2022 13:17, Jane Malalane wrote:
>>> On 04/03/2022 08:17, 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 03.03.2022 17:37, Jane Malalane wrote:
>>>>> On 03/03/2022 11:37, Jan Beulich wrote:
>>>>>> On 02.03.2022 16:00, 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.
>>>>>>>
>>>>>>> Note that this interface is intended to be compatible with AMD so that
>>>>>>> AVIC support can be introduced in a future patch. Unlike Intel that
>>>>>>> has multiple controls for APIC Virtualization, AMD has one global
>>>>>>> 'AVIC Enable' control bit, so fine-graining of APIC virtualization
>>>>>>> control cannot be done on a common interface. Therefore, for xAPIC HW
>>>>>>> assisted virtualization support to be reported, HW must support
>>>>>>> virtualize_apic_accesses as well as apic_reg_virt.
>>>>>>
>>>>>> Okay, here you now describe _what_ is being implemented, but I'm
>>>>>> afraid it still lacks justification (beyond making this re-usable for
>>>>>> AVIC, which imo can only be a secondary goal). You actually say ...
>>> Is the following any better...?
>>>
>>> "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.
>>>
>>> HW assisted xAPIC virtualization will be reported if HW, at the minimum,
>>>    supports virtualize_apic_accesses as this feature alone means that an
>>> access to the APIC page will cause an APIC-access VM exit. An
>>> APIC-access VM exit provides a VMM with information about the access
>>> causing the VM exit, unlike a regular EPT fault, thus simplifying some
>>> internal handling.
>>>
>>> HW assisted x2APIC virtualization will be reported if HW supports
>>> virtualize_x2apic_mode and, at least, either apic_reg_virt or
>>> virtual_intr_delivery. This is due to apic_reg_virt and
>>> virtual_intr_delivery preventing a VM exit from occuring or at least
>>> replacing a regular EPT fault VM-exit with an APIC-access VM-exit on
>>> read and write APIC accesses, respectively.
>>> This also means that sysctl follows the conditionals in
>>> vmx_vlapic_msr_changed().
>>>
>>> For that purpose, also add an arch-specific "capabilities" parameter
>>> to struct xen_sysctl_physinfo.
>>>
>>> Note that this interface is intended to be compatible with AMD so that
>>> AVIC support can be introduced in a future patch. Unlike Intel that
>>> has multiple controls for APIC Virtualization, AMD has one global
>>> 'AVIC Enable' control bit, so fine-graining of APIC virtualization
>>> control cannot be done on a common interface."
>>
>> Yes, this looks quite a bit better, thanks. Assuming, of course, it's
>> in sync with the code in v5 ...
> Yes, ofc.
> 
> Just one thing, since vmx_vlapic_msr_changed() uses 
> has_assisted_x{2}apic anyway do you think it's still necessary to add a 
> comment pointing to this function in vmx_init_vmcs_config() when setting 
> asissted_x{2}apic_available and v.v. ?

If they both use the same, non-open-coded condition, then I don't think a
cross reference is needed.

Jan