Re: [PATCH V4 13/24] xen/ioreq: Use guest_cmpxchg64() instead of cmpxchg()

[Oleksandr <olekstysh@xxxxxxxxx>]

On 15.01.21 21:37, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien


> On 12/01/2021 21:52, Oleksandr Tyshchenko wrote:
> > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >
> > The cmpxchg() in ioreq_send_buffered() operates on memory shared
> > with the emulator domain (and the target domain if the legacy
> > interface is used).
> >
> > In order to be on the safe side we need to switch
> > to guest_cmpxchg64() to prevent a domain to DoS Xen on Arm.
> >
> > As there is no plan to support the legacy interface on Arm,
> > we will have a page to be mapped in a single domain at the time,
> > so we can use s->emulator in guest_cmpxchg64() safely.
> I think you want to explain why you are using the 64-bit version of 
> helper.
The point to use 64-bit version of helper is to support Arm32 since the 
IOREQ code uses cmpxchg() with 64-bit value.
I will update patch description.


> > Thankfully the only user of the legacy interface is x86 so far
> > and there is not concern regarding the atomics operations.
> >
> > Please note, that the legacy interface *must* not be used on Arm
> > without revisiting the code.
> >
> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > CC: Julien Grall <julien.grall@xxxxxxx>
> > [On Arm only]
> > Tested-by: Wei Chen <Wei.Chen@xxxxxxx>
> >
> > ---
> > Please note, this is a split/cleanup/hardening of Julien's PoC:
> > "Add support for Guest IO forwarding to a device emulator"
> >
> > Changes RFC -> V1:
> >     - new patch
> >
> > Changes V1 -> V2:
> >     - move earlier to avoid breaking arm32 compilation
> >     - add an explanation to commit description and hvm_allow_set_param()
> >     - pass s->emulator
> >
> > Changes V2 -> V3:
> >     - update patch description
> >
> > Changes V3 -> V4:
> >     - add Stefano's A-b
> >     - drop comment from arm/hvm.c
> > ---
> >   xen/common/ioreq.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
> > index d233a49..d5f4dd3 100644
> > --- a/xen/common/ioreq.c
> > +++ b/xen/common/ioreq.c
> > @@ -29,6 +29,7 @@
> >   #include <xen/trace.h>
> >   #include <xen/vpci.h>
> >   +#include <asm/guest_atomics.h>
> >   #include <asm/hvm/ioreq.h>
> >     #include <public/hvm/ioreq.h>
> > @@ -1185,7 +1186,7 @@ static int ioreq_send_buffered(struct 
> > 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(s->emulator, &pg->ptrs.full, old.full, 
> > new.full);
> >       }
> >         notify_via_xen_event_channel(d, s->bufioreq_evtchn);
> Cheers,
--
Regards,

Oleksandr Tyshchenko