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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 10 November 2016 12:30
> To: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Paul Durrant
> <Paul.Durrant@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Tim (Xen.org)
> <tim@xxxxxxx>
> 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@xxxxxxxx>

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

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

> ---
> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel