Re: [PATCH v2 2/2] s390x/pci: Fix memory_region_access_valid call

[Pierre Morel <pmorel@linux.ibm.com>]

On 12/18/20 5:51 PM, Cornelia Huck wrote:
> On Fri, 18 Dec 2020 17:40:50 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 12/18/20 4:32 PM, Cornelia Huck wrote:
>>> On Fri, 18 Dec 2020 15:32:08 +0100
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>    
>>>> On 12/18/20 12:04 PM, Cornelia Huck wrote:
>>>>> On Fri, 18 Dec 2020 10:37:38 +0100
>>>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>>>       
>>>>>> On 12/17/20 11:16 PM, Matthew Rosato wrote:
>>>>>>> In pcistb_service_handler, a call is made to validate that the memory
>>>>>>> region can be accessed.  However, the call is made using the entire length
>>>>>>> of the pcistb operation, which can be larger than the allowed memory
>>>>>>> access size (8).  Since we already know that the provided buffer is a
>>>>>>> multiple of 8, fix the call to memory_region_access_valid to iterate
>>>>>>> over the memory region in the same way as the subsequent call to
>>>>>>> memory_region_dispatch_write.
>>>>>>>
>>>>>>> Fixes: 863f6f52b7 ("s390: implement pci instructions")
>>>>>>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
>>>>>>> ---
>>>>>>>      hw/s390x/s390-pci-inst.c | 10 ++++++----
>>>>>>>      1 file changed, 6 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>>>>>>> index e230293..76b08a3 100644
>>>>>>> --- a/hw/s390x/s390-pci-inst.c
>>>>>>> +++ b/hw/s390x/s390-pci-inst.c
>>>>>>> @@ -821,10 +821,12 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>>>>>>          mr = s390_get_subregion(mr, offset, len);
>>>>>>>          offset -= mr->addr;
>>>>>>>      
>>>>>>> -    if (!memory_region_access_valid(mr, offset, len, true,
>>>>>>> -                                    MEMTXATTRS_UNSPECIFIED)) {
>>>>>>> -        s390_program_interrupt(env, PGM_OPERAND, ra);
>>>>>>> -        return 0;
>>>>>>> +    for (i = 0; i < len; i += 8) {
>>>>>>> +        if (!memory_region_access_valid(mr, offset + i, 8, true,
>>>>>>> +                                        MEMTXATTRS_UNSPECIFIED)) {
>>>>>>> +            s390_program_interrupt(env, PGM_OPERAND, ra);
>>>>>>> +            return 0;
>>>>>>> +        }
>>>>>>>          }
>>>>>>>      
>>>>>>>          if (s390_cpu_virt_mem_read(cpu, gaddr, ar, buffer, len)) {
>>>>>>>          
>>>>>>
>>>>>> wouldn't it be made automatically by defining the io_region
>>>>>> max_access_size when reading the bars in clp_service_call?
>>>>>>      
>>>>>
>>>>> But that's already what is happening, isn't it? The access check is
>>>>> done for a size that is potentially too large, while the actual access
>>>>> will happen in chunks of 8? I think that this patch is correct.
>>>>>       
>>>>
>>>> Sorry I was too rapid and half wrong in my writing I was also not
>>>> specific enough.
>>>>
>>>> In MemoryRegionOps we have a field valid with a callback accepts().
>>>>
>>>> I was wondering if doing the check in the accept() callback which is
>>>> called by the memory_region_access_valid() function and then using
>>>> max_access_size would not be cleaner.
>>>>
>>>> Note that it does not change a lot but only where the check is done.
>>>
>>> But where would we add those ops? My understanding is that pcistb acts
>>> on whatever region the device provided, and that differs from device to
>>> device?
>>>
>>>    
>>
>> The ops already exist, I thought adding a dedicated callback for s390 on
>> every regions used by vfio_pci instead of the default.
>> But it does not add a lot, just looks cleaner to me.
> 
> But we end up here for every pci device, not just for vfio devices,
> don't we?
> 
> 

Yes, but isn't what is done here?

-- 
Pierre Morel
IBM Lab Boeblingen