| Message ID | 1610488352-18494-14-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IOREQ feature (+ virtio-mmio) on Arm | expand |
Hi Oleksandr, On 12/01/2021 21:52, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > 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. > > 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@epam.com> > Acked-by: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien.grall@arm.com> > [On Arm only] > Tested-by: Wei Chen <Wei.Chen@arm.com> > > --- > 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,
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@epam.com> >> >> 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@epam.com> >> Acked-by: Stefano Stabellini <sstabellini@kernel.org> >> CC: Julien Grall <julien.grall@arm.com> >> [On Arm only] >> Tested-by: Wei Chen <Wei.Chen@arm.com> >> >> --- >> 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, >
> -----Original Message----- > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Oleksandr Tyshchenko > Sent: 12 January 2021 21:52 > To: xen-devel@lists.xenproject.org > Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>; Paul Durrant <paul@xen.org>; Julien Grall > <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>; Julien Grall <julien.grall@arm.com> > Subject: [PATCH V4 13/24] xen/ioreq: Use guest_cmpxchg64() instead of cmpxchg() > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > 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. > > 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@epam.com> > Acked-by: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien.grall@arm.com> > [On Arm only] > Tested-by: Wei Chen <Wei.Chen@arm.com> Reviewed-by: Paul Durrant <paul@xen.org>
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);