| Message ID | 20230626094057.1192473-1-fbarrat@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pnv/xive2: Allow indirect TIMA accesses of all sizes | expand |
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? > .max_access_size = 8, > }, > .impl = { > - .min_access_size = 8, > + .min_access_size = 1, Unlikely. This is for the handler implementation, not related to HW. > .max_access_size = 8, > }, > };
On 6/26/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, > .max_access_size = 8, > }, > .impl = { > - .min_access_size = 8, > + .min_access_size = 1, > .max_access_size = 8, > }, > }; This makes the TM indirect ops and the TM direct ops similar on P10. Same as P9 LGTM, Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C.
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. >> .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. Fred >> .max_access_size = 8, >> }, >> }; >
Queued in gitlab.com/danielhb/qemu/tree/ppc-next. Thanks, Daniel On 6/26/23 06: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, > .max_access_size = 8, > }, > .impl = { > - .min_access_size = 8, > + .min_access_size = 1, > .max_access_size = 8, > }, > };
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.
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, .max_access_size = 8, }, .impl = { - .min_access_size = 8, + .min_access_size = 1, .max_access_size = 8, }, };