| Message ID | 1610488352-18494-20-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IOREQ feature (+ virtio-mmio) on Arm | expand |
On Tue, 12 Jan 2021, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > In order to avoid code duplication (both handle_read() and > handle_ioserv() contain the same code for the sign-extension) > put this code to a common helper to be used for both. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > CC: Julien Grall <julien.grall@arm.com> > [On Arm only] > Tested-by: Wei Chen <Wei.Chen@arm.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > Please note, this is a split/cleanup/hardening of Julien's PoC: > "Add support for Guest IO forwarding to a device emulator" > > Changes V1 -> V2: > - new patch > > Changes V2 -> V3: > - no changes > > Changes V3 -> V4: > - no changes here, but in new patch: > "xen/arm: io: Harden sign extension check" > --- > xen/arch/arm/io.c | 18 ++---------------- > xen/arch/arm/ioreq.c | 17 +---------------- > xen/include/asm-arm/traps.h | 24 ++++++++++++++++++++++++ > 3 files changed, 27 insertions(+), 32 deletions(-) > > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c > index 9814481..307c521 100644 > --- a/xen/arch/arm/io.c > +++ b/xen/arch/arm/io.c > @@ -24,6 +24,7 @@ > #include <asm/cpuerrata.h> > #include <asm/current.h> > #include <asm/mmio.h> > +#include <asm/traps.h> > #include <asm/hvm/ioreq.h> > > #include "decode.h" > @@ -40,26 +41,11 @@ static enum io_state handle_read(const struct mmio_handler *handler, > * setting r). > */ > register_t r = 0; > - uint8_t size = (1 << dabt.size) * 8; > > if ( !handler->ops->read(v, info, &r, handler->priv) ) > return IO_ABORT; > > - /* > - * Sign extend if required. > - * Note that we expect the read handler to have zeroed the bits > - * outside the requested access size. > - */ > - if ( dabt.sign && (r & (1UL << (size - 1))) ) > - { > - /* > - * We are relying on register_t using the same as > - * an unsigned long in order to keep the 32-bit assembly > - * code smaller. > - */ > - BUILD_BUG_ON(sizeof(register_t) != sizeof(unsigned long)); > - r |= (~0UL) << size; > - } > + r = sign_extend(dabt, r); > > set_user_reg(regs, dabt.reg, r); > > diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c > index 3c4a24d..40b9e59 100644 > --- a/xen/arch/arm/ioreq.c > +++ b/xen/arch/arm/ioreq.c > @@ -28,7 +28,6 @@ enum io_state handle_ioserv(struct cpu_user_regs *regs, struct vcpu *v) > const union hsr hsr = { .bits = regs->hsr }; > const struct hsr_dabt dabt = hsr.dabt; > /* Code is similar to handle_read */ > - uint8_t size = (1 << dabt.size) * 8; > register_t r = v->io.req.data; > > /* We are done with the IO */ > @@ -37,21 +36,7 @@ enum io_state handle_ioserv(struct cpu_user_regs *regs, struct vcpu *v) > if ( dabt.write ) > return IO_HANDLED; > > - /* > - * Sign extend if required. > - * Note that we expect the read handler to have zeroed the bits > - * outside the requested access size. > - */ > - if ( dabt.sign && (r & (1UL << (size - 1))) ) > - { > - /* > - * We are relying on register_t using the same as > - * an unsigned long in order to keep the 32-bit assembly > - * code smaller. > - */ > - BUILD_BUG_ON(sizeof(register_t) != sizeof(unsigned long)); > - r |= (~0UL) << size; > - } > + r = sign_extend(dabt, r); > > set_user_reg(regs, dabt.reg, r); > > diff --git a/xen/include/asm-arm/traps.h b/xen/include/asm-arm/traps.h > index 997c378..e301c44 100644 > --- a/xen/include/asm-arm/traps.h > +++ b/xen/include/asm-arm/traps.h > @@ -83,6 +83,30 @@ static inline bool VABORT_GEN_BY_GUEST(const struct cpu_user_regs *regs) > (unsigned long)abort_guest_exit_end == regs->pc; > } > > +/* Check whether the sign extension is required and perform it */ > +static inline register_t sign_extend(const struct hsr_dabt dabt, register_t r) > +{ > + uint8_t size = (1 << dabt.size) * 8; > + > + /* > + * Sign extend if required. > + * Note that we expect the read handler to have zeroed the bits > + * outside the requested access size. > + */ > + if ( dabt.sign && (r & (1UL << (size - 1))) ) > + { > + /* > + * We are relying on register_t using the same as > + * an unsigned long in order to keep the 32-bit assembly > + * code smaller. > + */ > + BUILD_BUG_ON(sizeof(register_t) != sizeof(unsigned long)); > + r |= (~0UL) << size; > + } > + > + return r; > +} > + > #endif /* __ASM_ARM_TRAPS__ */ > /* > * Local variables: > -- > 2.7.4 >
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c index 9814481..307c521 100644 --- a/xen/arch/arm/io.c +++ b/xen/arch/arm/io.c @@ -24,6 +24,7 @@ #include <asm/cpuerrata.h> #include <asm/current.h> #include <asm/mmio.h> +#include <asm/traps.h> #include <asm/hvm/ioreq.h> #include "decode.h" @@ -40,26 +41,11 @@ static enum io_state handle_read(const struct mmio_handler *handler, * setting r). */ register_t r = 0; - uint8_t size = (1 << dabt.size) * 8; if ( !handler->ops->read(v, info, &r, handler->priv) ) return IO_ABORT; - /* - * Sign extend if required. - * Note that we expect the read handler to have zeroed the bits - * outside the requested access size. - */ - if ( dabt.sign && (r & (1UL << (size - 1))) ) - { - /* - * We are relying on register_t using the same as - * an unsigned long in order to keep the 32-bit assembly - * code smaller. - */ - BUILD_BUG_ON(sizeof(register_t) != sizeof(unsigned long)); - r |= (~0UL) << size; - } + r = sign_extend(dabt, r); set_user_reg(regs, dabt.reg, r); diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c index 3c4a24d..40b9e59 100644 --- a/xen/arch/arm/ioreq.c +++ b/xen/arch/arm/ioreq.c @@ -28,7 +28,6 @@ enum io_state handle_ioserv(struct cpu_user_regs *regs, struct vcpu *v) const union hsr hsr = { .bits = regs->hsr }; const struct hsr_dabt dabt = hsr.dabt; /* Code is similar to handle_read */ - uint8_t size = (1 << dabt.size) * 8; register_t r = v->io.req.data; /* We are done with the IO */ @@ -37,21 +36,7 @@ enum io_state handle_ioserv(struct cpu_user_regs *regs, struct vcpu *v) if ( dabt.write ) return IO_HANDLED; - /* - * Sign extend if required. - * Note that we expect the read handler to have zeroed the bits - * outside the requested access size. - */ - if ( dabt.sign && (r & (1UL << (size - 1))) ) - { - /* - * We are relying on register_t using the same as - * an unsigned long in order to keep the 32-bit assembly - * code smaller. - */ - BUILD_BUG_ON(sizeof(register_t) != sizeof(unsigned long)); - r |= (~0UL) << size; - } + r = sign_extend(dabt, r); set_user_reg(regs, dabt.reg, r); diff --git a/xen/include/asm-arm/traps.h b/xen/include/asm-arm/traps.h index 997c378..e301c44 100644 --- a/xen/include/asm-arm/traps.h +++ b/xen/include/asm-arm/traps.h @@ -83,6 +83,30 @@ static inline bool VABORT_GEN_BY_GUEST(const struct cpu_user_regs *regs) (unsigned long)abort_guest_exit_end == regs->pc; } +/* Check whether the sign extension is required and perform it */ +static inline register_t sign_extend(const struct hsr_dabt dabt, register_t r) +{ + uint8_t size = (1 << dabt.size) * 8; + + /* + * Sign extend if required. + * Note that we expect the read handler to have zeroed the bits + * outside the requested access size. + */ + if ( dabt.sign && (r & (1UL << (size - 1))) ) + { + /* + * We are relying on register_t using the same as + * an unsigned long in order to keep the 32-bit assembly + * code smaller. + */ + BUILD_BUG_ON(sizeof(register_t) != sizeof(unsigned long)); + r |= (~0UL) << size; + } + + return r; +} + #endif /* __ASM_ARM_TRAPS__ */ /* * Local variables: