[PATCH 0/5] i386/hvf: x2apic support and some small fixes

Phil Dennis-Jordan posted 5 patches 2 weeks, 3 days ago
target/i386/hvf/hvf.c       |  7 +++----
target/i386/hvf/x86_cpuid.c |  6 +++---
target/i386/hvf/x86_emu.c   | 42 +++++++++++++++++++++++++++++++++++--
3 files changed, 46 insertions(+), 9 deletions(-)
[PATCH 0/5] i386/hvf: x2apic support and some small fixes
Posted by Phil Dennis-Jordan 2 weeks, 3 days ago
This is a loose collection of patches against the x86 hvf accel. They
can be applied/pulled independently from one another.

Patch 1 is a repost of a patch I've submitted a bunch of times already.
It wires up and enables x2APIC mode in conjunction with HVF - the
software APIC implementation in QEMU gained the feature earlier this
year but hvf wasn't included.
The change typically improves performance with modern SMP guest OSes by
a 2-digit percentage. (Exact values depend on workload.)

Patch 2 fixes cases of undefined behaviour recently introduced by commit
7cac7aa which made changes to HVF CPUID XSAVE functionality.

Patch 3 fixes a minor one-off memory leak during hvf startup.

Patch 4 ever so slightly improves APIC correctness under hvf: when
setting the APICBASE MSR, if the APIC deems the new value invalid,
we raise an exception (as per spec) rather than silently doing
nothing. This fixes a failing kvm-unit-tests test case.

Patch 5 removes some unnecessary duplication and type-rechecking in
HVF's inner loop. (No need to cast the cpu state pointer to X86CPU
within, the hvf_vcp_exec function already does that once at the top.)

Some of this work has been sponsored by Sauce Labs Inc.

Phil Dennis-Jordan (5):
  i386/hvf: Integrates x2APIC support with hvf accel
  i386/hvf: Fix for UB in handling CPUID function 0xD
  i386/hvf: Fixes startup memory leak (vmcs caps)
  i386/hvf: Raise exception on error setting APICBASE
  i386/hvf: Removes duplicate/shadowed variables in hvf_vcpu_exec

 target/i386/hvf/hvf.c       |  7 +++----
 target/i386/hvf/x86_cpuid.c |  6 +++---
 target/i386/hvf/x86_emu.c   | 42 +++++++++++++++++++++++++++++++++++--
 3 files changed, 46 insertions(+), 9 deletions(-)

-- 
2.39.3 (Apple Git-145)
Re: [PATCH 0/5] i386/hvf: x2apic support and some small fixes
Posted by Roman Bolshakov 2 weeks, 2 days ago
On Tue, Nov 05, 2024 at 04:57:55PM +0100, Phil Dennis-Jordan wrote:
> This is a loose collection of patches against the x86 hvf accel. They
> can be applied/pulled independently from one another.
> 
> Patch 1 is a repost of a patch I've submitted a bunch of times already.
> It wires up and enables x2APIC mode in conjunction with HVF - the
> software APIC implementation in QEMU gained the feature earlier this
> year but hvf wasn't included.
> The change typically improves performance with modern SMP guest OSes by
> a 2-digit percentage. (Exact values depend on workload.)
> 
> Patch 2 fixes cases of undefined behaviour recently introduced by commit
> 7cac7aa which made changes to HVF CPUID XSAVE functionality.
> 
> Patch 3 fixes a minor one-off memory leak during hvf startup.
> 
> Patch 4 ever so slightly improves APIC correctness under hvf: when
> setting the APICBASE MSR, if the APIC deems the new value invalid,
> we raise an exception (as per spec) rather than silently doing
> nothing. This fixes a failing kvm-unit-tests test case.
> 
> Patch 5 removes some unnecessary duplication and type-rechecking in
> HVF's inner loop. (No need to cast the cpu state pointer to X86CPU
> within, the hvf_vcp_exec function already does that once at the top.)
> 
> Some of this work has been sponsored by Sauce Labs Inc.
> 
> Phil Dennis-Jordan (5):
>   i386/hvf: Integrates x2APIC support with hvf accel
>   i386/hvf: Fix for UB in handling CPUID function 0xD
>   i386/hvf: Fixes startup memory leak (vmcs caps)
>   i386/hvf: Raise exception on error setting APICBASE
>   i386/hvf: Removes duplicate/shadowed variables in hvf_vcpu_exec
> 

