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

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

[Frederic Barrat]

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,
     },
 };
-- 
2.41.0

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

[Philippe Mathieu-Daudé]

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,
>       },
>   };

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

[Frederic Barrat]

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,
>>       },
>>   };
> 

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.

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

[Cédric Le Goater]

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.

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

[Daniel Henrique Barboza]

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,
>       },
>   };