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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.