[PATCH] hvf: guard xgetbv call.

[PATCH] hvf: guard xgetbv call.

[Hill Ma]

This prevents illegal instruction on cpus do not support xgetbv.

Buglink: https://bugs.launchpad.net/qemu/+bug/1758819
Signed-off-by: Hill Ma <maahiuzeon@gmail.com>
---
 target/i386/hvf/x86_cpuid.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/target/i386/hvf/x86_cpuid.c b/target/i386/hvf/x86_cpuid.c
index a6842912f5..b4b7111fc3 100644
--- a/target/i386/hvf/x86_cpuid.c
+++ b/target/i386/hvf/x86_cpuid.c
@@ -100,11 +100,16 @@ uint32_t hvf_get_supported_cpuid(uint32_t func, uint32_t idx,
         break;
     case 0xD:
         if (idx == 0) {
-            uint64_t host_xcr0 = xgetbv(0);
-            uint64_t supp_xcr0 = host_xcr0 & (XSTATE_FP_MASK | XSTATE_SSE_MASK |
+            uint64_t supp_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);
+                                  XSTATE_ZMM_Hi256_MASK | XSTATE_Hi16_ZMM_MASK;
+            if ((ecx & CPUID_EXT_AVX) &&
+                (ecx & CPUID_EXT_XSAVE) &&
+                (ecx & CPUID_EXT_OSXSAVE)) {
+                uint64_t host_xcr0 = xgetbv(0);
+                supp_xcr0 &= host_xcr0;
+            }
             eax &= supp_xcr0;
         } else if (idx == 1) {
             hv_vmx_read_capability(HV_VMX_CAP_PROCBASED2, &cap);
-- 
2.20.1 (Apple Git-117)

Re: [PATCH] hvf: guard xgetbv call.

[Roman Bolshakov]

On Fri, Dec 18, 2020 at 06:13:47PM -0800, Hill Ma wrote:
> This prevents illegal instruction on cpus do not support xgetbv.
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1758819
> Signed-off-by: Hill Ma <maahiuzeon@gmail.com>
> ---
>  target/i386/hvf/x86_cpuid.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 

Hi Hill,

I'm sorry for delay with the review.

> diff --git a/target/i386/hvf/x86_cpuid.c b/target/i386/hvf/x86_cpuid.c
> index a6842912f5..b4b7111fc3 100644
> --- a/target/i386/hvf/x86_cpuid.c
> +++ b/target/i386/hvf/x86_cpuid.c
> @@ -100,11 +100,16 @@ uint32_t hvf_get_supported_cpuid(uint32_t func, uint32_t idx,
>          break;
>      case 0xD:
>          if (idx == 0) {
> -            uint64_t host_xcr0 = xgetbv(0);
> -            uint64_t supp_xcr0 = host_xcr0 & (XSTATE_FP_MASK | XSTATE_SSE_MASK |
> +            uint64_t supp_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);
> +                                  XSTATE_ZMM_Hi256_MASK | XSTATE_Hi16_ZMM_MASK;


> +            if ((ecx & CPUID_EXT_AVX) &&
> +                (ecx & CPUID_EXT_XSAVE) &&
> +                (ecx & CPUID_EXT_OSXSAVE)) {

It's sufficient to check only CPUID_EXT_OSXSAVE to ensure xgetbv
presence (per SDM Vol. 1 13-5):

  Software operating with CPL > 0 may need to determine whether the
  XSAVE feature set and certain XSAVE-enabled features have been
  enabled. If CPL > 0, execution of the MOV from CR4 instruction causes
  a general-protection fault (#GP). The following alternative mechanisms
  allow software to discover the enabling of the XSAVE feature set
  regardless of CPL:

  * The value of CR4.OSXSAVE is returned in CPUID.1:ECX.OSXSAVE[bit 27].
    If software determines that CPUID.1:ECX.OSXSAVE = 1, the processor
    supports the XSAVE feature set and the feature set has been enabled in
    CR4.

  * Executing the XGETBV instruction with ECX = 0 returns the value of
    XCR0 in EDX:EAX. XGETBV can be executed if CR4.OSXSAVE = 1 (if
    CPUID.1:ECX.OSXSAVE = 1), regardless of CPL.

> +                uint64_t host_xcr0 = xgetbv(0);
> +                supp_xcr0 &= host_xcr0;
> +            }
>              eax &= supp_xcr0;

I think instead of the patch you can do:
-          if (idx == 0) {
+          if (idx == 0 && (ecx & CPUID_EXT_OSXSAVE)) {

That'd keep host values returned from CPUID on platforms that don't
support XSAVE.

Thanks,
Roman

>          } else if (idx == 1) {
>              hv_vmx_read_capability(HV_VMX_CAP_PROCBASED2, &cap);
> -- 
> 2.20.1 (Apple Git-117)
> 

Re: [PATCH] hvf: guard xgetbv call.

[Peter Maydell]

On Sat, 9 Jan 2021 at 05:49, Roman Bolshakov <r.bolshakov@yadro.com> wrote:
>
> On Fri, Dec 18, 2020 at 06:13:47PM -0800, Hill Ma wrote:
> > This prevents illegal instruction on cpus do not support xgetbv.
> >
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1758819
> > Signed-off-by: Hill Ma <maahiuzeon@gmail.com>
> > ---
> >  target/i386/hvf/x86_cpuid.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
>
> Hi Hill,
>
> I'm sorry for delay with the review.

So, hvf added a third use of inline asm execution of "xgetbv" to
the two we had already. Now we have:
 * this in hvf
 * a use in tcg_target_init() in tcg/i386/tcg-target.c.inc
 * a use in init_cpuid_cache() in util/bufferiszero.c

Is it possible to abstract this out so we have one version
of this, not three ? I note that the other two got the "avoid
executing an illegal insn" tests right...

thanks
-- PMM

Re: [PATCH] hvf: guard xgetbv call.

[Roman Bolshakov]

On Sat, Jan 09, 2021 at 11:42:18AM +0000, Peter Maydell wrote:
> On Sat, 9 Jan 2021 at 05:49, Roman Bolshakov <r.bolshakov@yadro.com> wrote:
> >
> > On Fri, Dec 18, 2020 at 06:13:47PM -0800, Hill Ma wrote:
> > > This prevents illegal instruction on cpus do not support xgetbv.
> > >
> > > Buglink: https://bugs.launchpad.net/qemu/+bug/1758819
> > > Signed-off-by: Hill Ma <maahiuzeon@gmail.com>
> > > ---
> > >  target/i386/hvf/x86_cpuid.c | 11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > >
> >
> > Hi Hill,
> >
> > I'm sorry for delay with the review.
> 
> So, hvf added a third use of inline asm execution of "xgetbv" to
> the two we had already. Now we have:
>  * this in hvf
>  * a use in tcg_target_init() in tcg/i386/tcg-target.c.inc
>  * a use in init_cpuid_cache() in util/bufferiszero.c
> 
> Is it possible to abstract this out so we have one version
> of this, not three ? I note that the other two got the "avoid
> executing an illegal insn" tests right...

It surely is. If xgetbv() is extended like below and moved out of hvf,
we can reuse it in all other places and no duplication of #UD avoidance
will happen.

diff --git a/target/i386/hvf/x86_cpuid.c b/target/i386/hvf/x86_cpuid.c
index a6842912f5..7994f92d96 100644
--- a/target/i386/hvf/x86_cpuid.c
+++ b/target/i386/hvf/x86_cpuid.c
@@ -27,15 +27,21 @@
 #include "vmx.h"
 #include "sysemu/hvf.h"

-static uint64_t xgetbv(uint32_t xcr)
+static int xgetbv(uint32_t cpuid_ecx, uint32_t idx, uint64_t *xcr)
 {
-    uint32_t eax, edx;
+    uint32_t xcrl, xcrh;

-    __asm__ volatile ("xgetbv"
-                      : "=a" (eax), "=d" (edx)
-                      : "c" (xcr));
+    if (cpuid_ecx && CPUID_EXT_OSXSAVE) {
+        /* The xgetbv instruction is not available to older versions of
+         * the assembler, so we encode the instruction manually.
+         */
+        asm(".byte 0x0f, 0x01, 0xd0" : "=a" (xcrl), "=d" (xcrh) : "c" (idx));

-    return (((uint64_t)edx) << 32) | eax;
+        *xcr = (((uint64_t)xcrh) << 32) | xcrl;
+        return 0;
+    }
+
+    return 1;
 }

 uint32_t hvf_get_supported_cpuid(uint32_t func, uint32_t idx,


Hill, feel free to update the three places Peter mentioned.  If it's
more convenient for you I can make complete patch.

Thanks,
Roman

> 
> thanks
> -- PMM

Re: [PATCH] hvf: guard xgetbv call.

[Richard Henderson]

On 1/9/21 3:46 PM, Roman Bolshakov wrote:
> +static int xgetbv(uint32_t cpuid_ecx, uint32_t idx, uint64_t *xcr)
>  {
> -    uint32_t eax, edx;
> +    uint32_t xcrl, xcrh;
> 
> -    __asm__ volatile ("xgetbv"
> -                      : "=a" (eax), "=d" (edx)
> -                      : "c" (xcr));
> +    if (cpuid_ecx && CPUID_EXT_OSXSAVE) {
> +        /* The xgetbv instruction is not available to older versions of
> +         * the assembler, so we encode the instruction manually.
> +         */
> +        asm(".byte 0x0f, 0x01, 0xd0" : "=a" (xcrl), "=d" (xcrh) : "c" (idx));
> 
> -    return (((uint64_t)edx) << 32) | eax;
> +        *xcr = (((uint64_t)xcrh) << 32) | xcrl;
> +        return 0;
> +    }
> +
> +    return 1;
>  }

Not to bikeshed too much, but this looks like it should return bool, and true
on success, not the other way around.


r~

Re: [PATCH] hvf: guard xgetbv call.

[Richard Henderson]

On 1/10/21 8:34 AM, Richard Henderson wrote:
> On 1/9/21 3:46 PM, Roman Bolshakov wrote:
>> +static int xgetbv(uint32_t cpuid_ecx, uint32_t idx, uint64_t *xcr)
>>  {
>> -    uint32_t eax, edx;
>> +    uint32_t xcrl, xcrh;
>>
>> -    __asm__ volatile ("xgetbv"
>> -                      : "=a" (eax), "=d" (edx)
>> -                      : "c" (xcr));
>> +    if (cpuid_ecx && CPUID_EXT_OSXSAVE) {
>> +        /* The xgetbv instruction is not available to older versions of
>> +         * the assembler, so we encode the instruction manually.
>> +         */
>> +        asm(".byte 0x0f, 0x01, 0xd0" : "=a" (xcrl), "=d" (xcrh) : "c" (idx));
>>
>> -    return (((uint64_t)edx) << 32) | eax;
>> +        *xcr = (((uint64_t)xcrh) << 32) | xcrl;
>> +        return 0;
>> +    }
>> +
>> +    return 1;
>>  }
> 
> Not to bikeshed too much, but this looks like it should return bool, and true
> on success, not the other way around.

Also, if we're going to put this some place common, forcing the caller to do
the cpuid that feeds this, then we should probably make all of the startup
cpuid stuff common as well.

Note that we'd probably have to use constructor priorities to get that right
for util/bufferiszero.c.


r~

Re: [PATCH] hvf: guard xgetbv call.

[Roman Bolshakov]

On Sun, Jan 10, 2021 at 08:38:36AM -1000, Richard Henderson wrote:
> On 1/10/21 8:34 AM, Richard Henderson wrote:
> > On 1/9/21 3:46 PM, Roman Bolshakov wrote:
> >> +static int xgetbv(uint32_t cpuid_ecx, uint32_t idx, uint64_t *xcr)
> >>  {
> >> -    uint32_t eax, edx;
> >> +    uint32_t xcrl, xcrh;
> >>
> >> -    __asm__ volatile ("xgetbv"
> >> -                      : "=a" (eax), "=d" (edx)
> >> -                      : "c" (xcr));
> >> +    if (cpuid_ecx && CPUID_EXT_OSXSAVE) {
> >> +        /* The xgetbv instruction is not available to older versions of
> >> +         * the assembler, so we encode the instruction manually.
> >> +         */
> >> +        asm(".byte 0x0f, 0x01, 0xd0" : "=a" (xcrl), "=d" (xcrh) : "c" (idx));
> >>
> >> -    return (((uint64_t)edx) << 32) | eax;
> >> +        *xcr = (((uint64_t)xcrh) << 32) | xcrl;
> >> +        return 0;
> >> +    }
> >> +
> >> +    return 1;
> >>  }
> > 
> > Not to bikeshed too much, but this looks like it should return bool, and true
> > on success, not the other way around.
> 

I agree, it'd better to comprehend (and Hill has already sent v2 with
this).

> Also, if we're going to put this some place common, forcing the caller to do
> the cpuid that feeds this, then we should probably make all of the startup
> cpuid stuff common as well.
> 

I proposed the version because all callers of xgetbv instruction already
call cpuid before invoking inline xgetbv.

> Note that we'd probably have to use constructor priorities to get that right
> for util/bufferiszero.c.
> 

Please correct me if I read this wrong. What you're saying is we should
initialize cpuid in constructors and then use cached cpuid ecx in
xgetbv() (and drop one argument, respectively)?

Thanks,
Roman

Re: [PATCH] hvf: guard xgetbv call.

[Richard Henderson]

On 1/10/21 6:31 PM, Roman Bolshakov wrote:
>> Also, if we're going to put this some place common, forcing the caller to do
>> the cpuid that feeds this, then we should probably make all of the startup
>> cpuid stuff common as well.
>>
> 
> I proposed the version because all callers of xgetbv instruction already
> call cpuid before invoking inline xgetbv.
> 
>> Note that we'd probably have to use constructor priorities to get that right
>> for util/bufferiszero.c.
>>
> 
> Please correct me if I read this wrong. What you're saying is we should
> initialize cpuid in constructors and then use cached cpuid ecx in
> xgetbv() (and drop one argument, respectively)?

I would have xgetbv and all of the cpuid values cached at startup, and all
three places would read global variables.


r~

Re: [PATCH] hvf: guard xgetbv call.

[Roman Bolshakov]

On Mon, Jan 11, 2021 at 07:06:22AM -1000, Richard Henderson wrote:
> On 1/10/21 6:31 PM, Roman Bolshakov wrote:
> >> Also, if we're going to put this some place common, forcing the caller to do
> >> the cpuid that feeds this, then we should probably make all of the startup
> >> cpuid stuff common as well.
> >>
> > 
> > I proposed the version because all callers of xgetbv instruction already
> > call cpuid before invoking inline xgetbv.
> > 
> >> Note that we'd probably have to use constructor priorities to get that right
> >> for util/bufferiszero.c.
> >>
> > 
> > Please correct me if I read this wrong. What you're saying is we should
> > initialize cpuid in constructors and then use cached cpuid ecx in
> > xgetbv() (and drop one argument, respectively)?
> 
> I would have xgetbv and all of the cpuid values cached at startup, and all
> three places would read global variables.
> 

It makes sense, thanks for the clarification.

Do you think it should be part of the change Hill is doing or it might
be a follow-up cleanup patch?

-Roman

Re: [PATCH] hvf: guard xgetbv call.

[Richard Henderson]

On 1/11/21 9:49 PM, Roman Bolshakov wrote:
>> I would have xgetbv and all of the cpuid values cached at startup, and all
>> three places would read global variables.
>>
> 
> It makes sense, thanks for the clarification.
> 
> Do you think it should be part of the change Hill is doing or it might
> be a follow-up cleanup patch?

Could be follow-up, yes.


r~