[PATCH] x86: always supply .cpuid() handler to x86_emulate()

[PATCH] x86: always supply .cpuid() handler to x86_emulate()

[Jan Beulich]

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

With us incremementally adding proper CPUID checks to x86_emulate()
(see commit de05bd965a ["x86emul: correct {,F}CMOV and F{,U}COMI{,P}
emulation"]) it is no longer appropriate to invoke the function with
that hook being NULL, as long as respective instructions may get used
in that case.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I've noticed the problem first with a not yet submitted patch checking
cx8 (PV 32-bit kernels occasionally use cmpxchg8b for page table
accesses), and I think we don't strictly need the change here for 4.8,
but then again it may be better to handle this properly right away.

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1542,7 +1542,7 @@ static int hvmemul_wbinvd(
     return X86EMUL_OKAY;
 }
 
-static int hvmemul_cpuid(
+int hvmemul_cpuid(
     unsigned int *eax,
     unsigned int *ebx,
     unsigned int *ecx,
@@ -1892,11 +1892,13 @@ int hvm_emulate_one_mmio(unsigned long m
         .read       = x86emul_unhandleable_rw,
         .insn_fetch = hvmemul_insn_fetch,
         .write      = mmcfg_intercept_write,
+        .cpuid      = hvmemul_cpuid,
     };
     static const struct x86_emulate_ops hvm_ro_emulate_ops_mmio = {
         .read       = x86emul_unhandleable_rw,
         .insn_fetch = hvmemul_insn_fetch,
-        .write      = mmio_ro_emulated_write
+        .write      = mmio_ro_emulated_write,
+        .cpuid      = hvmemul_cpuid,
     };
     struct mmio_ro_emulate_ctxt mmio_ro_ctxt = { .cr2 = gla };
     struct hvm_emulate_ctxt ctxt;
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5327,6 +5327,7 @@ static const struct x86_emulate_ops ptwr
     .insn_fetch = ptwr_emulated_read,
     .write      = ptwr_emulated_write,
     .cmpxchg    = ptwr_emulated_cmpxchg,
+    .cpuid      = pv_emul_cpuid,
 };
 
 /* Write page fault handler: check if guest is trying to modify a PTE. */
@@ -5414,6 +5415,7 @@ static const struct x86_emulate_ops mmio
     .read       = x86emul_unhandleable_rw,
     .insn_fetch = ptwr_emulated_read,
     .write      = mmio_ro_emulated_write,
+    .cpuid      = pv_emul_cpuid,
 };
 
 int mmcfg_intercept_write(
@@ -5451,6 +5453,7 @@ static const struct x86_emulate_ops mmcf
     .read       = x86emul_unhandleable_rw,
     .insn_fetch = ptwr_emulated_read,
     .write      = mmcfg_intercept_write,
+    .cpuid      = pv_emul_cpuid,
 };
 
 /* Check if guest is trying to modify a r/o MMIO page. */
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -306,6 +306,7 @@ static const struct x86_emulate_ops hvm_
     .insn_fetch = hvm_emulate_insn_fetch,
     .write      = hvm_emulate_write,
     .cmpxchg    = hvm_emulate_cmpxchg,
+    .cpuid      = hvmemul_cpuid,
 };
 
 static int
@@ -374,6 +375,7 @@ static const struct x86_emulate_ops pv_s
     .insn_fetch = pv_emulate_read,
     .write      = pv_emulate_write,
     .cmpxchg    = pv_emulate_cmpxchg,
+    .cpuid      = pv_emul_cpuid,
 };
 
 const struct x86_emulate_ops *shadow_init_emulation(
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2755,6 +2755,24 @@ static int priv_op_write_msr(unsigned in
     return X86EMUL_UNHANDLEABLE;
 }
 
+int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
+                  unsigned int *edx, struct x86_emulate_ctxt *ctxt)
+{
+    struct cpu_user_regs regs = *ctxt->regs;
+
+    regs._eax = *eax;
+    regs._ecx = *ecx;
+
+    pv_cpuid(&regs);
+
+    *eax = regs._eax;
+    *ebx = regs._ebx;
+    *ecx = regs._ecx;
+    *edx = regs._edx;
+
+    return X86EMUL_OKAY;
+}
+
 /* Instruction fetch with error handling. */
 #define insn_fetch(type, base, eip, limit)                                  \
 ({  unsigned long _rc, _ptr = (base) + (eip);                               \
--- a/xen/include/asm-x86/hvm/emulate.h
+++ b/xen/include/asm-x86/hvm/emulate.h
@@ -60,6 +60,12 @@ void hvm_emulate_init(
     unsigned int insn_bytes);
 void hvm_emulate_writeback(
     struct hvm_emulate_ctxt *hvmemul_ctxt);
+int hvmemul_cpuid(
+    unsigned int *eax,
+    unsigned int *ebx,
+    unsigned int *ecx,
+    unsigned int *edx,
+    struct x86_emulate_ctxt *ctxt);
 struct segment_register *hvmemul_get_seg_reg(
     enum x86_segment seg,
     struct hvm_emulate_ctxt *hvmemul_ctxt);
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -504,6 +504,8 @@ extern int mmcfg_intercept_write(enum x8
                                  void *p_data,
                                  unsigned int bytes,
                                  struct x86_emulate_ctxt *ctxt);
+int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
+                  unsigned int *edx, struct x86_emulate_ctxt *ctxt);
 
 int  ptwr_do_page_fault(struct vcpu *, unsigned long,
                         struct cpu_user_regs *);



[-- Attachment #2: x86-emul-ops-CPUID.patch --]
[-- Type: text/plain, Size: 4986 bytes --]

x86: always supply .cpuid() handler to x86_emulate()

With us incremementally adding proper CPUID checks to x86_emulate()
(see commit de05bd965a ["x86emul: correct {,F}CMOV and F{,U}COMI{,P}
emulation"]) it is no longer appropriate to invoke the function with
that hook being NULL, as long as respective instructions may get used
in that case.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I've noticed the problem first with a not yet submitted patch checking
cx8 (PV 32-bit kernels occasionally use cmpxchg8b for page table
accesses), and I think we don't strictly need the change here for 4.8,
but then again it may be better to handle this properly right away.

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1542,7 +1542,7 @@ static int hvmemul_wbinvd(
     return X86EMUL_OKAY;
 }
 
-static int hvmemul_cpuid(
+int hvmemul_cpuid(
     unsigned int *eax,
     unsigned int *ebx,
     unsigned int *ecx,
@@ -1892,11 +1892,13 @@ int hvm_emulate_one_mmio(unsigned long m
         .read       = x86emul_unhandleable_rw,
         .insn_fetch = hvmemul_insn_fetch,
         .write      = mmcfg_intercept_write,
+        .cpuid      = hvmemul_cpuid,
     };
     static const struct x86_emulate_ops hvm_ro_emulate_ops_mmio = {
         .read       = x86emul_unhandleable_rw,
         .insn_fetch = hvmemul_insn_fetch,
-        .write      = mmio_ro_emulated_write
+        .write      = mmio_ro_emulated_write,
+        .cpuid      = hvmemul_cpuid,
     };
     struct mmio_ro_emulate_ctxt mmio_ro_ctxt = { .cr2 = gla };
     struct hvm_emulate_ctxt ctxt;
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5327,6 +5327,7 @@ static const struct x86_emulate_ops ptwr
     .insn_fetch = ptwr_emulated_read,
     .write      = ptwr_emulated_write,
     .cmpxchg    = ptwr_emulated_cmpxchg,
+    .cpuid      = pv_emul_cpuid,
 };
 
 /* Write page fault handler: check if guest is trying to modify a PTE. */
@@ -5414,6 +5415,7 @@ static const struct x86_emulate_ops mmio
     .read       = x86emul_unhandleable_rw,
     .insn_fetch = ptwr_emulated_read,
     .write      = mmio_ro_emulated_write,
+    .cpuid      = pv_emul_cpuid,
 };
 
 int mmcfg_intercept_write(
@@ -5451,6 +5453,7 @@ static const struct x86_emulate_ops mmcf
     .read       = x86emul_unhandleable_rw,
     .insn_fetch = ptwr_emulated_read,
     .write      = mmcfg_intercept_write,
+    .cpuid      = pv_emul_cpuid,
 };
 
 /* Check if guest is trying to modify a r/o MMIO page. */
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -306,6 +306,7 @@ static const struct x86_emulate_ops hvm_
     .insn_fetch = hvm_emulate_insn_fetch,
     .write      = hvm_emulate_write,
     .cmpxchg    = hvm_emulate_cmpxchg,
+    .cpuid      = hvmemul_cpuid,
 };
 
 static int
@@ -374,6 +375,7 @@ static const struct x86_emulate_ops pv_s
     .insn_fetch = pv_emulate_read,
     .write      = pv_emulate_write,
     .cmpxchg    = pv_emulate_cmpxchg,
+    .cpuid      = pv_emul_cpuid,
 };
 
 const struct x86_emulate_ops *shadow_init_emulation(
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2755,6 +2755,24 @@ static int priv_op_write_msr(unsigned in
     return X86EMUL_UNHANDLEABLE;
 }
 
+int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
+                  unsigned int *edx, struct x86_emulate_ctxt *ctxt)
+{
+    struct cpu_user_regs regs = *ctxt->regs;
+
+    regs._eax = *eax;
+    regs._ecx = *ecx;
+
+    pv_cpuid(&regs);
+
+    *eax = regs._eax;
+    *ebx = regs._ebx;
+    *ecx = regs._ecx;
+    *edx = regs._edx;
+
+    return X86EMUL_OKAY;
+}
+
 /* Instruction fetch with error handling. */
 #define insn_fetch(type, base, eip, limit)                                  \
 ({  unsigned long _rc, _ptr = (base) + (eip);                               \
--- a/xen/include/asm-x86/hvm/emulate.h
+++ b/xen/include/asm-x86/hvm/emulate.h
@@ -60,6 +60,12 @@ void hvm_emulate_init(
     unsigned int insn_bytes);
 void hvm_emulate_writeback(
     struct hvm_emulate_ctxt *hvmemul_ctxt);
+int hvmemul_cpuid(
+    unsigned int *eax,
+    unsigned int *ebx,
+    unsigned int *ecx,
+    unsigned int *edx,
+    struct x86_emulate_ctxt *ctxt);
 struct segment_register *hvmemul_get_seg_reg(
     enum x86_segment seg,
     struct hvm_emulate_ctxt *hvmemul_ctxt);
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -504,6 +504,8 @@ extern int mmcfg_intercept_write(enum x8
                                  void *p_data,
                                  unsigned int bytes,
                                  struct x86_emulate_ctxt *ctxt);
+int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
+                  unsigned int *edx, struct x86_emulate_ctxt *ctxt);
 
 int  ptwr_do_page_fault(struct vcpu *, unsigned long,
                         struct cpu_user_regs *);

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86: always supply .cpuid() handler to x86_emulate()

[Wei Liu]

On Thu, Nov 10, 2016 at 05:30:25AM -0700, Jan Beulich wrote:
> With us incremementally adding proper CPUID checks to x86_emulate()
> (see commit de05bd965a ["x86emul: correct {,F}CMOV and F{,U}COMI{,P}
> emulation"]) it is no longer appropriate to invoke the function with
> that hook being NULL, as long as respective instructions may get used
> in that case.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I've noticed the problem first with a not yet submitted patch checking
> cx8 (PV 32-bit kernels occasionally use cmpxchg8b for page table
> accesses), and I think we don't strictly need the change here for 4.8,
> but then again it may be better to handle this properly right away.

I'm fine with having this if we have some free cycles. If it is reviewed
today, feel free to commit it. Otherwise I will try to sneak it in after
it is reviewed.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86: always supply .cpuid() handler to x86_emulate()

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 10 November 2016 12:30
> To: xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Tim (Xen.org)
> <tim@xen.org>
> Subject: [PATCH] x86: always supply .cpuid() handler to x86_emulate()
> 
> With us incremementally adding proper CPUID checks to x86_emulate()
> (see commit de05bd965a ["x86emul: correct {,F}CMOV and F{,U}COMI{,P}
> emulation"]) it is no longer appropriate to invoke the function with
> that hook being NULL, as long as respective instructions may get used
> in that case.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

This all looks sensible to me and I'm kind of surprised we got away without it before.

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> ---
> I've noticed the problem first with a not yet submitted patch checking
> cx8 (PV 32-bit kernels occasionally use cmpxchg8b for page table
> accesses), and I think we don't strictly need the change here for 4.8,
> but then again it may be better to handle this properly right away.
> 
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1542,7 +1542,7 @@ static int hvmemul_wbinvd(
>      return X86EMUL_OKAY;
>  }
> 
> -static int hvmemul_cpuid(
> +int hvmemul_cpuid(
>      unsigned int *eax,
>      unsigned int *ebx,
>      unsigned int *ecx,
> @@ -1892,11 +1892,13 @@ int hvm_emulate_one_mmio(unsigned long m
>          .read       = x86emul_unhandleable_rw,
>          .insn_fetch = hvmemul_insn_fetch,
>          .write      = mmcfg_intercept_write,
> +        .cpuid      = hvmemul_cpuid,
>      };
>      static const struct x86_emulate_ops hvm_ro_emulate_ops_mmio = {
>          .read       = x86emul_unhandleable_rw,
>          .insn_fetch = hvmemul_insn_fetch,
> -        .write      = mmio_ro_emulated_write
> +        .write      = mmio_ro_emulated_write,
> +        .cpuid      = hvmemul_cpuid,
>      };
>      struct mmio_ro_emulate_ctxt mmio_ro_ctxt = { .cr2 = gla };
>      struct hvm_emulate_ctxt ctxt;
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5327,6 +5327,7 @@ static const struct x86_emulate_ops ptwr
>      .insn_fetch = ptwr_emulated_read,
>      .write      = ptwr_emulated_write,
>      .cmpxchg    = ptwr_emulated_cmpxchg,
> +    .cpuid      = pv_emul_cpuid,
>  };
> 
>  /* Write page fault handler: check if guest is trying to modify a PTE. */
> @@ -5414,6 +5415,7 @@ static const struct x86_emulate_ops mmio
>      .read       = x86emul_unhandleable_rw,
>      .insn_fetch = ptwr_emulated_read,
>      .write      = mmio_ro_emulated_write,
> +    .cpuid      = pv_emul_cpuid,
>  };
> 
>  int mmcfg_intercept_write(
> @@ -5451,6 +5453,7 @@ static const struct x86_emulate_ops mmcf
>      .read       = x86emul_unhandleable_rw,
>      .insn_fetch = ptwr_emulated_read,
>      .write      = mmcfg_intercept_write,
> +    .cpuid      = pv_emul_cpuid,
>  };
> 
>  /* Check if guest is trying to modify a r/o MMIO page. */
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -306,6 +306,7 @@ static const struct x86_emulate_ops hvm_
>      .insn_fetch = hvm_emulate_insn_fetch,
>      .write      = hvm_emulate_write,
>      .cmpxchg    = hvm_emulate_cmpxchg,
> +    .cpuid      = hvmemul_cpuid,
>  };
> 
>  static int
> @@ -374,6 +375,7 @@ static const struct x86_emulate_ops pv_s
>      .insn_fetch = pv_emulate_read,
>      .write      = pv_emulate_write,
>      .cmpxchg    = pv_emulate_cmpxchg,
> +    .cpuid      = pv_emul_cpuid,
>  };
> 
>  const struct x86_emulate_ops *shadow_init_emulation(
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -2755,6 +2755,24 @@ static int priv_op_write_msr(unsigned in
>      return X86EMUL_UNHANDLEABLE;
>  }
> 
> +int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
> +                  unsigned int *edx, struct x86_emulate_ctxt *ctxt)
> +{
> +    struct cpu_user_regs regs = *ctxt->regs;
> +
> +    regs._eax = *eax;
> +    regs._ecx = *ecx;
> +
> +    pv_cpuid(&regs);
> +
> +    *eax = regs._eax;
> +    *ebx = regs._ebx;
> +    *ecx = regs._ecx;
> +    *edx = regs._edx;
> +
> +    return X86EMUL_OKAY;
> +}
> +
>  /* Instruction fetch with error handling. */
>  #define insn_fetch(type, base, eip, limit)                                  \
>  ({  unsigned long _rc, _ptr = (base) + (eip);                               \
> --- a/xen/include/asm-x86/hvm/emulate.h
> +++ b/xen/include/asm-x86/hvm/emulate.h
> @@ -60,6 +60,12 @@ void hvm_emulate_init(
>      unsigned int insn_bytes);
>  void hvm_emulate_writeback(
>      struct hvm_emulate_ctxt *hvmemul_ctxt);
> +int hvmemul_cpuid(
> +    unsigned int *eax,
> +    unsigned int *ebx,
> +    unsigned int *ecx,
> +    unsigned int *edx,
> +    struct x86_emulate_ctxt *ctxt);
>  struct segment_register *hvmemul_get_seg_reg(
>      enum x86_segment seg,
>      struct hvm_emulate_ctxt *hvmemul_ctxt);
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -504,6 +504,8 @@ extern int mmcfg_intercept_write(enum x8
>                                   void *p_data,
>                                   unsigned int bytes,
>                                   struct x86_emulate_ctxt *ctxt);
> +int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
> +                  unsigned int *edx, struct x86_emulate_ctxt *ctxt);
> 
>  int  ptwr_do_page_fault(struct vcpu *, unsigned long,
>                          struct cpu_user_regs *);
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86: always supply .cpuid() handler to x86_emulate()

[Andrew Cooper]

On 10/11/16 12:30, Jan Beulich wrote:
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -2755,6 +2755,24 @@ static int priv_op_write_msr(unsigned in
>      return X86EMUL_UNHANDLEABLE;
>  }
>  
> +int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
> +                  unsigned int *edx, struct x86_emulate_ctxt *ctxt)
> +{
> +    struct cpu_user_regs regs = *ctxt->regs;

You need a CPUID faulting check here, matching the hvm side of things,
or you will leave a latent bug which gets exposed when switching to
using full x86_emulate() for PV guests.

Would it be wise to add a fail_if(!ops->cpuid) to x86_emulate() to catch
other misuses?

~Andrew

> +
> +    regs._eax = *eax;
> +    regs._ecx = *ecx;
> +
> +    pv_cpuid(&regs);
> +
> +    *eax = regs._eax;
> +    *ebx = regs._ebx;
> +    *ecx = regs._ecx;
> +    *edx = regs._edx;
> +
> +    return X86EMUL_OKAY;
> +}
> +
>  /* Instruction fetch with error handling. */
>  #define insn_fetch(type, base, eip, limit)                                  \
>  ({  unsigned long _rc, _ptr = (base) + (eip);                               \


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86: always supply .cpuid() handler to x86_emulate()

[Jan Beulich]

>>> On 11.11.16 at 15:16, <andrew.cooper3@citrix.com> wrote:
> On 10/11/16 12:30, Jan Beulich wrote:
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -2755,6 +2755,24 @@ static int priv_op_write_msr(unsigned in
>>      return X86EMUL_UNHANDLEABLE;
>>  }
>>  
>> +int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
>> +                  unsigned int *edx, struct x86_emulate_ctxt *ctxt)
>> +{
>> +    struct cpu_user_regs regs = *ctxt->regs;
> 
> You need a CPUID faulting check here, matching the hvm side of things,
> or you will leave a latent bug which gets exposed when switching to
> using full x86_emulate() for PV guests.

That addition is part of the patch actually switching to x86_emulate().
I don't think it belongs here, as for now we don't mean to emulate
CPUID insns.

> Would it be wise to add a fail_if(!ops->cpuid) to x86_emulate() to catch
> other misuses?

I don't think so - there may be cases where it's indeed unneeded
(and I foresee such arising quickly once the ->validate() hooks is
in place, addition of which is now too part of aforementioned patch).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86: always supply .cpuid() handler to x86_emulate()

[Andrew Cooper]

On 11/11/16 14:58, Jan Beulich wrote:
>>>> On 11.11.16 at 15:16, <andrew.cooper3@citrix.com> wrote:
>> On 10/11/16 12:30, Jan Beulich wrote:
>>> --- a/xen/arch/x86/traps.c
>>> +++ b/xen/arch/x86/traps.c
>>> @@ -2755,6 +2755,24 @@ static int priv_op_write_msr(unsigned in
>>>      return X86EMUL_UNHANDLEABLE;
>>>  }
>>>  
>>> +int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
>>> +                  unsigned int *edx, struct x86_emulate_ctxt *ctxt)
>>> +{
>>> +    struct cpu_user_regs regs = *ctxt->regs;
>> You need a CPUID faulting check here, matching the hvm side of things,
>> or you will leave a latent bug which gets exposed when switching to
>> using full x86_emulate() for PV guests.
> That addition is part of the patch actually switching to x86_emulate().
> I don't think it belongs here, as for now we don't mean to emulate
> CPUID insns.

Fine, so long as it doesn't get forgotten.

>
>> Would it be wise to add a fail_if(!ops->cpuid) to x86_emulate() to catch
>> other misuses?
> I don't think so - there may be cases where it's indeed unneeded
> (and I foresee such arising quickly once the ->validate() hooks is
> in place, addition of which is now too part of aforementioned patch).

Ok.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86: always supply .cpuid() handler to x86_emulate()

[Jan Beulich]

>>> On 11.11.16 at 14:54, <wei.liu2@citrix.com> wrote:
> On Thu, Nov 10, 2016 at 05:30:25AM -0700, Jan Beulich wrote:
>> With us incremementally adding proper CPUID checks to x86_emulate()
>> (see commit de05bd965a ["x86emul: correct {,F}CMOV and F{,U}COMI{,P}
>> emulation"]) it is no longer appropriate to invoke the function with
>> that hook being NULL, as long as respective instructions may get used
>> in that case.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> I've noticed the problem first with a not yet submitted patch checking
>> cx8 (PV 32-bit kernels occasionally use cmpxchg8b for page table
>> accesses), and I think we don't strictly need the change here for 4.8,
>> but then again it may be better to handle this properly right away.
> 
> I'm fine with having this if we have some free cycles. If it is reviewed
> today, feel free to commit it. Otherwise I will try to sneak it in after
> it is reviewed.

In the hope that you don't mind, I've translated this to R-a-b.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] x86: always supply .cpuid() handler to x86_emulate()

[Wei Liu]

On Fri, Nov 11, 2016 at 09:21:00AM -0700, Jan Beulich wrote:
> >>> On 11.11.16 at 14:54, <wei.liu2@citrix.com> wrote:
> > On Thu, Nov 10, 2016 at 05:30:25AM -0700, Jan Beulich wrote:
> >> With us incremementally adding proper CPUID checks to x86_emulate()
> >> (see commit de05bd965a ["x86emul: correct {,F}CMOV and F{,U}COMI{,P}
> >> emulation"]) it is no longer appropriate to invoke the function with
> >> that hook being NULL, as long as respective instructions may get used
> >> in that case.
> >> 
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> I've noticed the problem first with a not yet submitted patch checking
> >> cx8 (PV 32-bit kernels occasionally use cmpxchg8b for page table
> >> accesses), and I think we don't strictly need the change here for 4.8,
> >> but then again it may be better to handle this properly right away.
> > 
> > I'm fine with having this if we have some free cycles. If it is reviewed
> > today, feel free to commit it. Otherwise I will try to sneak it in after
> > it is reviewed.
> 
> In the hope that you don't mind, I've translated this to R-a-b.

That's absolutely fine.

> 
> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel