Re: [PATCH] hvf: guard xgetbv call.

[Roman Bolshakov <r.bolshakov@yadro.com>]

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