[PATCH v2 0/2] s390x/pci: some pcistb fixes

[PATCH v2 0/2] s390x/pci: some pcistb fixes

[Matthew Rosato]

Here are a few fixes pulled out of the 'Fixing s390 vfio-pci ISM support'
patchset.

v2:
- Changed loop pattern for patch 2.  @Thomas to be on the safe side I
didn't include your RB since I changed code, please have a look.

If there are further issues/comments I will address them after the
holidays, these aren't urgent fixes.  Thanks!

Matthew Rosato (2):
  s390x/pci: fix pcistb length
  s390x/pci: Fix memory_region_access_valid call

 hw/s390x/s390-pci-inst.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/2] s390x/pci: fix pcistb length

[Matthew Rosato]

In pcistb_service_call, we are grabbing 8 bits from a guest register to
indicate the length of the store operation -- but per the architecture
the length is actually defined by 13 bits of the guest register.

Fixes: 863f6f52b7 ("s390: implement pci instructions")
Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
---
 hw/s390x/s390-pci-inst.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index d9e1e29..e230293 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -755,7 +755,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
     int i;
     uint32_t fh;
     uint8_t pcias;
-    uint8_t len;
+    uint16_t len;
     uint8_t buffer[128];
 
     if (env->psw.mask & PSW_MASK_PSTATE) {
@@ -765,7 +765,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
 
     fh = env->regs[r1] >> 32;
     pcias = (env->regs[r1] >> 16) & 0xf;
-    len = env->regs[r1] & 0xff;
+    len = env->regs[r1] & 0x1fff;
     offset = env->regs[r3];
 
     if (!(fh & FH_MASK_ENABLE)) {
-- 
1.8.3.1

Re: [PATCH v2 1/2] s390x/pci: fix pcistb length

[Christian Borntraeger]

On 17.12.20 23:16, Matthew Rosato wrote:
> In pcistb_service_call, we are grabbing 8 bits from a guest register to
> indicate the length of the store operation -- but per the architecture
> the length is actually defined by 13 bits of the guest register.
> 
> Fixes: 863f6f52b7 ("s390: implement pci instructions")
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/s390x/s390-pci-inst.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index d9e1e29..e230293 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -755,7 +755,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>      int i;
>      uint32_t fh;
>      uint8_t pcias;
> -    uint8_t len;
> +    uint16_t len;
>      uint8_t buffer[128];
>  
>      if (env->psw.mask & PSW_MASK_PSTATE) {
> @@ -765,7 +765,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>  
>      fh = env->regs[r1] >> 32;
>      pcias = (env->regs[r1] >> 16) & 0xf;
> -    len = env->regs[r1] & 0xff;
> +    len = env->regs[r1] & 0x1fff;
>      offset = env->regs[r3];
>  
>      if (!(fh & FH_MASK_ENABLE)) {
> 

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

[Matthew Rosato]

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)) {
-- 
1.8.3.1

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

[Thomas Huth]

On 17/12/2020 23.16, 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)) {
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

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

[Pierre Morel]

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?

-- 
Pierre Morel
IBM Lab Boeblingen

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

[Cornelia Huck]

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.

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

[Pierre Morel]

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.


-- 
Pierre Morel
IBM Lab Boeblingen

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

[Cornelia Huck]

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?

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

[Pierre Morel]

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.


-- 
Pierre Morel
IBM Lab Boeblingen

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

[Cornelia Huck]

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?

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

[Pierre Morel]

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

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

[Pierre Morel]

On 12/18/20 6:05 PM, Pierre Morel wrote:
> 
> 
> 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>
>>>>>>>> ---

...snip...

>>>>
>>>
>>> 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?
> 

It was not my intention to slow the integration process.
We can start with this fix and eventually move the code to the callback 
in another series when/if we all agree.

Acked-by: Pierre Morel <pmorel@linux.ibm.com>



-- 
Pierre Morel
IBM Lab Boeblingen

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

[Cornelia Huck]

On Mon, 21 Dec 2020 09:50:23 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 12/18/20 6:05 PM, Pierre Morel wrote:
> > 
> > 
> > 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>
> >>>>>>>> ---  
> 
> ...snip...
> 
> >>>>  
> >>>
> >>> 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?
> >   
> 
> It was not my intention to slow the integration process.
> We can start with this fix and eventually move the code to the callback 
> in another series when/if we all agree.

Yeah, I also fear that we might have been talking past each other. It's
late in the year :)

> 
> Acked-by: Pierre Morel <pmorel@linux.ibm.com>

Thanks!

Re: [PATCH v2 0/2] s390x/pci: some pcistb fixes

[Cornelia Huck]

On Thu, 17 Dec 2020 17:16:35 -0500
Matthew Rosato <mjrosato@linux.ibm.com> wrote:

> Here are a few fixes pulled out of the 'Fixing s390 vfio-pci ISM support'
> patchset.
> 
> v2:
> - Changed loop pattern for patch 2.  @Thomas to be on the safe side I
> didn't include your RB since I changed code, please have a look.
> 
> If there are further issues/comments I will address them after the
> holidays, these aren't urgent fixes.  Thanks!
> 
> Matthew Rosato (2):
>   s390x/pci: fix pcistb length
>   s390x/pci: Fix memory_region_access_valid call
> 
>  hw/s390x/s390-pci-inst.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 

Thanks, applied.