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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 23.09.20 21:12, Julien Grall wrote:

Hi Julien

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


--
Regards,

Oleksandr Tyshchenko