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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.02.18 at 15:49, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 07/12/17 14:15, Jan Beulich wrote:
>> --- 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);

For the sake of the response below, please note the passing of
&old (rather than p_old) here.

>> +
>> +    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.

>> --- 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).

> 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.

Jan


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