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

[Frederic Barrat <fbarrat@linux.ibm.com>]

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