| Message ID | 1606732298-22107-20-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V3,01/23] x86/ioreq: Prepare IOREQ feature for making it common | expand |
Hi, Oleksandr Tyshchenko writes: > 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> > > --- > 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. > + } > + > + return r; > +} > + > #endif /* __ASM_ARM_TRAPS__ */ > /* > * Local variables:
On 30.11.20 23:03, Volodymyr Babchuk wrote: > Hi, Hi Volodymyr > > Oleksandr Tyshchenko writes: > >> 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> >> >> --- >> 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?
On 01.12.2020 00:27, Oleksandr wrote: > On 30.11.20 23:03, Volodymyr Babchuk wrote: >> Oleksandr Tyshchenko writes: >>> --- 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? At which point please put in a respective ASSERT(), possibly amended by a brief comment. Jan
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@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> >>> >>> --- >>> 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,
Hi Jan, On 01/12/2020 07:55, Jan Beulich wrote: > On 01.12.2020 00:27, Oleksandr wrote: >> On 30.11.20 23:03, Volodymyr Babchuk wrote: >>> Oleksandr Tyshchenko writes: >>>> --- 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? > > At which point please put in a respective ASSERT(), possibly amended > by a brief comment. ASSERT()s are only meant to catch programatic error. However, in this case, the bigger risk is an hardware bug such as advertising a sign extension for either 64-bit (or 32-bit) on Arm64 (resp. Arm32). Actually the Armv8 spec is a bit more blurry when running in AArch32 state because they suggest that the sign extension can be set even for 32-bit access. I think this is a spelling mistake, but it is probably better to be cautious here. Therefore, I would recommend to rework the code so it is only called when len < sizeof(register_t). Cheers,
On 01.12.20 12:30, Julien Grall wrote: Hi Julien > Hi Jan, > > On 01/12/2020 07:55, Jan Beulich wrote: >> On 01.12.2020 00:27, Oleksandr wrote: >>> On 30.11.20 23:03, Volodymyr Babchuk wrote: >>>> Oleksandr Tyshchenko writes: >>>>> --- 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? >> >> At which point please put in a respective ASSERT(), possibly amended >> by a brief comment. > > ASSERT()s are only meant to catch programatic error. However, in this > case, the bigger risk is an hardware bug such as advertising a sign > extension for either 64-bit (or 32-bit) on Arm64 (resp. Arm32). > > Actually the Armv8 spec is a bit more blurry when running in AArch32 > state because they suggest that the sign extension can be set even for > 32-bit access. I think this is a spelling mistake, but it is probably > better to be cautious here. > > Therefore, I would recommend to rework the code so it is only called > when len < sizeof(register_t). I am not sure I understand the recommendation, could you please clarify (also I don't see 'len' being used here).
On 01.12.2020 11:30, Julien Grall wrote: > Hi Jan, > > On 01/12/2020 07:55, Jan Beulich wrote: >> On 01.12.2020 00:27, Oleksandr wrote: >>> On 30.11.20 23:03, Volodymyr Babchuk wrote: >>>> Oleksandr Tyshchenko writes: >>>>> --- 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? >> >> At which point please put in a respective ASSERT(), possibly amended >> by a brief comment. > > ASSERT()s are only meant to catch programatic error. However, in this > case, the bigger risk is an hardware bug such as advertising a sign > extension for either 64-bit (or 32-bit) on Arm64 (resp. Arm32). > > Actually the Armv8 spec is a bit more blurry when running in AArch32 > state because they suggest that the sign extension can be set even for > 32-bit access. I think this is a spelling mistake, but it is probably > better to be cautious here. > > Therefore, I would recommend to rework the code so it is only called > when len < sizeof(register_t). This would be even better in this case, I agree. Jan
Hi Oleksandr, On 01/12/2020 10:42, Oleksandr wrote: > > On 01.12.20 12:30, Julien Grall wrote: > > Hi Julien > >> Hi Jan, >> >> On 01/12/2020 07:55, Jan Beulich wrote: >>> On 01.12.2020 00:27, Oleksandr wrote: >>>> On 30.11.20 23:03, Volodymyr Babchuk wrote: >>>>> Oleksandr Tyshchenko writes: >>>>>> --- 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? >>> >>> At which point please put in a respective ASSERT(), possibly amended >>> by a brief comment. >> >> ASSERT()s are only meant to catch programatic error. However, in this >> case, the bigger risk is an hardware bug such as advertising a sign >> extension for either 64-bit (or 32-bit) on Arm64 (resp. Arm32). >> >> Actually the Armv8 spec is a bit more blurry when running in AArch32 >> state because they suggest that the sign extension can be set even for >> 32-bit access. I think this is a spelling mistake, but it is probably >> better to be cautious here. >> >> Therefore, I would recommend to rework the code so it is only called >> when len < sizeof(register_t). > > I am not sure I understand the recommendation, could you please clarify > (also I don't see 'len' being used here). Sorry I meant 'size'. I think something like: if ( dabt.sign && (size < sizeof(register_t)) && (r & (1UL << (size - 1)) ) { } Another posibility would be: if ( dabt.sign && (size < sizeof(register_t)) ) { /* find whether the sign bit is set and propagate it */ } I have a slight preference for the latter as the "if" is easier to read. In any case, I think this change should be done in a separate patch (I don't mint whether this is done after or before this one). Cheers,
On 01.12.20 14:13, Julien Grall wrote: > Hi Oleksandr, Hi Julien. > >>>>>>> --- 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? >>>> >>>> At which point please put in a respective ASSERT(), possibly amended >>>> by a brief comment. >>> >>> ASSERT()s are only meant to catch programatic error. However, in >>> this case, the bigger risk is an hardware bug such as advertising a >>> sign extension for either 64-bit (or 32-bit) on Arm64 (resp. Arm32). >>> >>> Actually the Armv8 spec is a bit more blurry when running in AArch32 >>> state because they suggest that the sign extension can be set even >>> for 32-bit access. I think this is a spelling mistake, but it is >>> probably better to be cautious here. >>> >>> Therefore, I would recommend to rework the code so it is only called >>> when len < sizeof(register_t). >> >> I am not sure I understand the recommendation, could you please >> clarify (also I don't see 'len' being used here). > > Sorry I meant 'size'. I think something like: > > if ( dabt.sign && (size < sizeof(register_t)) && > (r & (1UL << (size - 1)) ) > { > } > > Another posibility would be: > > if ( dabt.sign && (size < sizeof(register_t)) ) > { > /* find whether the sign bit is set and propagate it */ > } > > I have a slight preference for the latter as the "if" is easier to read. > > In any case, I think this change should be done in a separate patch (I > don't mint whether this is done after or before this one). ok, I got it, thank you for the clarification. Of course, I will do that in a separate patch, since the current one is to avoid code duplication only. BTW, do you have comments on this patch itself?
On 01/12/2020 12:24, Oleksandr wrote: > > On 01.12.20 14:13, Julien Grall wrote: >> Hi Oleksandr, > > Hi Julien. > > >> >>>>>>>> --- 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? >>>>> >>>>> At which point please put in a respective ASSERT(), possibly amended >>>>> by a brief comment. >>>> >>>> ASSERT()s are only meant to catch programatic error. However, in >>>> this case, the bigger risk is an hardware bug such as advertising a >>>> sign extension for either 64-bit (or 32-bit) on Arm64 (resp. Arm32). >>>> >>>> Actually the Armv8 spec is a bit more blurry when running in AArch32 >>>> state because they suggest that the sign extension can be set even >>>> for 32-bit access. I think this is a spelling mistake, but it is >>>> probably better to be cautious here. >>>> >>>> Therefore, I would recommend to rework the code so it is only called >>>> when len < sizeof(register_t). >>> >>> I am not sure I understand the recommendation, could you please >>> clarify (also I don't see 'len' being used here). >> >> Sorry I meant 'size'. I think something like: >> >> if ( dabt.sign && (size < sizeof(register_t)) && >> (r & (1UL << (size - 1)) ) >> { >> } >> >> Another posibility would be: >> >> if ( dabt.sign && (size < sizeof(register_t)) ) >> { >> /* find whether the sign bit is set and propagate it */ >> } >> >> I have a slight preference for the latter as the "if" is easier to read. >> >> In any case, I think this change should be done in a separate patch (I >> don't mint whether this is done after or before this one). > > ok, I got it, thank you for the clarification. Of course, I will do that > in a separate patch, since the current one is to avoid code duplication > only. BTW, do you have comments on this patch itself? The series is in my TODO list. I will have a look once in a bit :). Cheers,
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; + } + + return r; +} + #endif /* __ASM_ARM_TRAPS__ */ /* * Local variables: