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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/02/18 08:07, Jan Beulich wrote:
>
>>> +
>>> +    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?
> No, here we copy info hvm_emulate_cmpxchg()'s local variable,
> while there we copy into its caller's one. But anyway, the double
> copying gets eliminated by patch 25.

Ok.

>
>>> --- 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.
> I can split out the type change, but you realize this means touching
> twice some of the exact same code? As to changes not mentioned
> in the commit message - I'm having trouble to spot those (the type
> change is mentioned).

What you don't mention is that you're changing how
ptwr_emulated_update() evaluates what to do, and this took me a while to
figure out.

>
>> 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.
> I'll add a comment, even if I have a hard time finding a good place
> to put it. Ahead of the function isn't really a good place imo, but I
> also can't figure anything better.

With that sorted, Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

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