[v1] target/i386/hvf: hide MPX states from XCR0

[PATCH] target/i386/hvf: hide MPX states from XCR0

Posted by Paolo Bonzini 3 weeks, 4 days ago

QEMU does not show availability of MPX in CPUID when running under
Hypervisor.framework.  Therefore, in the unlikely chance that the host
has MPX enabled, hide those bits from leaf 0xD as well.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/hvf/x86_cpuid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/i386/hvf/x86_cpuid.c b/target/i386/hvf/x86_cpuid.c
index e56cd8411ba..4b184767f4a 100644
--- a/target/i386/hvf/x86_cpuid.c
+++ b/target/i386/hvf/x86_cpuid.c
@@ -110,9 +110,9 @@ uint32_t hvf_get_supported_cpuid(uint32_t func, uint32_t idx,
         if (idx == 0) {
             uint64_t host_xcr0;
             if (xgetbv(ecx, 0, &host_xcr0)) {
+                /* Only show xcr0 bits corresponding to usable features.  */
                 uint64_t supp_xcr0 = host_xcr0 & (XSTATE_FP_MASK |
                                   XSTATE_SSE_MASK | XSTATE_YMM_MASK |
-                                  XSTATE_BNDREGS_MASK | XSTATE_BNDCSR_MASK |
                                   XSTATE_OPMASK_MASK | XSTATE_ZMM_Hi256_MASK |
                                   XSTATE_Hi16_ZMM_MASK);
                 eax &= supp_xcr0;
-- 
2.47.0

Re: [PATCH] target/i386/hvf: hide MPX states from XCR0

Posted by Phil Dennis-Jordan 3 weeks, 3 days ago

On Tue, 29 Oct 2024 at 14:05, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> QEMU does not show availability of MPX in CPUID when running under
> Hypervisor.framework.  Therefore, in the unlikely chance that the host
> has MPX enabled, hide those bits from leaf 0xD as well.

To clarify: is there some kind of issue with MPX in Qemu in general?
Or is this a consistency effort - normal Macs don't expose this
feature, so we have no idea if it were to work if someone did manage
to hack up some frankensteinian host system that somehow does have
those bits set?


> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/hvf/x86_cpuid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/i386/hvf/x86_cpuid.c b/target/i386/hvf/x86_cpuid.c
> index e56cd8411ba..4b184767f4a 100644
> --- a/target/i386/hvf/x86_cpuid.c
> +++ b/target/i386/hvf/x86_cpuid.c
> @@ -110,9 +110,9 @@ uint32_t hvf_get_supported_cpuid(uint32_t func, uint32_t idx,
>          if (idx == 0) {
>              uint64_t host_xcr0;
>              if (xgetbv(ecx, 0, &host_xcr0)) {
> +                /* Only show xcr0 bits corresponding to usable features.  */
>                  uint64_t supp_xcr0 = host_xcr0 & (XSTATE_FP_MASK |
>                                    XSTATE_SSE_MASK | XSTATE_YMM_MASK |
> -                                  XSTATE_BNDREGS_MASK | XSTATE_BNDCSR_MASK |
>                                    XSTATE_OPMASK_MASK | XSTATE_ZMM_Hi256_MASK |
>                                    XSTATE_Hi16_ZMM_MASK);
>                  eax &= supp_xcr0;
> --
> 2.47.0
>
>

Re: [PATCH] target/i386/hvf: hide MPX states from XCR0

Posted by Paolo Bonzini 3 weeks, 3 days ago

On 10/30/24 13:30, Phil Dennis-Jordan wrote:
> On Tue, 29 Oct 2024 at 14:05, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> QEMU does not show availability of MPX in CPUID when running under
>> Hypervisor.framework.  Therefore, in the unlikely chance that the host
>> has MPX enabled, hide those bits from leaf 0xD as well.
> 
> To clarify: is there some kind of issue with MPX in Qemu in general?
> Or is this a consistency effort - normal Macs don't expose this
> feature, so we have no idea if it were to work if someone did manage
> to hack up some frankensteinian host system that somehow does have
> those bits set?

That, and also that real hardware will only show XSTATE_BNDREGS_MASK and 
XSTATE_BNDCSR_MASK if the MPX bit is set in CPUID; which it isn't in 
hvf_get_supported_cpuid().

In fact, for completeness it should also go the other way: if 
XSTATE_YMM_MASK is not set in the result of XGETBV, AVX should be 
hidden.  And if any of OPMASK, ZMM_Hi256 and Hi16_ZMM are not set in the 
result of XGETBV, AVX512F (and AVX10 eventually) should be hidden in 
hvf_get_supported_cpuid().

By the way, could you check if Macs set the PKRU bit of XCR0 (bit 9) 
and/or the OSPKE bit in CPUID (that's bit 4 of CPUID[EAX=7, ECX=0].ECX)?

Thanks,

Paolo

> 
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   target/i386/hvf/x86_cpuid.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/i386/hvf/x86_cpuid.c b/target/i386/hvf/x86_cpuid.c
>> index e56cd8411ba..4b184767f4a 100644
>> --- a/target/i386/hvf/x86_cpuid.c
>> +++ b/target/i386/hvf/x86_cpuid.c
>> @@ -110,9 +110,9 @@ uint32_t hvf_get_supported_cpuid(uint32_t func, uint32_t idx,
>>           if (idx == 0) {
>>               uint64_t host_xcr0;
>>               if (xgetbv(ecx, 0, &host_xcr0)) {
>> +                /* Only show xcr0 bits corresponding to usable features.  */
>>                   uint64_t supp_xcr0 = host_xcr0 & (XSTATE_FP_MASK |
>>                                     XSTATE_SSE_MASK | XSTATE_YMM_MASK |
>> -                                  XSTATE_BNDREGS_MASK | XSTATE_BNDCSR_MASK |
>>                                     XSTATE_OPMASK_MASK | XSTATE_ZMM_Hi256_MASK |
>>                                     XSTATE_Hi16_ZMM_MASK);
>>                   eax &= supp_xcr0;
>> --
>> 2.47.0
>>
>>
> 
>

Re: [PATCH] target/i386/hvf: hide MPX states from XCR0

Posted by Phil Dennis-Jordan 3 weeks, 2 days ago

On Wed, 30 Oct 2024 at 14:35, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 10/30/24 13:30, Phil Dennis-Jordan wrote:
> > On Tue, 29 Oct 2024 at 14:05, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>
> >> QEMU does not show availability of MPX in CPUID when running under
> >> Hypervisor.framework.  Therefore, in the unlikely chance that the host
> >> has MPX enabled, hide those bits from leaf 0xD as well.
> >
> > To clarify: is there some kind of issue with MPX in Qemu in general?
> > Or is this a consistency effort - normal Macs don't expose this
> > feature, so we have no idea if it were to work if someone did manage
> > to hack up some frankensteinian host system that somehow does have
> > those bits set?
>
> That, and also that real hardware will only show XSTATE_BNDREGS_MASK and
> XSTATE_BNDCSR_MASK if the MPX bit is set in CPUID; which it isn't in
> hvf_get_supported_cpuid().

Understood, and makes sense. Perhaps you could add this to the commit
message as well?

> In fact, for completeness it should also go the other way: if
> XSTATE_YMM_MASK is not set in the result of XGETBV, AVX should be
> hidden.  And if any of OPMASK, ZMM_Hi256 and Hi16_ZMM are not set in the
> result of XGETBV, AVX512F (and AVX10 eventually) should be hidden in
> hvf_get_supported_cpuid().

Indeed. macOS does also have AVX512 support, and I believe there were
two Mac models which shipped with Xeon CPUs that exposed some version
of the feature, the iMac Pro and the 2019 Mac Pro. I don't have access
to such a machine however, so I’m not sure exactly what the host
support entails. (I don't know if HVF guests theoretically could
support AVX512 features supported by the CPU but not the host OS as
long as it only uses register state correctly saved by the host OS?)

> By the way, could you check if Macs set the PKRU bit of XCR0 (bit 9)
> and/or the OSPKE bit in CPUID (that's bit 4 of CPUID[EAX=7, ECX=0].ECX)?

On a 2019 MacBook Pro 15" with a Coffee Lake Intel Core i9 9880H CPU,
the OSPKE bit is not set. (cpuid 7,0 returns: ebx = 0x029c67af, ecx =
0x40000000, edx = 0xbc000e00)

Assuming I'm doing this right, xcr0 on this machine appears to be 0x7,
so there's no PKRU bit set.

While trying to log xcr0, I noticed that xgetbv checks the passed ecx
value against CPUID_EXT_OSXSAVE. Except as far as I'm aware, OSXSAVE
is in cpuid function 1's ecx, whereas the code path in
hvf_get_supported_cpuid() is, by my reading, passing the ecx value for
cpuid function 0xD, index 0. The xgetbv call thus returns false on my
machine (ecx = 0x440), and eax is not modified. (stays 0x1f rather
than 0x7)

It seems like we ought to either fetch the CPUID_EXT_OSXSAVE bit in
another cpuid call, or cache it. Or do I have this wrong?

Thanks,
Phil