Re: [PATCH] pnv/xive2: Allow indirect TIMA accesses of all sizes

[Philippe Mathieu-Daudé]

Hi Frederic,

On 26/6/23 13:25, Frederic Barrat wrote:
> On 26/06/2023 11:48, Philippe Mathieu-Daudé wrote:
> > On 26/6/23 11:40, Frederic Barrat wrote:
> > > Booting linux on the powernv10 machine logs a few errors like:
> > >
> > > Invalid read at addr 0x38, size 1, region 'xive-ic-tm-indirect', 
> > > reason: invalid size (min:8 max:8)
> > > Invalid write at addr 0x38, size 1, region 'xive-ic-tm-indirect', 
> > > reason: invalid size (min:8 max:8)
> > > Invalid read at addr 0x38, size 1, region 'xive-ic-tm-indirect', 
> > > reason: invalid size (min:8 max:8)
> > >
> > > Those errors happen when linux is resetting XIVE. We're trying to
> > > read/write the enablement bit for the hardware context and qemu
> > > doesn't allow indirect TIMA accesses of less than 8 bytes. Direct TIMA
> > > access can go through though, as well as indirect TIMA accesses on P9.
> > > So even though there are some restrictions regarding the address/size
> > > combinations for TIMA access, the example above is perfectly valid.
> > >
> > > This patch lets indirect TIMA accesses of all sizes go through. The
> > > special operations will be intercepted and the default "raw" handlers
> > > will pick up all other requests and complain about invalid sizes as
> > > appropriate.
> > >
> > > Tested-by: Nicholas Piggin <npiggin@gmail.com>
> > > Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> > > ---
> > >   hw/intc/pnv_xive2.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/intc/pnv_xive2.c b/hw/intc/pnv_xive2.c
> > > index ed438a20ed..e8ab176de6 100644
> > > --- a/hw/intc/pnv_xive2.c
> > > +++ b/hw/intc/pnv_xive2.c
> > > @@ -1644,11 +1644,11 @@ static const MemoryRegionOps 
> > > pnv_xive2_ic_tm_indirect_ops = {
> > >       .write = pnv_xive2_ic_tm_indirect_write,
> > >       .endianness = DEVICE_BIG_ENDIAN,
> > >       .valid = {
> > > -        .min_access_size = 8,
> > > +        .min_access_size = 1,
> >
> > Maybe. Is there a bus involved in between?
> >
> > What about other I/O regions?
>
>
> XIVE is attached to the main system bus and the CPU can trigger 1, 2, 4 
> and 8-byte accesses. The TIMA is a part of XIVE which supports various 
> size of mmio operations, all the way down to byte operations. It 
> actually relies on it.
>
> There are 2 memory regions where we want to allow byte-access. One, 
> known as TIMA direct access, already allows access with min size = 1. 
> I'm just aligning the other one, known as TIMA indirect access, to do 
> the same, since it's what the hardware allows.
> This is similar to what we had on P9 and both regions are already 
> defined with min size = 1 there. So it really looks like what I'm 
> changing here was just an oversight.

OK.

> > >           .max_access_size = 8,
> > >       },
> > >       .impl = {
> > > -        .min_access_size = 8,
> > > +        .min_access_size = 1,
> >
> > Unlikely. This is for the handler implementation, not related to HW.
>
>
> The handlers for the TIMA regions are aware of the size of the access, 
> and behave differently based on it (see xive_tm_find_op() for example). 
> So I think this is correct. Let me know if I'm missing something here.

I guess I got confused by the "Only 4 or 8 bytes loads are allowed"
comment in xive_tm_raw_read/write(), so I was somehow expecting
min_access_size = 4. I don't object to this patch however.

Thanks,

Phil.