To the series,
Reviewed-by: Roman Bolshakov <rbolshakov@ddn.com>
Tested-by: Roman Bolshakov <rbolshakov@ddn.com>

I figured the issue with 24.04 guests, it was an issue on my side (too
little memory provided to the guest).

Paolo, please apply this if you have no objections.

Regards,
Roman
Re: [PATCH 0/5] i386/hvf: x2apic support and some small fixes
Posted by Phil Dennis-Jordan 2 weeks, 2 days ago
Hi Roman,

Welcome back, and thanks for reviewing these patches!

On Wed, 6 Nov 2024 at 16:26, Roman Bolshakov <roman@roolebo.dev> wrote:

> On Tue, Nov 05, 2024 at 04:57:55PM +0100, Phil Dennis-Jordan wrote:
> > This is a loose collection of patches against the x86 hvf accel. They
> > can be applied/pulled independently from one another.
> >
> > Patch 1 is a repost of a patch I've submitted a bunch of times already.
> > It wires up and enables x2APIC mode in conjunction with HVF - the
> > software APIC implementation in QEMU gained the feature earlier this
> > year but hvf wasn't included.
> > The change typically improves performance with modern SMP guest OSes by
> > a 2-digit percentage. (Exact values depend on workload.)
> >
> > Patch 2 fixes cases of undefined behaviour recently introduced by commit
> > 7cac7aa which made changes to HVF CPUID XSAVE functionality.
> >
> > Patch 3 fixes a minor one-off memory leak during hvf startup.
> >
> > Patch 4 ever so slightly improves APIC correctness under hvf: when
> > setting the APICBASE MSR, if the APIC deems the new value invalid,
> > we raise an exception (as per spec) rather than silently doing
> > nothing. This fixes a failing kvm-unit-tests test case.
> >
> > Patch 5 removes some unnecessary duplication and type-rechecking in
> > HVF's inner loop. (No need to cast the cpu state pointer to X86CPU
> > within, the hvf_vcp_exec function already does that once at the top.)
> >
> > Some of this work has been sponsored by Sauce Labs Inc.
> >
> > Phil Dennis-Jordan (5):
> >   i386/hvf: Integrates x2APIC support with hvf accel
> >   i386/hvf: Fix for UB in handling CPUID function 0xD
> >   i386/hvf: Fixes startup memory leak (vmcs caps)
> >   i386/hvf: Raise exception on error setting APICBASE
> >   i386/hvf: Removes duplicate/shadowed variables in hvf_vcpu_exec
> >
>
> To the series,
> Reviewed-by: Roman Bolshakov <rbolshakov@ddn.com>
> Tested-by: Roman Bolshakov <rbolshakov@ddn.com>
>
> I figured the issue with 24.04 guests, it was an issue on my side (too
> little memory provided to the guest).
>

I'm glad it was that simple - I'm not aware of any incompatibilities, and I
did test with a bunch of guest OSes including a few Linux versions,
FreeBSD, Win10, and macOS.

Regarding your question about kvm-unit-tests: Last time I checked, the
upstream version of its APIC tests crashed Qemu on hvf (and I think also
TCG,) with or without any of the patches in this patch set applied. The
reason is that one of the test cases assumes it's running on a hypervisor
supporting a KVM-specific hypercall, i.e. it assumes it's running on KVM. I
tried submitting a patch that tests for support of that hypercall at
runtime, but it didn't get merged at the time:
https://marc.info/?l=kvm&m=169736751712535&w=2
I should update that to make sure it applies correctly to the latest
upstream and re-submit it, as those unit tests are actually extremely
useful for development & debugging.

If the patch no longer applies correctly, you can also just disable the
test_pv_ipi() test by whatever means you like.

Patch 1/5 obviously(?) allows the x2apic tests to run at all, while patch
4/5 fixes some of the failures in test_apicbase() and test_enable_x2apic().
There are still a significant number of test failures remaining, but these
equally apply to TCG and aren't specific to these hvf patches. (Of course
it would be nice to fix them, but I've been spending a lot of time and
energy on the PV Graphics and vmapple patch set… hopefully that will
eventually be merged and I might get a chance to get some of the other
small improvements I've got lying around into an upstreamable state.)

All the best,
Phil



> Paolo, please apply this if you have no objections.
>
> Regards,
> Roman
>