Re: [PATCH v1 3/8] xen/arm: Support sign-extension for every read access

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-09-25 at 15:51 +0100, Julien Grall wrote:
> The guest may try to load data from the emulated MMIO region using
> instruction with Sign-Extension (i.e ldrs*). This can happen for any
> access smaller than the register size (byte/half-word for aarch32,
> byte/half-word/word for aarch64).
> 
> The support of sign-extension was limited for byte access in vGIG

"vGIC"

> emulation. Although there is no reason to not have it generically.
> 
> So move the support just after we get the data from the MMIO emulation.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
> ---
> 
> I was thinking to completely drop the sign-extension support in Xen as
> it will be very unlikely to use ldrs* instruction to access MMIO.
> Although the code is fairly small, so it doesn't harm to keep it
> generically.

Yes, I think we should keep it, since we don't control what instructions
are used to access MMIO, however unlikely...



>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/io.c          | 29 ++++++++++++++++++++++++++++-
>  xen/arch/arm/vgic-v2.c     | 10 +++++-----
>  xen/arch/arm/vgic-v3.c     |  4 ++--
>  xen/include/asm-arm/vgic.h |  8 +++-----
>  4 files changed, 38 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index 32b2194..e1b03a2 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -23,6 +23,32 @@
>  #include <asm/current.h>
>  #include <asm/mmio.h>
>  
> +static int handle_read(mmio_read_t read_cb, struct vcpu *v,
> +                       mmio_info_t *info, register_t *r)
> +{
> +    uint8_t size = (1 << info->dabt.size) * 8;
> +
> +    if ( !read_cb(v, info, r) )
> +        return 0;
> +
> +    /*
> +     * Extend the bit sign if required.

I think you meant s/bit sign/sign bit/ but more correct would be "Sign
extend if required".

> +     * Note that we expect the read handler to have zeroed the bit
> +     * unused in the register.

"... to have zeroed the unused bits in the register".

But I think "unused" is a bit misleading, you mean the ones outside the
requested access size, those bits are still "used" IYSWIM. I can't think of
a terse term for "outside the requested access size I'm afraid.

Did you confirm that all existing handlers meet this requirement? Perhaps
an ASSERT would be handy?

> +     */
> +    if ( info->dabt.sign && (*r & (1UL << (size - 1)) ))
> +    {
> +        /*
> +         * We are relying on register_t as the same size as
> +         * an unsigned long or order to keep the 32bit some smaller

"order"? I'm not sure what you meant here so I can't suggest an
alternative.

> +         */
> +        BUILD_BUG_ON(sizeof(register_t) != sizeof(unsigned long));
> +        *r |= (~0UL) << size;

I think here and in the initial if you need to be careful of the case where
size == 32 (on arm32) or == 64 (on arm64), since a shift by >= the size of
the variable is, I think, undefined behaviour.

It's also a waste of time sign extending in that case.

Ian.