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