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

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

[Phil Dennis-Jordan]

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)

[PATCH 1/5] i386/hvf: Integrates x2APIC support with hvf accel

[Phil Dennis-Jordan]

Support for x2APIC mode was recently introduced in the software emulated
APIC implementation for TCG. Enabling it when using macOS’s hvf
accelerator is useful and significantly helps performance, as Qemu
currently uses the emulated APIC when running on hvf as well.

This change wires up the read & write operations for the MSR VM exits
and allow-lists the CPUID flag in the x86 hvf runtime.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---

v2:

 * Fixed merge conflict due to recent upstream CPUID code change.

 target/i386/hvf/x86_cpuid.c |  2 +-
 target/i386/hvf/x86_emu.c   | 31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/target/i386/hvf/x86_cpuid.c b/target/i386/hvf/x86_cpuid.c
index 3a116548a3d..ac922d7fd16 100644
--- a/target/i386/hvf/x86_cpuid.c
+++ b/target/i386/hvf/x86_cpuid.c
@@ -77,7 +77,7 @@ uint32_t hvf_get_supported_cpuid(uint32_t func, uint32_t idx,
         ecx &= CPUID_EXT_SSE3 | CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSSE3 |
              CPUID_EXT_FMA | CPUID_EXT_CX16 | CPUID_EXT_PCID |
              CPUID_EXT_SSE41 | CPUID_EXT_SSE42 | CPUID_EXT_MOVBE |
-             CPUID_EXT_POPCNT | CPUID_EXT_AES |
+             CPUID_EXT_POPCNT | CPUID_EXT_AES | CPUID_EXT_X2APIC |
              (supported_xcr0 ? CPUID_EXT_XSAVE : 0) |
              CPUID_EXT_AVX | CPUID_EXT_F16C | CPUID_EXT_RDRAND;
         ecx |= CPUID_EXT_HYPERVISOR;
diff --git a/target/i386/hvf/x86_emu.c b/target/i386/hvf/x86_emu.c
index 38c782b8e3b..be675bcfb71 100644
--- a/target/i386/hvf/x86_emu.c
+++ b/target/i386/hvf/x86_emu.c
@@ -663,6 +663,15 @@ static void exec_lods(CPUX86State *env, struct x86_decode *decode)
     env->eip += decode->len;
 }
 
