| Message ID | 1608243397-29428-3-git-send-email-mjrosato@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x/pci: some pcistb fixes | expand |
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>
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?
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.
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.
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?
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.
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?
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?
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>
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!
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)) {
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(-)