qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Pierre Morel
Subject: Re: [PATCH v2 2/2] s390x/pci: Fix memory_region_access_valid call
Date: Fri, 18 Dec 2020 17:40:50 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0



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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]