Re: [PATCH RESEND v10 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC
On 11/07/2022 09:26, Jan Beulich wrote:
> On 11.07.2022 10:00, Jane Malalane wrote:
>> On 30/06/2022 07:03, Jan Beulich wrote:
>>> On 30.06.2022 05:25, Tian, Kevin wrote:
> From: Jane Malalane
> Sent: Wednesday, June 29, 2022 9:56 PM
>
> Introduce a new per-domain creation x86 specific flag to
> select whether hardware assisted virtualization should be used for
> x{2}APIC.
>
> A per-domain option is added to xl in order to select the usage of
> x{2}APIC hardware assisted virtualization, as well as a global
> configuration option.
>
> Having all APIC interaction exit to Xen for emulation is slow and can
> induce much overhead. Hardware can speed up x{2}APIC by decoding the
> APIC access and providing a VM exit with a more specific exit reason
> than a regular EPT fault or by altogether avoiding a VM exit.
Above is obvious and could be removed.
I think the key is just the next paragraph for why we
want this per-domain control.
>>>
>>> Indeed, but the paragraph above sets the context. It might be possible
>>> to shorten it, but ...
>>>
Apart from that:
Reviewed-by: Kevin Tian
>
> On the other hand, being able to disable x{2}APIC hardware assisted
> virtualization can be useful for testing and debugging purposes.
>>>
>>> ... I think it is desirable for this sentence to start with "Otoh" or
>>> alike.
>>>
>>> JanHello Jan,
>>
>> In the previous email, I was referring to this discussion about the
>> commit message. I haven't sent out a v11 because there was no change
>> other than this one suggested. What I said earlier was that I thought
>> the "Having all APIC interaction exit to Xen for emulation is slow..."
>> paragraph provided context for what I say after but I am happy to remove it.
>
> I'd be fine for it to be kept as you had it, but you really should have
> sent out both patches. There are rare cases where sending out individual
> updates within a series is reasonable (e.g. to avoid spamming the list
> with a large amount of unchanged patches), but I think here you want to
> make things easy for committers and not have them hunt down the earlier
> version.
Apologies, makes sense.
Thanks,
Jane.
Re: [PATCH RESEND v10 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC
On 11.07.2022 10:00, Jane Malalane wrote:
> On 30/06/2022 07:03, Jan Beulich wrote:
>> On 30.06.2022 05:25, Tian, Kevin wrote:
From: Jane Malalane
Sent: Wednesday, June 29, 2022 9:56 PM
Introduce a new per-domain creation x86 specific flag to
select whether hardware assisted virtualization should be used for
x{2}APIC.
A per-domain option is added to xl in order to select the usage of
x{2}APIC hardware assisted virtualization, as well as a global
configuration option.
Having all APIC interaction exit to Xen for emulation is slow and can
induce much overhead. Hardware can speed up x{2}APIC by decoding the
APIC access and providing a VM exit with a more specific exit reason
than a regular EPT fault or by altogether avoiding a VM exit.
>>>
>>> Above is obvious and could be removed.
>>>
>>> I think the key is just the next paragraph for why we
>>> want this per-domain control.
>>
>> Indeed, but the paragraph above sets the context. It might be possible
>> to shorten it, but ...
>>
>>> Apart from that:
>>>
>>> Reviewed-by: Kevin Tian
>>>
On the other hand, being able to disable x{2}APIC hardware assisted
virtualization can be useful for testing and debugging purposes.
>>
>> ... I think it is desirable for this sentence to start with "Otoh" or
>> alike.
>>
>> JanHello Jan,
>
> In the previous email, I was referring to this discussion about the
> commit message. I haven't sent out a v11 because there was no change
> other than this one suggested. What I said earlier was that I thought
> the "Having all APIC interaction exit to Xen for emulation is slow..."
> paragraph provided context for what I say after but I am happy to remove it.
I'd be fine for it to be kept as you had it, but you really should have
sent out both patches. There are rare cases where sending out individual
updates within a series is reasonable (e.g. to avoid spamming the list
with a large amount of unchanged patches), but I think here you want to
make things easy for committers and not have them hunt down the earlier
version.
Jan
Re: [PATCH RESEND v10 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC
On 30/06/2022 07:03, Jan Beulich wrote:
> On 30.06.2022 05:25, Tian, Kevin wrote:
>>> From: Jane Malalane
>>> Sent: Wednesday, June 29, 2022 9:56 PM
>>>
>>> Introduce a new per-domain creation x86 specific flag to
>>> select whether hardware assisted virtualization should be used for
>>> x{2}APIC.
>>>
>>> A per-domain option is added to xl in order to select the usage of
>>> x{2}APIC hardware assisted virtualization, as well as a global
>>> configuration option.
>>>
>>> Having all APIC interaction exit to Xen for emulation is slow and can
>>> induce much overhead. Hardware can speed up x{2}APIC by decoding the
>>> APIC access and providing a VM exit with a more specific exit reason
>>> than a regular EPT fault or by altogether avoiding a VM exit.
>>
>> Above is obvious and could be removed.
>>
>> I think the key is just the next paragraph for why we
>> want this per-domain control.
>
> Indeed, but the paragraph above sets the context. It might be possible
> to shorten it, but ...
>
>> Apart from that:
>>
>> Reviewed-by: Kevin Tian
>>
>>>
>>> On the other hand, being able to disable x{2}APIC hardware assisted
>>> virtualization can be useful for testing and debugging purposes.
>
> ... I think it is desirable for this sentence to start with "Otoh" or
> alike.
>
> JanHello Jan,
In the previous email, I was referring to this discussion about the
commit message. I haven't sent out a v11 because there was no change
other than this one suggested. What I said earlier was that I thought
the "Having all APIC interaction exit to Xen for emulation is slow..."
paragraph provided context for what I say after but I am happy to remove it.
Thanks,
Jane.
Re: [PATCH RESEND v10 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC
On 30.06.2022 05:25, Tian, Kevin wrote:
>> From: Jane Malalane
>> Sent: Wednesday, June 29, 2022 9:56 PM
>>
>> Introduce a new per-domain creation x86 specific flag to
>> select whether hardware assisted virtualization should be used for
>> x{2}APIC.
>>
>> A per-domain option is added to xl in order to select the usage of
>> x{2}APIC hardware assisted virtualization, as well as a global
>> configuration option.
>>
>> Having all APIC interaction exit to Xen for emulation is slow and can
>> induce much overhead. Hardware can speed up x{2}APIC by decoding the
>> APIC access and providing a VM exit with a more specific exit reason
>> than a regular EPT fault or by altogether avoiding a VM exit.
>
> Above is obvious and could be removed.
>
> I think the key is just the next paragraph for why we
> want this per-domain control.
Indeed, but the paragraph above sets the context. It might be possible
to shorten it, but ...
> Apart from that:
>
> Reviewed-by: Kevin Tian
>
>>
>> On the other hand, being able to disable x{2}APIC hardware assisted
>> virtualization can be useful for testing and debugging purposes.
... I think it is desirable for this sentence to start with "Otoh" or
alike.
Jan
RE: [PATCH RESEND v10 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC
> From: Jane Malalane
> Sent: Wednesday, June 29, 2022 9:56 PM
>
> Introduce a new per-domain creation x86 specific flag to
> select whether hardware assisted virtualization should be used for
> x{2}APIC.
>
> A per-domain option is added to xl in order to select the usage of
> x{2}APIC hardware assisted virtualization, as well as a global
> configuration option.
>
> Having all APIC interaction exit to Xen for emulation is slow and can
> induce much overhead. Hardware can speed up x{2}APIC by decoding the
> APIC access and providing a VM exit with a more specific exit reason
> than a regular EPT fault or by altogether avoiding a VM exit.
Above is obvious and could be removed.
I think the key is just the next paragraph for why we
want this per-domain control.
Apart from that:
Reviewed-by: Kevin Tian
>
> On the other hand, being able to disable x{2}APIC hardware assisted
> virtualization can be useful for testing and debugging purposes.
>
> Note:
>
> - vmx_install_vlapic_mapping doesn't require modifications regardless
> of whether the guest has "Virtualize APIC accesses" enabled or not,
> i.e., setting the APIC_ACCESS_ADDR VMCS field is fine so long as
> virtualize_apic_accesses is supported by the CPU.
>
> - Both per-domain and global assisted_x{2}apic options are not part of
> the migration stream, unless explicitly set in the respective
> configuration files. Default settings of assisted_x{2}apic done
> internally by the toolstack, based on host capabilities at create
> time, are not migrated.
>
> Suggested-by: Andrew Cooper
> Signed-off-by: Jane Malalane
> Acked-by: Christian Lindig
> Reviewed-by: "Roger Pau Monné"
> Reviewed-by: Anthony PERARD
> ---
> CC: Wei Liu
> CC: Anthony PERARD
> CC: George Dunlap
> CC: Nick Rosbrook
> CC: Juergen Gross
> CC: Christian Lindig
> CC: David Scott
> CC: Jan Beulich
> CC: Andrew Cooper
> CC: "Roger Pau Monné"
> CC: Jun Nakajima
> CC: Kevin Tian
>
> v10:
> * Improve commit message note on migration
>
> v9:
> * Fix style issues
> * Fix exit() logic for assisted_x{2}apic parsing
> * Add and use XEN_X86_MISC_FLAGS_MAX for ABI checking instead of
>using XEN_X86_ASSISTED_X2APIC directly
> * Expand commit message to mention migration is safe
>
> v8:
> * Widen assisted_x{2}apic parsing to PVH guests in
>parse_config_data()
>
> v7:
> * Fix void return in libxl__arch_domain_build_info_setdefault
> * Fix style issues
> * Use EINVAL when rejecting assisted_x{2}apic for PV guests and
>ENODEV otherwise, when assisted_x{2}apic isn't supported
> * Define has_assisted_x{2}apic macros for when !CONFIG_HVM
> * Replace "EPT" fault reference with "p2m" fault since the former is
>Intel-specific
>
> v6:
> * Use ENODEV instead of EINVAL when rejecting assisted_x{2}apic
>for PV guests
> * Move has_assisted_x{2}apic macros out of an Intel specific header
> * Remove references to Intel specific features in documentation
>
> v5:
> * Revert v4 changes in vmx_vlapic_msr_changed(), preserving the use of
>the has_assisted_x{2}apic macros
> * Following changes in assisted_x{2}apic_available definitions in
>patch 1, retighten conditionals for setting
>XEN_HVM_CPUID_APIC_ACCESS_VIRT and XEN_HVM_CPUID_X2APIC_VIRT
> in
>cpuid_hypervisor_leaves()
>
> v4:
> * Add has_assisted_x{2}apic macros and use them where appropriate
> * Replace CPU checks with per-domain assisted_x{2}apic control
>options in vmx_vlapic_msr_changed() and cpuid_hypervisor_leaves(),
>following edits to assisted_x{2}apic_available definitions in
>patch 1
>Note: new assisted_x{2}apic_available definitions make later
>cpu_has_vmx_apic_reg_virt and cpu_has_vmx_virtual_intr_delivery
>checks redundant in vmx_vlapic_msr_changed()
>
> v3:
> * Change info in xl.cfg to better express reality and fix
>capitalization of x{2}apic
> * Move "physinfo" variable definition to the beggining of
>libxl__domain_build_info_setdefault()
> * Reposition brackets in if statement to match libxl coding style
> * Shorten logic in libxl__arch_domain_build_info_setdefault()
> * Correct dprintk message in arch_sanitise_domain_config()
> * Make appropriate changes in vmx_vlapic_msr_changed() and
>cpuid_hypervisor_leaves() for amended "assisted_x2apic" bit
> * Remove unneeded parantheses
>
> v2:
> * Add a LIBXL_HAVE_ASSISTED_APIC macro
> * Pass xcpyshinfo as a pointer in libxl__arch_get_physinfo
> * Add a return statement in now "int"
>libxl__arch_domain_build_info_setdefault()
> * Preserve libxl__arch_domain_build_info_setdefault 's location in
>libxl_create.c
> * Correct x{2}apic default setting logic in
>libxl__arch_domain_prepare_config()
> * Correct logic for parsing assisted_x{2}apic host/guest options in
>xl_parse.c and initialize them to -1 in xl.c
> * Use guest options directly in vmx_vlapic_msr_changed
> * Fix indentation of bool assisted_x{2}apic in struct hvm_domain
> * Add a change in
