Re: [Xen-devel] [PATCH v3 20/25] x86emul: correctly handle CMPXCHG* comparison failures

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/12/17 14:15, Jan Beulich wrote:
> If the ->cmpxchg() hook finds a mismatch, we should deal with this the
> same way as when the "manual" comparison reports a mismatch.
>
> This involves reverting bfce0e62c3 ("x86/emul: Drop
> X86EMUL_CMPXCHG_FAILED"), albeit with X86EMUL_CMPXCHG_FAILED now
> becoming a value distinct from X86EMUL_RETRY.
>
> In order to not leave mixed code also fully switch affected functions
> from paddr_t to intpte_t.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v3: New.
> ---
> The code could be further simplified if we could rely on all
> ->cmpxchg() hooks always using CMPXCHG, but for now we need to cope
> with them using plain writes (and hence accept the double reads if
> CMPXCHG is actually being used).
> Note that the patch doesn't address the incorrectness of there not
> being a memory write even in the comparison-failed case.
>
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -302,8 +302,12 @@ hvm_emulate_cmpxchg(enum x86_segment seg
>      memcpy(&old, p_old, bytes);
>      memcpy(&new, p_new, bytes);
>  
> -    return v->arch.paging.mode->shadow.x86_emulate_cmpxchg(
> -               v, addr, old, new, bytes, sh_ctxt);
> +    rc = v->arch.paging.mode->shadow.x86_emulate_cmpxchg(
> +             v, addr, &old, new, bytes, sh_ctxt);
> +
> +    memcpy(p_old, &old, bytes);

This is redundant with ...

> +
> +    return rc;
>  }
>  
>  static const struct x86_emulate_ops hvm_shadow_emulator_ops = {
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -4741,11 +4741,11 @@ sh_x86_emulate_write(struct vcpu *v, uns
>  
>  static int
>  sh_x86_emulate_cmpxchg(struct vcpu *v, unsigned long vaddr,
> -                        unsigned long old, unsigned long new,
> -                        unsigned int bytes, struct sh_emulate_ctxt *sh_ctxt)
> +                       unsigned long *p_old, unsigned long new,
> +                       unsigned int bytes, struct sh_emulate_ctxt *sh_ctxt)
>  {
>      void *addr;
> -    unsigned long prev;
> +    unsigned long prev, old = *p_old;
>      int rv = X86EMUL_OKAY;
>  
>      /* Unaligned writes are only acceptable on HVM */
> @@ -4769,7 +4769,10 @@ sh_x86_emulate_cmpxchg(struct vcpu *v, u
>      }
>  
>      if ( prev != old )
> -        rv = X86EMUL_RETRY;
> +    {
> +        *p_old = prev;

... this, is it not?

> +        rv = X86EMUL_CMPXCHG_FAILED;
> +    }
>  
>      SHADOW_DEBUG(EMULATE, "va %#lx was %#lx expected %#lx"
>                    " wanted %#lx now %#lx bytes %u\n",
> --- a/xen/arch/x86/pv/ro-page-fault.c
> +++ b/xen/arch/x86/pv/ro-page-fault.c
> @@ -65,14 +65,16 @@ static int ptwr_emulated_read(enum x86_s
>      return X86EMUL_OKAY;
>  }
>  
> -static int ptwr_emulated_update(unsigned long addr, paddr_t old, paddr_t val,
> -                                unsigned int bytes, unsigned int do_cmpxchg,
> +static int ptwr_emulated_update(unsigned long addr, intpte_t *p_old,
> +                                intpte_t val, unsigned int bytes,
>                                  struct x86_emulate_ctxt *ctxt)
>  {
>      unsigned long mfn;
>      unsigned long unaligned_addr = addr;
>      struct page_info *page;
>      l1_pgentry_t pte, ol1e, nl1e, *pl1e;
> +    intpte_t old = p_old ? *p_old : 0;
> +    unsigned int offset = 0;

I really think this conversion to intpte needs splitting out into a
separate patch.  You're making multiple changes in this function which
aren't at commit message at all, including introducing the distinction
I've just noted of *p_old being NULL meaning a write rather than cmpxchg.

On that note specifically, it would be clearer to have "const bool
do_cmpxchg = *p_old; /* cmpxchg, or write? */".  If you don't want to do
it, then there needs to be a comment with the function explaining the
semantics of p_old.

~Andrew


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel