Re: [PATCH v16 1/5] arm/vpci: honor access size when returning an error

[Julien Grall <julien@xxxxxxx>]

Hi Roger,

On 23/05/2024 08:55, Roger Pau Monné wrote:
> On Wed, May 22, 2024 at 06:59:20PM -0400, Stewart Hildebrand wrote:
> > From: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> >
> > Guest can try to read config space using different access sizes: 8,
> > 16, 32, 64 bits. We need to take this into account when we are
> > returning an error back to MMIO handler, otherwise it is possible to
> > provide more data than requested: i.e. guest issues LDRB instruction
> > to read one byte, but we are writing 0xFFFFFFFFFFFFFFFF in the target
> > register.
> Shouldn't this be taken care of in the trap handler subsystem, rather
> than forcing each handler to ensure the returned data matches the
> access size?
I understand how this can be useful when we return all 1s.

However, in most of the current cases, we already need to deal with the 
masking because the data is extracted from a wider field (for instance, 
see the vGIC emulation). For those handlers, I would argue it would be 
concerning/ a bug if the handler return bits above the access size. 
Although, this would only impact the guest itself.
So overall, this seems to be a matter of taste and I don't quite (yet) 
see the benefits to do it in io.c. Regardless that...
> IOW, something like:
>
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index 96c740d5636c..b7e12df85f87 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -37,6 +37,7 @@ static enum io_state handle_read(const struct mmio_handler 
> *handler,
>           return IO_ABORT;
>
>       r = sign_extend(dabt, r);
> +    r = r & GENMASK_ULL((1U << dabt.size) * 8 - 1, 0);
... in some case we need to sign extend up to the width of the register 
(even if the access is 8-byte). So we would need to do the masking 
*before* calling sign_extend().
Cheers,

--
Julien Grall