Re: [PATCH V1 14/16] xen/ioreq: Use guest_cmpxchg64() instead of cmpxchg()

[Julien Grall <julien@xxxxxxx>]

On 22/09/2020 21:05, Oleksandr wrote:
> On 16.09.20 12:12, Julien Grall wrote:
>
> Hi all.
>
> > On 16/09/2020 10:09, Paul Durrant wrote:
> > > > -----Original Message-----
> > > > From: Julien Grall <julien@xxxxxxx>
> > > > Sent: 16 September 2020 10:07
> > > > To: Jan Beulich <jbeulich@xxxxxxxx>; Oleksandr Tyshchenko 
> > > > <olekstysh@xxxxxxxxx>
> > > > Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Oleksandr Tyshchenko 
> > > > <oleksandr_tyshchenko@xxxxxxxx>; Paul Durrant
> > > > <paul@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Julien 
> > > > Grall <jgrall@xxxxxxxxxx>
> > > > Subject: Re: [PATCH V1 14/16] xen/ioreq: Use guest_cmpxchg64() 
> > > > instead of cmpxchg()
> > > >
> > > >
> > > > On 16/09/2020 10:04, Jan Beulich wrote:
> > > > > On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
> > > > > > @@ -1325,7 +1327,7 @@ static int hvm_send_buffered_ioreq(struct 
> > > > > > hvm_ioreq_server *s, ioreq_t *p)
> > > > > >            new.read_pointer = old.read_pointer - n * 
> > > > > > IOREQ_BUFFER_SLOT_NUM;
> > > > > >            new.write_pointer = old.write_pointer - n * 
> > > > > > IOREQ_BUFFER_SLOT_NUM;
> > > > > > -        cmpxchg(&pg->ptrs.full, old.full, new.full);
> > > > > > +        guest_cmpxchg64(d, &pg->ptrs.full, old.full, new.full);
> > > > > But the memory we're updating is shared with s->emulator, not with d,
> > > > > if I'm not mistaken.
> > > > It is unfortunately shared with both s->emulator and d when using the
> > > > legacy interface.
> > > When using magic pages they should be punched out of the P2M by the 
> > > time the code gets here, so the memory should not be guest-visible.
> > Can you point me to the code that doing this?
> >
> > Cheers,
> If we are not going to use legacy interface on Arm we will have a page 
> to be mapped in a single domain at the time.
Right, but this is common code... You have to think what would be the 
implication if we are using the legacy interface.
Thankfully the only user of the legacy interface is x86 so far and there 
is not concern regarding the atomics operations.
If we are happy to consider that the legacy interface will never be used 
(I am starting to be worry that one will ask it on Arm...) then we 
should be fine.
I think would be worth documenting in the commit message and the code 
(hvm_allow_set_param()) that the legacy interface *must* not be used 
without revisiting the code.
Cheers,

--
Julien Grall