+static void raise_exception(CPUX86State *env, int exception_index,
+                            int error_code)
+{
+    env->exception_nr = exception_index;
+    env->error_code = error_code;
+    env->has_error_code = true;
+    env->exception_injected = 1;
+}
+
 void simulate_rdmsr(CPUX86State *env)
 {
     X86CPU *cpu = env_archcpu(env);
@@ -677,6 +686,17 @@ void simulate_rdmsr(CPUX86State *env)
     case MSR_IA32_APICBASE:
         val = cpu_get_apic_base(cpu->apic_state);
         break;
+    case MSR_APIC_START ... MSR_APIC_END: {
+        int ret;
+        int index = (uint32_t)env->regs[R_ECX] - MSR_APIC_START;
+
+        ret = apic_msr_read(index, &val);
+        if (ret < 0) {
+            raise_exception(env, EXCP0D_GPF, 0);
+        }
+
+        break;
+    }
     case MSR_IA32_UCODE_REV:
         val = cpu->ucode_rev;
         break;
@@ -777,6 +797,17 @@ void simulate_wrmsr(CPUX86State *env)
     case MSR_IA32_APICBASE:
         cpu_set_apic_base(cpu->apic_state, data);
         break;
+    case MSR_APIC_START ... MSR_APIC_END: {
+        int ret;
+        int index = (uint32_t)env->regs[R_ECX] - MSR_APIC_START;
+
+        ret = apic_msr_write(index, data);
+        if (ret < 0) {
+            raise_exception(env, EXCP0D_GPF, 0);
+        }
+
+        break;
+    }
     case MSR_FSBASE:
         wvmcs(cs->accel->fd, VMCS_GUEST_FS_BASE, data);
         break;
-- 
2.39.3 (Apple Git-145)

Re: [PATCH 1/5] i386/hvf: Integrates x2APIC support with hvf accel

[Roman Bolshakov]

On Tue, Nov 05, 2024 at 04:57:56PM +0100, Phil Dennis-Jordan wrote:
> Support for x2APIC mode was recently introduced in the software emulated
> APIC implementation for TCG. Enabling it when using macOS´s hvf
> accelerator is useful and significantly helps performance, as Qemu
> currently uses the emulated APIC when running on hvf as well.
> 
> This change wires up the read & write operations for the MSR VM exits
> and allow-lists the CPUID flag in the x86 hvf runtime.
> 
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>

Reviewed-by: Roman Bolshakov <rbolshakov@ddn.com>

Hi Phil,

I don't observe any change in x2apic KVM unit tests with the patch.
I tried the single patch and the series, not looked into the test
failure yet.

Do you happen to know why?

Also what guest do you run? Desktop Ubuntu 24.04 doesn't seem to work
with x86 HVF.

Thanks,
Roman

[PATCH 2/5] i386/hvf: Fix for UB in handling CPUID function 0xD

[Phil Dennis-Jordan]

The handling for CPUID function 0xD (supported XSAVE features) was
improved in a recent patch. Unfortunately, this appears to have
introduced undefined behaviour for cases where ecx > 30, as the result
of (1 << idx) is undefined if idx > 30.

Per Intel SDM section 13.2, the behaviour for ecx values up to and
including 62 are specified. This change therefore specifically sets
all registers returned by the CPUID instruction to 0 for 63 and higher.
Furthermore, the bit shift uses uint64_t, where behaviour for the entire
range of 2..62 is safe and correct.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 target/i386/hvf/x86_cpuid.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/i386/hvf/x86_cpuid.c b/target/i386/hvf/x86_cpuid.c
index ac922d7fd16..9d9ccaa815d 100644
--- a/target/i386/hvf/x86_cpuid.c
+++ b/target/i386/hvf/x86_cpuid.c
@@ -119,8 +119,8 @@ uint32_t hvf_get_supported_cpuid(uint32_t func, uint32_t idx,
         eax = 0;
         break;
     case 0xD:
-        if (!supported_xcr0 ||
-            (idx > 1 && !(supported_xcr0 & (1 << idx)))) {
+        if (!supported_xcr0 || idx >= 63 ||
+            (idx > 1 && !(supported_xcr0 & (UINT64_C(1) << idx)))) {
             eax = ebx = ecx = edx = 0;
             break;
         }
-- 
2.39.3 (Apple Git-145)

Re: [PATCH 2/5] i386/hvf: Fix for UB in handling CPUID function 0xD

[Roman Bolshakov]

On Tue, Nov 05, 2024 at 04:57:57PM +0100, Phil Dennis-Jordan wrote:
> The handling for CPUID function 0xD (supported XSAVE features) was
> improved in a recent patch. Unfortunately, this appears to have
> introduced undefined behaviour for cases where ecx > 30, as the result
> of (1 << idx) is undefined if idx > 30.
> 
> Per Intel SDM section 13.2, the behaviour for ecx values up to and
> including 62 are specified. This change therefore specifically sets
> all registers returned by the CPUID instruction to 0 for 63 and higher.
> Furthermore, the bit shift uses uint64_t, where behaviour for the entire
> range of 2..62 is safe and correct.
> 

Thanks for correcting the regression.

Reviewed-by: Roman Bolshakov <rbolshakov@ddn.com>

[PATCH 3/5] i386/hvf: Fixes startup memory leak (vmcs caps)

[Phil Dennis-Jordan]

The hvf_caps data structure only exists once as part of the hvf accelerator
state, but it is initialised during vCPU initialisation. This change therefore
adds a check to ensure memory for it is only allocated once.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 target/i386/hvf/hvf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 68dc5d9cf75..8527bce6eef 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -239,7 +239,9 @@ int hvf_arch_init_vcpu(CPUState *cpu)
     init_emu();
     init_decoder();
 
-    hvf_state->hvf_caps = g_new0(struct hvf_vcpu_caps, 1);
+    if (hvf_state->hvf_caps == NULL) {
+        hvf_state->hvf_caps = g_new0(struct hvf_vcpu_caps, 1);
+    }
     env->hvf_mmio_buf = g_new(char, 4096);
 
     if (x86cpu->vmware_cpuid_freq) {
-- 
2.39.3 (Apple Git-145)

Re: [PATCH 3/5] i386/hvf: Fixes startup memory leak (vmcs caps)

[Roman Bolshakov]

On Tue, Nov 05, 2024 at 04:57:58PM +0100, Phil Dennis-Jordan wrote:
> The hvf_caps data structure only exists once as part of the hvf accelerator
> state, but it is initialised during vCPU initialisation. This change therefore
> adds a check to ensure memory for it is only allocated once.
> 

Looks good,

Reviewed-by: Roman Bolshakov <rbolshakov@ddn.com>

Regards,
Roman

[PATCH 4/5] i386/hvf: Raise exception on error setting APICBASE

[Phil Dennis-Jordan]

When setting the APICBASE MSR to an illegal value, the APIC
implementation will return an error. This change forwards that report
to the guest as an exception rather than ignoring it when using the hvf
accelerator.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 target/i386/hvf/x86_emu.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/target/i386/hvf/x86_emu.c b/target/i386/hvf/x86_emu.c
index be675bcfb71..015f760acb3 100644
--- a/target/i386/hvf/x86_emu.c
+++ b/target/i386/hvf/x86_emu.c
@@ -794,9 +794,16 @@ void simulate_wrmsr(CPUX86State *env)
     switch (msr) {
     case MSR_IA32_TSC:
         break;
-    case MSR_IA32_APICBASE:
-        cpu_set_apic_base(cpu->apic_state, data);
+    case MSR_IA32_APICBASE: {
+        int r;
+
+        r = cpu_set_apic_base(cpu->apic_state, data);
+        if (r < 0) {
+            raise_exception(env, EXCP0D_GPF, 0);
+        }
+
         break;
+    }
     case MSR_APIC_START ... MSR_APIC_END: {
         int ret;
         int index = (uint32_t)env->regs[R_ECX] - MSR_APIC_START;
-- 
2.39.3 (Apple Git-145)

Re: [PATCH 4/5] i386/hvf: Raise exception on error setting APICBASE

[Roman Bolshakov]

On Tue, Nov 05, 2024 at 04:57:59PM +0100, Phil Dennis-Jordan wrote:
> When setting the APICBASE MSR to an illegal value, the APIC
> implementation will return an error. This change forwards that report
> to the guest as an exception rather than ignoring it when using the hvf
> accelerator.
> 

Reviewed-by: Roman Bolshakov <rbolshakov@ddn.com>

Thanks,
Roman

[PATCH 5/5] i386/hvf: Removes duplicate/shadowed variables in hvf_vcpu_exec

[Phil Dennis-Jordan]

Pointers to the x86 CPU state already exist at the function scope,
no need to re-obtain them in individual exit reason cases.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 target/i386/hvf/hvf.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 8527bce6eef..c5d025d5576 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -586,8 +586,6 @@ int hvf_vcpu_exec(CPUState *cpu)
             break;
         }
         case EXIT_REASON_XSETBV: {
-            X86CPU *x86_cpu = X86_CPU(cpu);
-            CPUX86State *env = &x86_cpu->env;
             uint32_t eax = (uint32_t)rreg(cpu->accel->fd, HV_X86_RAX);
             uint32_t ecx = (uint32_t)rreg(cpu->accel->fd, HV_X86_RCX);
             uint32_t edx = (uint32_t)rreg(cpu->accel->fd, HV_X86_RDX);
@@ -644,7 +642,6 @@ int hvf_vcpu_exec(CPUState *cpu)
                 break;
             }
             case 8: {
-                X86CPU *x86_cpu = X86_CPU(cpu);
                 if (exit_qual & 0x10) {
                     RRX(env, reg) = cpu_get_apic_tpr(x86_cpu->apic_state);
                 } else {
-- 
2.39.3 (Apple Git-145)

Re: [PATCH 5/5] i386/hvf: Removes duplicate/shadowed variables in hvf_vcpu_exec

[Roman Bolshakov]

On Tue, Nov 05, 2024 at 04:58:00PM +0100, Phil Dennis-Jordan wrote:
> Pointers to the x86 CPU state already exist at the function scope,
> no need to re-obtain them in individual exit reason cases.
> 

Reviewed-by: Roman Bolshakov <rbolshakov@ddn.com>

Regards,
Roman

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

[Roman Bolshakov]

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

[Phil Dennis-Jordan]

[-- Attachment #1: Type: text/plain, Size: 3847 bytes --]

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
>

[-- Attachment #2: Type: text/html, Size: 4919 bytes --]