On Fri, 18 Dec 2020 17:40:50 +0100
Pierre Morel <pmo...@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 <pmo...@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 <pmo...@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 <mjros...@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?
