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

[Julien Grall <julien@xxxxxxx>]

Hi Roger,

On 28/05/2024 08:11, Roger Pau Monné wrote:
> On Mon, May 27, 2024 at 10:14:59PM +0100, Julien Grall wrote:
> > 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.
> Even if there was a bug in the handler, it would be mitigated by the
> truncation done in io.c.
>
> > 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...
> It's up to you really, it's all ARM code so I don't really have a
> stake.  IMO it makes the handlers more complicated and fragile.
I will let the other Arm folks commenting on it.

> If nothing else I would at least add an ASSERT() in io.c to ensure
> that the data returned from the handler matches the size constrains
> you expect.
That would be a good idea.

> > > 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().
> I would consider doing the truncation in sign_extend() if suitable,
> even if that's doing more than what the function name implies.
If we decide to do a general truncation, then I would rather prefer if 
it happens outside of sign_extend(). Or the function needs to be renamed 
(I can't find a good name so far).
Cheers,

--
Julien Grall