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

[Julien Grall <julien@xxxxxxx>]

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,

--
Julien Grall