Re: [PATCH V3 19/23] xen/arm: io: Abstract sign-extension

[Julien Grall <julien@xxxxxxx>]

On 30/11/2020 23:27, Oleksandr wrote:
> On 30.11.20 23:03, Volodymyr Babchuk wrote:
> > Hi,
> Hi Volodymyr
>
>
> > Oleksandr Tyshchenko writes:
> >
> > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > >
> > > 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@xxxxxxxx>
> > > CC: Julien Grall <julien.grall@xxxxxxx>
> > >
> > > ---
> > > 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
> > > ---
> > > ---
> > >   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 f44cfd4..8d6ec6c 100644
> > > --- a/xen/arch/arm/io.c
> > > +++ b/xen/arch/arm/io.c
> > > @@ -23,6 +23,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"
> > > @@ -39,26 +40,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 f08190c..2f39289 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;
> > If `size` is 64, you will get undefined behavior there.
> I think, we don't need to worry about undefined behavior here. Having 
> size=64 would be possible with doubleword (dabt.size=3). But if "r" 
> adjustment gets called (I mean Syndrome Sign Extend bit is set) then
> we deal with byte, halfword or word operations (dabt.size<3). Or I 
> missed something?
This is known and was pointed out in the commit message introducing the 
sign-extension:
"Note that the bit can only be set for access size smaller than the 
register size (i.e byte/half-word for aarch32, byte/half-word/word for 
aarch32). So we don't have to worry about undefined C behavior."
Cheers,

--
Julien Grall