[Qemu-devel] [PULL 0/5] alpha-softmmu fixes

[Qemu-devel] [PULL 0/5] alpha-softmmu fixes

[Richard Henderson]

The only non-alpha patch in the list has now been reviewed.
Please pull.


r~



The following changes since commit c3cb8e77804313e1be99b5f28a34a346736707a5:

  ioport: remove LITTLE_ENDIAN mark for portio (2013-07-12 14:37:47 -0500)

are available in the git repository at:

  git://github.com/rth7680/qemu.git axp-next

for you to fetch changes up to e605e969953d793036dbd450da6b59bb434fb3d8:

  hw/alpha: Use SRM epoch (2013-07-14 13:40:36 -0700)

----------------------------------------------------------------
Richard Henderson (5):
      hw/alpha: Don't use get_system_io
      hw/alpha: Don't machine check on missing pci i/o
      exec: Support 64-bit operations in address_space_rw
      hw/alpha: Drop latch_tmp hack
      hw/alpha: Use SRM epoch

 exec.c               | 68 ++++++++++++++++++++++++++++++++++++++++---------
 hw/alpha/alpha_sys.h |  2 +-
 hw/alpha/dp264.c     |  4 ++-
 hw/alpha/pci.c       | 44 ++++++++++----------------------
 hw/alpha/typhoon.c   | 72 +++++++++++++++-------------------------------------
 5 files changed, 93 insertions(+), 97 deletions(-)

[Qemu-devel] [PULL 1/5] hw/alpha: Don't use get_system_io

[Richard Henderson]

Advancements in the ioport subsystem mean that we need no longer
thunk memory-mapped i/o through the system-io address space.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 hw/alpha/alpha_sys.h |  1 -
 hw/alpha/pci.c       | 44 --------------------------------------------
 hw/alpha/typhoon.c   | 20 ++++++++------------
 3 files changed, 8 insertions(+), 57 deletions(-)

diff --git a/hw/alpha/alpha_sys.h b/hw/alpha/alpha_sys.h
index 50e7730..0987851 100644
--- a/hw/alpha/alpha_sys.h
+++ b/hw/alpha/alpha_sys.h
@@ -14,7 +14,6 @@ PCIBus *typhoon_init(ram_addr_t, ISABus **, qemu_irq *, AlphaCPU *[4],
                      pci_map_irq_fn);
 
 /* alpha_pci.c.  */
-extern const MemoryRegionOps alpha_pci_bw_io_ops;
 extern const MemoryRegionOps alpha_pci_conf1_ops;
 extern const MemoryRegionOps alpha_pci_iack_ops;
 
diff --git a/hw/alpha/pci.c b/hw/alpha/pci.c
index 7327d48..25637e0 100644
--- a/hw/alpha/pci.c
+++ b/hw/alpha/pci.c
@@ -12,50 +12,6 @@
 #include "sysemu/sysemu.h"
 
 
-/* PCI IO reads/writes, to byte-word addressable memory.  */
-/* ??? Doesn't handle multiple PCI busses.  */
-
-static uint64_t bw_io_read(void *opaque, hwaddr addr, unsigned size)
-{
-    switch (size) {
-    case 1:
-        return cpu_inb(addr);
-    case 2:
-        return cpu_inw(addr);
-    case 4:
-        return cpu_inl(addr);
-    }
-    abort();
-}
-
-static void bw_io_write(void *opaque, hwaddr addr,
-                        uint64_t val, unsigned size)
-{
-    switch (size) {
-    case 1:
-        cpu_outb(addr, val);
-        break;
-    case 2:
-        cpu_outw(addr, val);
-        break;
-    case 4:
-        cpu_outl(addr, val);
-        break;
-    default:
-        abort();
-    }
-}
-
-const MemoryRegionOps alpha_pci_bw_io_ops = {
-    .read = bw_io_read,
-    .write = bw_io_write,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-    .impl = {
-        .min_access_size = 1,
-        .max_access_size = 4,
-    },
-};
-
 /* PCI config space reads/writes, to byte-word addressable memory.  */
 static uint64_t bw_conf1_read(void *opaque, hwaddr addr,
                               unsigned size)
diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index 1c3ac8e..9dac9df 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -705,7 +705,6 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
     const uint64_t MB = 1024 * 1024;
     const uint64_t GB = 1024 * MB;
     MemoryRegion *addr_space = get_system_memory();
-    MemoryRegion *addr_space_io = get_system_io();
     DeviceState *dev;
     TyphoonState *s;
     PCIHostState *phb;
@@ -765,28 +764,25 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
                                 &s->pchip.reg_mem);
 
     /* Pchip0 PCI I/O, 0x801.FC00.0000, 32MB.  */
-    /* ??? Ideally we drop the "system" i/o space on the floor and give the
-       PCI subsystem the full address space reserved by the chipset.
-       We can't do that until the MEM and IO paths in memory.c are unified.  */
-    memory_region_init_io(&s->pchip.reg_io, OBJECT(s), &alpha_pci_bw_io_ops,
-                          NULL, "pci0-io", 32*MB);
+    memory_region_init(&s->pchip.reg_io, OBJECT(s), "pci0-io", 32*MB);
     memory_region_add_subregion(addr_space, 0x801fc000000ULL,
                                 &s->pchip.reg_io);
 
     b = pci_register_bus(dev, "pci",
                          typhoon_set_irq, sys_map_irq, s,
-                         &s->pchip.reg_mem, addr_space_io, 0, 64, TYPE_PCI_BUS);
+                         &s->pchip.reg_mem, &s->pchip.reg_io,
+                         0, 64, TYPE_PCI_BUS);
     phb->bus = b;
 
     /* Pchip0 PCI special/interrupt acknowledge, 0x801.F800.0000, 64MB.  */
-    memory_region_init_io(&s->pchip.reg_iack, OBJECT(s), &alpha_pci_iack_ops, b,
-                          "pci0-iack", 64*MB);
+    memory_region_init_io(&s->pchip.reg_iack, OBJECT(s), &alpha_pci_iack_ops,
+                          b, "pci0-iack", 64*MB);
     memory_region_add_subregion(addr_space, 0x801f8000000ULL,
                                 &s->pchip.reg_iack);
 
     /* Pchip0 PCI configuration, 0x801.FE00.0000, 16MB.  */
-    memory_region_init_io(&s->pchip.reg_conf, OBJECT(s), &alpha_pci_conf1_ops, b,
-                          "pci0-conf", 16*MB);
+    memory_region_init_io(&s->pchip.reg_conf, OBJECT(s), &alpha_pci_conf1_ops,
+                          b, "pci0-conf", 16*MB);
     memory_region_add_subregion(addr_space, 0x801fe000000ULL,
                                 &s->pchip.reg_conf);
 
@@ -804,7 +800,7 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
     {
         qemu_irq isa_pci_irq, *isa_irqs;
 
-        *isa_bus = isa_bus_new(NULL, addr_space_io);
+        *isa_bus = isa_bus_new(NULL, &s->pchip.reg_io);
         isa_pci_irq = *qemu_allocate_irqs(typhoon_set_isa_irq, s, 1);
         isa_irqs = i8259_init(*isa_bus, isa_pci_irq);
         isa_bus_irqs(*isa_bus, isa_irqs);
-- 
1.8.3.1

[Qemu-devel] [PULL 2/5] hw/alpha: Don't machine check on missing pci i/o

[Richard Henderson]

Not really correct, but we don't implement all of the random devices
that the kernel looks for.  This is good enough to keep us booting.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 hw/alpha/alpha_sys.h |  1 +
 hw/alpha/pci.c       | 26 ++++++++++++++++++++++++++
 hw/alpha/typhoon.c   |  3 ++-
 3 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/hw/alpha/alpha_sys.h b/hw/alpha/alpha_sys.h
index 0987851..e11025b 100644
--- a/hw/alpha/alpha_sys.h
+++ b/hw/alpha/alpha_sys.h
@@ -14,6 +14,7 @@ PCIBus *typhoon_init(ram_addr_t, ISABus **, qemu_irq *, AlphaCPU *[4],
                      pci_map_irq_fn);
 
 /* alpha_pci.c.  */
+extern const MemoryRegionOps alpha_pci_ignore_ops;
 extern const MemoryRegionOps alpha_pci_conf1_ops;
 extern const MemoryRegionOps alpha_pci_iack_ops;
 
diff --git a/hw/alpha/pci.c b/hw/alpha/pci.c
index 25637e0..d839dd5 100644
--- a/hw/alpha/pci.c
+++ b/hw/alpha/pci.c
@@ -12,6 +12,32 @@
 #include "sysemu/sysemu.h"
 
 
+/* Fallback for unassigned PCI I/O operations.  Avoids MCHK.  */
+
+static uint64_t ignore_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return 0;
+}
+
+static void ignore_write(void *opaque, hwaddr addr, uint64_t v, unsigned size)
+{
+}
+
+const MemoryRegionOps alpha_pci_ignore_ops = {
+    .read = ignore_read,
+    .write = ignore_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 8,
+    },
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 8,
+    },
+};
+
+
 /* PCI config space reads/writes, to byte-word addressable memory.  */
 static uint64_t bw_conf1_read(void *opaque, hwaddr addr,
                               unsigned size)
diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index 9dac9df..69e2255 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -764,7 +764,8 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus **isa_bus,
                                 &s->pchip.reg_mem);
 
     /* Pchip0 PCI I/O, 0x801.FC00.0000, 32MB.  */
-    memory_region_init(&s->pchip.reg_io, OBJECT(s), "pci0-io", 32*MB);
+    memory_region_init_io(&s->pchip.reg_io, OBJECT(s), &alpha_pci_ignore_ops,
+                          NULL, "pci0-io", 32*MB);
     memory_region_add_subregion(addr_space, 0x801fc000000ULL,
                                 &s->pchip.reg_io);
 
-- 
1.8.3.1

[Qemu-devel] [PULL 3/5] exec: Support 64-bit operations in address_space_rw

[Richard Henderson]

Honor the implementation maximum access size, and at least check
the minimum access size.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 exec.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 56 insertions(+), 12 deletions(-)

diff --git a/exec.c b/exec.c
index 80ee2ab..c99a883 100644
--- a/exec.c
+++ b/exec.c
@@ -1896,15 +1896,37 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
     return false;
 }
 
-static inline int memory_access_size(MemoryRegion *mr, int l, hwaddr addr)
+static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
 {
-    if (l >= 4 && (((addr & 3) == 0 || mr->ops->impl.unaligned))) {
-        return 4;
+    unsigned access_size_min = mr->ops->impl.min_access_size;
+    unsigned access_size_max = mr->ops->impl.max_access_size;
+
+    /* Regions are assumed to support 1-4 byte accesses unless
+       otherwise specified.  */
+    if (access_size_min == 0) {
+        access_size_min = 1;
+    }
+    if (access_size_max == 0) {
+        access_size_max = 4;
+    }
+
+    /* Bound the maximum access by the alignment of the address.  */
+    if (!mr->ops->impl.unaligned) {
+        unsigned align_size_max = addr & -addr;
+        if (align_size_max != 0 && align_size_max < access_size_max) {
+            access_size_max = align_size_max;
+        }
     }
-    if (l >= 2 && (((addr & 1) == 0) || mr->ops->impl.unaligned)) {
-        return 2;
+
+    /* Don't attempt accesses larger than the maximum.  */
+    if (l > access_size_max) {
+        l = access_size_max;
     }
-    return 1;
+    /* ??? The users of this function are wrong, not supporting minimums larger
+       than the remaining length.  C.f. memory.c:access_with_adjusted_size.  */
+    assert(l >= access_size_min);
+
+    return l;
 }
 
 bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
@@ -1926,18 +1948,29 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
                 l = memory_access_size(mr, l, addr1);
                 /* XXX: could force current_cpu to NULL to avoid
                    potential bugs */
-                if (l == 4) {
+                switch (l) {
+                case 8:
+                    /* 64 bit write access */
+                    val = ldq_p(buf);
+                    error |= io_mem_write(mr, addr1, val, 8);
+                    break;
+                case 4:
                     /* 32 bit write access */
                     val = ldl_p(buf);
                     error |= io_mem_write(mr, addr1, val, 4);
-                } else if (l == 2) {
+                    break;
+                case 2:
                     /* 16 bit write access */
                     val = lduw_p(buf);
                     error |= io_mem_write(mr, addr1, val, 2);
-                } else {
+                    break;
+                case 1:
                     /* 8 bit write access */
                     val = ldub_p(buf);
                     error |= io_mem_write(mr, addr1, val, 1);
+                    break;
+                default:
+                    abort();
                 }
             } else {
                 addr1 += memory_region_get_ram_addr(mr);
@@ -1950,18 +1983,29 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
             if (!memory_access_is_direct(mr, is_write)) {
                 /* I/O case */
                 l = memory_access_size(mr, l, addr1);
-                if (l == 4) {
+                switch (l) {
+                case 8:
+                    /* 64 bit read access */
+                    error |= io_mem_read(mr, addr1, &val, 8);
+                    stq_p(buf, val);
+                    break;
+                case 4:
                     /* 32 bit read access */
                     error |= io_mem_read(mr, addr1, &val, 4);
                     stl_p(buf, val);
-                } else if (l == 2) {
+                    break;
+                case 2:
                     /* 16 bit read access */
                     error |= io_mem_read(mr, addr1, &val, 2);
                     stw_p(buf, val);
-                } else {
+                    break;
+                case 1:
                     /* 8 bit read access */
                     error |= io_mem_read(mr, addr1, &val, 1);
                     stb_p(buf, val);
+                    break;
+                default:
+                    abort();
                 }
             } else {
                 /* RAM case */
-- 
1.8.3.1

[Qemu-devel] [PULL 4/5] hw/alpha: Drop latch_tmp hack

[Richard Henderson]

The memory and i/o core now support passing 64-bit accesses along
from the guest, so we no longer need to emulate them.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 hw/alpha/typhoon.c | 53 ++++++++++++-----------------------------------------
 1 file changed, 12 insertions(+), 41 deletions(-)

diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index 69e2255..3d7a1cd 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -51,9 +51,6 @@ typedef struct TyphoonState {
     TyphoonPchip pchip;
     MemoryRegion dchip_region;
     MemoryRegion ram_region;
-
-    /* QEMU emulation state.  */
-    uint32_t latch_tmp;
 } TyphoonState;
 
 /* Called when one of DRIR or DIM changes.  */
@@ -76,10 +73,6 @@ static uint64_t cchip_read(void *opaque, hwaddr addr, unsigned size)
     TyphoonState *s = opaque;
     uint64_t ret = 0;
 
-    if (addr & 4) {
-        return s->latch_tmp;
-    }
-
     switch (addr) {
     case 0x0000:
         /* CSC: Cchip System Configuration Register.  */
@@ -199,7 +192,6 @@ static uint64_t cchip_read(void *opaque, hwaddr addr, unsigned size)
         return -1;
     }
 
-    s->latch_tmp = ret >> 32;
     return ret;
 }
 
@@ -214,10 +206,6 @@ static uint64_t pchip_read(void *opaque, hwaddr addr, unsigned size)
     TyphoonState *s = opaque;
     uint64_t ret = 0;
 
-    if (addr & 4) {
-        return s->latch_tmp;
-    }
-
     switch (addr) {
     case 0x0000:
         /* WSBA0: Window Space Base Address Register.  */
@@ -302,23 +290,14 @@ static uint64_t pchip_read(void *opaque, hwaddr addr, unsigned size)
         return -1;
     }
 
-    s->latch_tmp = ret >> 32;
     return ret;
 }
 
 static void cchip_write(void *opaque, hwaddr addr,
-                        uint64_t v32, unsigned size)
+                        uint64_t val, unsigned size)
 {
     TyphoonState *s = opaque;
-    uint64_t val, oldval, newval;
-
-    if (addr & 4) {
-        val = v32 << 32 | s->latch_tmp;
-        addr ^= 4;
-    } else {
-        s->latch_tmp = v32;
-        return;
-    }
+    uint64_t oldval, newval;
 
     switch (addr) {
     case 0x0000:
@@ -471,18 +450,10 @@ static void dchip_write(void *opaque, hwaddr addr,
 }
 
 static void pchip_write(void *opaque, hwaddr addr,
-                        uint64_t v32, unsigned size)
+                        uint64_t val, unsigned size)
 {
     TyphoonState *s = opaque;
-    uint64_t val, oldval;
-
-    if (addr & 4) {
-        val = v32 << 32 | s->latch_tmp;
-        addr ^= 4;
-    } else {
-        s->latch_tmp = v32;
-        return;
-    }
+    uint64_t oldval;
 
     switch (addr) {
     case 0x0000:
@@ -585,12 +556,12 @@ static const MemoryRegionOps cchip_ops = {
     .write = cchip_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .valid = {
-        .min_access_size = 4,  /* ??? Should be 8.  */
+        .min_access_size = 8,
         .max_access_size = 8,
     },
     .impl = {
-        .min_access_size = 4,
-        .max_access_size = 4,
+        .min_access_size = 8,
+        .max_access_size = 8,
     },
 };
 
@@ -599,11 +570,11 @@ static const MemoryRegionOps dchip_ops = {
     .write = dchip_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .valid = {
-        .min_access_size = 4,  /* ??? Should be 8.  */
+        .min_access_size = 8,
         .max_access_size = 8,
     },
     .impl = {
-        .min_access_size = 4,
+        .min_access_size = 8,
         .max_access_size = 8,
     },
 };
@@ -613,12 +584,12 @@ static const MemoryRegionOps pchip_ops = {
     .write = pchip_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .valid = {
-        .min_access_size = 4,  /* ??? Should be 8.  */
+        .min_access_size = 8,
         .max_access_size = 8,
     },
     .impl = {
-        .min_access_size = 4,
-        .max_access_size = 4,
+        .min_access_size = 8,
+        .max_access_size = 8,
     },
 };
 
-- 
1.8.3.1

[Qemu-devel] [PULL 5/5] hw/alpha: Use SRM epoch

[Richard Henderson]

The 1980 epoch is used by the ARC PALcode for NT.  But we're emulating
a system using the SRM PALcode.  Using the proper epoch results in less
confusion in the guest userland.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 hw/alpha/dp264.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 8dad08f..95fde61 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -73,7 +73,9 @@ static void clipper_init(QEMUMachineInitArgs *args)
     pci_bus = typhoon_init(ram_size, &isa_bus, &rtc_irq, cpus,
                            clipper_pci_map_irq);
 
-    rtc_init(isa_bus, 1980, rtc_irq);
+    /* Since we have an SRM-compatible PALcode, use the SRM epoch.  */
+    rtc_init(isa_bus, 1900, rtc_irq);
+
     pit_init(isa_bus, 0x40, 0, NULL);
     isa_create_simple(isa_bus, "i8042");
 
-- 
1.8.3.1

Re: [Qemu-devel] [PULL 3/5] exec: Support 64-bit operations in address_space_rw

[Markus Armbruster]

Richard Henderson <rth@twiddle.net> writes:

> Honor the implementation maximum access size, and at least check
> the minimum access size.
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Richard Henderson <rth@twiddle.net>

Fails for me:

qemu-system-x86_64: /work/armbru/qemu/exec.c:1927: memory_access_size: Assertion `l >= access_size_min' failed.

Workaround: disable USB.

Backtrace:

#3  0x0000003707a2e752 in __assert_fail () from /lib64/libc.so.6
#4  0x00000000006951e3 in memory_access_size (mr=0x1116c00, l=1, addr=12)
    at /work/armbru/qemu/exec.c:1927
#5  0x000000000069529d in address_space_rw (as=0x1045920, addr=49228, buf=
    0x7ffff5392af8 "@\240\377\367L\300", len=1, is_write=true)
    at /work/armbru/qemu/exec.c:1948
#6  0x000000000069566b in address_space_write (as=0x1045920, addr=49228, buf=
    0x7ffff5392af8 "@\240\377\367L\300", len=1)
    at /work/armbru/qemu/exec.c:2027
#7  0x0000000000701356 in cpu_outb (addr=49228, val=64 '@')
    at /work/armbru/qemu/ioport.c:51
#8  0x0000000000705ab1 in kvm_handle_io (port=49228, data=0x7ffff7ffa000, 
    direction=1, size=1, count=1) at /work/armbru/qemu/kvm-all.c:1517
#9  0x000000000070606f in kvm_cpu_exec (cpu=0x10d3c90)
    at /work/armbru/qemu/kvm-all.c:1664
#10 0x0000000000687da7 in qemu_kvm_cpu_thread_fn (arg=0x10d3c90)
    at /work/armbru/qemu/cpus.c:751
#11 0x0000003708607d14 in start_thread () from /lib64/libpthread.so.0
#12 0x0000003707af168d in clone () from /lib64/libc.so.6

Hopefully useful state:

(gdb) p *mr
$1 = {ops = 0x8b0a20, iommu_ops = 0x0, opaque = 0x1116450, owner = 0x0, 
  parent = 0x10c3a10, size = {lo = 32, hi = 0}, addr = 49216, destructor = 
    0x70b2a4 <memory_region_destructor_none>, ram_addr = 18446744073709551615, 
  subpage = false, terminates = true, romd_mode = true, ram = false, 
  readonly = false, enabled = true, rom_device = false, warning_printed = 
    false, flush_coalesced_mmio = false, alias = 0x0, alias_offset = 0, 
  priority = 1, may_overlap = true, subregions = {tqh_first = 0x0, tqh_last = 
    0x1116c78}, subregions_link = {tqe_next = 0x111b2e8, tqe_prev = 
    0x7ffff4a9c928}, coalesced = {tqh_first = 0x0, tqh_last = 0x1116c98}, 
  name = 0x11174e0 "uhci", dirty_log_mask = 0 '\000', ioeventfd_nb = 0, 
  ioeventfds = 0x0, iommu_notify = {notifiers = {lh_first = 0x0}}}
(gdb) p *mr->ops
$2 = {read = 0x5becdf <uhci_port_read>, write = 0x5be8cd <uhci_port_write>, 
  endianness = DEVICE_LITTLE_ENDIAN, valid = {min_access_size = 1, 
    max_access_size = 4, unaligned = false, accepts = 0}, impl = {
    min_access_size = 2, max_access_size = 2, unaligned = false}, old_mmio = {
    read = {0, 0, 0}, write = {0, 0, 0}}}

info mtree
memory
0000000000000000-7ffffffffffffffe (prio 0, RW): system
  0000000000000000-000000001fffffff (prio 0, RW): alias ram-below-4g @pc.ram 0000000000000000-000000001fffffff
  00000000000a0000-00000000000bffff (prio 1, RW): alias smram-region @pci 00000000000a0000-00000000000bffff
  00000000000c0000-00000000000c3fff (prio 1, RW): alias pam-pci @pci 00000000000c0000-00000000000c3fff
  00000000000c4000-00000000000c7fff (prio 1, RW): alias pam-pci @pci 00000000000c4000-00000000000c7fff
  00000000000c8000-00000000000cbfff (prio 1, RW): alias pam-pci @pci 00000000000c8000-00000000000cbfff
  00000000000cc000-00000000000cffff (prio 1, RW): alias pam-pci @pci 00000000000cc000-00000000000cffff
  00000000000d0000-00000000000d3fff (prio 1, RW): alias pam-pci @pci 00000000000d0000-00000000000d3fff
  00000000000d4000-00000000000d7fff (prio 1, RW): alias pam-pci @pci 00000000000d4000-00000000000d7fff
  00000000000d8000-00000000000dbfff (prio 1, RW): alias pam-pci @pci 00000000000d8000-00000000000dbfff
  00000000000dc000-00000000000dffff (prio 1, RW): alias pam-pci @pci 00000000000dc000-00000000000dffff
  00000000000e0000-00000000000e3fff (prio 1, RW): alias pam-pci @pci 00000000000e0000-00000000000e3fff
  00000000000e4000-00000000000e7fff (prio 1, RW): alias pam-pci @pci 00000000000e4000-00000000000e7fff
  00000000000e8000-00000000000ebfff (prio 1, RW): alias pam-pci @pci 00000000000e8000-00000000000ebfff
  00000000000ec000-00000000000effff (prio 1, RW): alias pam-pci @pci 00000000000ec000-00000000000effff
  00000000000f0000-00000000000fffff (prio 1, RW): alias pam-pci @pci 00000000000f0000-00000000000fffff
  0000000020000000-00000000ffffffff (prio 0, RW): alias pci-hole @pci 0000000020000000-00000000ffffffff
  00000000fec00000-00000000fec00fff (prio 0, RW): kvm-ioapic
  00000000fed00000-00000000fed003ff (prio 0, RW): hpet
  00000000fee00000-00000000feefffff (prio 4096, RW): icc-apic-container
    00000000fee00000-00000000feefffff (prio 0, RW): kvm-apic-msi
  0000000100000000-40000000ffffffff (prio 0, RW): alias pci-hole64 @pci 0000000100000000-40000000ffffffff
I/O
0000000000000000-000000000000ffff (prio 0, RW): io
  0000000000000000-0000000000000007 (prio 0, RW): dma-chan
  0000000000000008-000000000000000f (prio 0, RW): dma-cont
  0000000000000020-0000000000000021 (prio 0, RW): kvm-pic
  0000000000000040-0000000000000043 (prio 0, RW): kvm-pit
  0000000000000060-0000000000000060 (prio 0, RW): i8042-data
  0000000000000061-0000000000000061 (prio 0, RW): elcr
  0000000000000064-0000000000000064 (prio 0, RW): i8042-cmd
  0000000000000070-0000000000000071 (prio 0, RW): rtc
  000000000000007e-000000000000007f (prio 0, RW): kvmvapic
  0000000000000080-0000000000000080 (prio 0, RW): ioport80
  0000000000000081-0000000000000083 (prio 0, RW): dma-page
  0000000000000087-0000000000000087 (prio 0, RW): dma-page
  0000000000000089-000000000000008b (prio 0, RW): dma-page
  000000000000008f-000000000000008f (prio 0, RW): dma-page
  0000000000000092-0000000000000092 (prio 0, RW): port92
  00000000000000a0-00000000000000a1 (prio 0, RW): kvm-pic
  00000000000000b2-00000000000000b3 (prio 0, RW): apm-io
  00000000000000c0-00000000000000cf (prio 0, RW): dma-chan
  00000000000000d0-00000000000000df (prio 0, RW): dma-cont
  00000000000000f0-00000000000000f0 (prio 0, RW): ioportF0
  0000000000000170-0000000000000177 (prio 0, RW): ide
  00000000000001f0-00000000000001f7 (prio 0, RW): ide
  0000000000000376-0000000000000376 (prio 0, RW): ide
  00000000000003b0-00000000000003df (prio 0, RW): cirrus-io
  00000000000003f1-00000000000003f5 (prio 0, RW): fdc
  00000000000003f6-00000000000003f6 (prio 0, RW): ide
  00000000000003f7-00000000000003f7 (prio 0, RW): fdc
  00000000000003f8-00000000000003ff (prio 0, RW): serial
  00000000000004d0-00000000000004d0 (prio 0, RW): kvm-elcr
  00000000000004d1-00000000000004d1 (prio 0, RW): kvm-elcr
  0000000000000505-0000000000000505 (prio 0, RW): pvpanic
  0000000000000510-0000000000000511 (prio 0, RW): fwcfg
  0000000000000cf8-0000000000000cfb (prio 0, RW): pci-conf-idx
  0000000000000cf9-0000000000000cf9 (prio 1, RW): piix3-reset-control
  0000000000000cfc-0000000000000cff (prio 0, RW): pci-conf-data
  0000000000005658-0000000000005658 (prio 0, RW): vmport
  000000000000ae00-000000000000ae0e (prio 0, RW): acpi-pci-hotplug
  000000000000af00-000000000000af1f (prio 0, RW): acpi-cpu-hotplug
  000000000000afe0-000000000000afe3 (prio 0, RW): acpi-gpe0
  000000000000b100-000000000000b13f (prio 0, RW): pm-smbus
i440FX
PIIX3
piix3-ide
piix3-usb-uhci
PIIX4_PM
cirrus-vga
e1000
aliases
pc.ram
0000000000000000-000000001fffffff (prio 0, RW): pc.ram
pci
0000000000000000-7ffffffffffffffe (prio 0, RW): pci
  00000000000a0000-00000000000bffff (prio 1, RW): cirrus-lowmem-container
    00000000000a0000-00000000000bffff (prio 0, RW): cirrus-low-memory
  00000000000c0000-00000000000dffff (prio 1, RW): pc.rom
  00000000000e0000-00000000000fffff (prio 1, R-): alias isa-bios @pc.bios 0000000000000000-000000000001ffff
  00000000fffe0000-00000000ffffffff (prio 0, R-): pc.bios
pc.bios
00000000fffe0000-00000000ffffffff (prio 0, R-): pc.bios

Re: [Qemu-devel] [PULL 3/5] exec: Support 64-bit operations in address_space_rw

[Paolo Bonzini]

Il 17/07/2013 11:50, Markus Armbruster ha scritto:
> Richard Henderson <rth@twiddle.net> writes:
> 
>> Honor the implementation maximum access size, and at least check
>> the minimum access size.
>>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Richard Henderson <rth@twiddle.net>
> 
> Fails for me:
> 
> qemu-system-x86_64: /work/armbru/qemu/exec.c:1927: memory_access_size: Assertion `l >= access_size_min' failed.

This:

    unsigned access_size_min = mr->ops->impl.min_access_size;
    unsigned access_size_max = mr->ops->impl.max_access_size;

must be respectively:

    unsigned access_size_min = 1;
    unsigned access_size_max = mr->ops->valid.max_access_size;

access_size_min can be 1 because erroneous accesses must not crash 
QEMU, they should trigger exceptions in the guest or just return 
garbage (depending on the CPU).  I'm not sure I understand the comment, 
placing a 4-byte field at the last byte of a region makes no sense 
(unless impl.unaligned is true).

access_size_max can be mr->ops->valid.max_access_size because memory.c 
can and will still break accesses bigger than 
mr->ops->impl.max_access_size.

Markus, can you try the minimal patch above?  Or this one that also
does the consequent simplifications.

diff --git a/exec.c b/exec.c
index c99a883..0904283 100644
--- a/exec.c
+++ b/exec.c
@@ -1898,14 +1898,8 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
 
 static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
 {
-    unsigned access_size_min = mr->ops->impl.min_access_size;
-    unsigned access_size_max = mr->ops->impl.max_access_size;
+    unsigned access_size_max = mr->ops->valid.max_access_size;
 
-    /* Regions are assumed to support 1-4 byte accesses unless
-       otherwise specified.  */
-    if (access_size_min == 0) {
-        access_size_min = 1;
-    }
     if (access_size_max == 0) {
         access_size_max = 4;
     }
@@ -1922,9 +1916,6 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
     if (l > access_size_max) {
         l = access_size_max;
     }
-    /* ??? The users of this function are wrong, not supporting minimums larger
-       than the remaining length.  C.f. memory.c:access_with_adjusted_size.  */
-    assert(l >= access_size_min);
 
     return l;
 }

Paolo

Re: [Qemu-devel] [PULL 3/5] exec: Support 64-bit operations in address_space_rw

[Richard Henderson]

On 07/17/2013 04:09 AM, Paolo Bonzini wrote:
>>
>> Fails for me:
>>
>> qemu-system-x86_64: /work/armbru/qemu/exec.c:1927: memory_access_size: Assertion `l >= access_size_min' failed.
> 
> This:
> 
>     unsigned access_size_min = mr->ops->impl.min_access_size;
>     unsigned access_size_max = mr->ops->impl.max_access_size;
> 
> must be respectively:
> 
>     unsigned access_size_min = 1;
>     unsigned access_size_max = mr->ops->valid.max_access_size;
> 
> access_size_min can be 1 because erroneous accesses must not crash 
> QEMU, they should trigger exceptions in the guest or just return 
> garbage (depending on the CPU).  I'm not sure I understand the comment, 
> placing a 4-byte field at the last byte of a region makes no sense 
> (unless impl.unaligned is true).
> 
> access_size_max can be mr->ops->valid.max_access_size because memory.c 
> can and will still break accesses bigger than 
> mr->ops->impl.max_access_size.
> 
> Markus, can you try the minimal patch above?  Or this one that also
> does the consequent simplifications.

NAK.

If you remove the check here, you're just trading it for one in the device.
The device told you that it can't support a 1 byte read.  (Either that, or the
device incorrectly reported what it can actually do.)

The proper fix is to change the interface of memory_access_size such that it
can report errors.  Indeed, very likely we should change it and its callers to
also support over-sized reads, like access_with_adjusted_size in memory.c.


r~

Re: [Qemu-devel] [PULL 3/5] exec: Support 64-bit operations in address_space_rw

[Paolo Bonzini]

Il 17/07/2013 15:23, Richard Henderson ha scritto:
> On 07/17/2013 04:09 AM, Paolo Bonzini wrote:
>>>
>>> Fails for me:
>>>
>>> qemu-system-x86_64: /work/armbru/qemu/exec.c:1927: memory_access_size: Assertion `l >= access_size_min' failed.
>>
>> This:
>>
>>     unsigned access_size_min = mr->ops->impl.min_access_size;
>>     unsigned access_size_max = mr->ops->impl.max_access_size;
>>
>> must be respectively:
>>
>>     unsigned access_size_min = 1;
>>     unsigned access_size_max = mr->ops->valid.max_access_size;
>>
>> access_size_min can be 1 because erroneous accesses must not crash 
>> QEMU, they should trigger exceptions in the guest or just return 
>> garbage (depending on the CPU).  I'm not sure I understand the comment, 
>> placing a 4-byte field at the last byte of a region makes no sense 
>> (unless impl.unaligned is true).
>>
>> access_size_max can be mr->ops->valid.max_access_size because memory.c 
>> can and will still break accesses bigger than 
>> mr->ops->impl.max_access_size.
>>
>> Markus, can you try the minimal patch above?  Or this one that also
>> does the consequent simplifications.
> 
> NAK.
> 
> If you remove the check here, you're just trading it for one in the device.
> The device told you that it can't support a 1 byte read.  (Either that, or the
> device incorrectly reported what it can actually do.)

There are two parts to this.

First of all, mr->ops->impl.min_access_size is definitely wrong.  The
device told me that the MMIO functions only know about 2-byte accesses,
but that it _can_ support 1-, 2- and 4- byte reads (with coalescing done
by memory.c).  So I could change access_size_min to
mr->ops->valid.min_access_size, which would also fix Markus's problem.

But then, accesses smaller than mr->ops->valid.min_access_size are fine,
they just result in exceptions or garbage reads (depending on the CPU).
 address_space_rw reports these errors just fine,  memory_access_size's
only purpose is to split address_space_rw's MMIO writes in a sensible
manner.  There is no error reporting because it is done in memory.c.

In fact, I'm not even sure if users of memory_access_size (DMA to an
MMIO destination) exist in real hardware.  I'm curious if "BSAVE"ing
16-color EGA graphics works with a modern graphic card and a BIOS that
doesn't use PIO.

Paolo

> The proper fix is to change the interface of memory_access_size such that it
> can report errors.  Indeed, very likely we should change it and its callers to
> also support over-sized reads, like access_with_adjusted_size in memory.c.
> 
> 
> r~
> 

Re: [Qemu-devel] [PULL 3/5] exec: Support 64-bit operations in address_space_rw

[Richard Henderson]

On 07/17/2013 06:45 AM, Paolo Bonzini wrote:
>> NAK.
>>
>> If you remove the check here, you're just trading it for one in the device.
>> The device told you that it can't support a 1 byte read.  (Either that, or the
>> device incorrectly reported what it can actually do.)
> 
> There are two parts to this.
> 
> First of all, mr->ops->impl.min_access_size is definitely wrong.  The
> device told me that the MMIO functions only know about 2-byte accesses,
> but that it _can_ support 1-, 2- and 4- byte reads (with coalescing done
> by memory.c). 

I don't know enough about the specific device (or even which device it was)
to know whether the IMPL and VALID fields are correct.

> So I could change access_size_min to
> mr->ops->valid.min_access_size, which would also fix Markus's problem.

No, you can't.  At least not without changing all of the callers.

If you do as you suggest, the callers will invoke the device with a value of
SIZE that is illegal according to IMPL.  We might as well crash now than later.

There are three possible solutions:

(1) Return an error from memory_access_size, change the callers to propagate
    the error in some fashion.  This isn't ideal, since in this case VALID
    indicates that the guest access is correct.

(2) Return the implementation minimum, change the callers to interact with
    the device using that minimum.  With this scenario, we should likely
    share code with access_with_adjusted_size.

(3) Determine that the device's impl.min_access_size is wrong and adjust it.

Responding to your earlier

> erroneous accesses must not crash 
> QEMU, they should trigger exceptions in the guest or just return 
> garbage (depending on the CPU).

I completely agree -- if we were talking about VALID.  Since this is IMPL, it's
not an "erroneous access", but rather QEMU not being self-consistent.
And for internal logic errors, we've got asserts and aborts all over.


r~

Re: [Qemu-devel] [PULL 3/5] exec: Support 64-bit operations in address_space_rw

[Paolo Bonzini]

Il 17/07/2013 16:29, Richard Henderson ha scritto:
> On 07/17/2013 06:45 AM, Paolo Bonzini wrote:
>>> NAK.
>>>
>>> If you remove the check here, you're just trading it for one in the device.
>>> The device told you that it can't support a 1 byte read.  (Either that, or the
>>> device incorrectly reported what it can actually do.)
>>
>> There are two parts to this.
>>
>> First of all, mr->ops->impl.min_access_size is definitely wrong.  The
>> device told me that the MMIO functions only know about 2-byte accesses,
>> but that it _can_ support 1-, 2- and 4- byte reads (with coalescing done
>> by memory.c). 
> 
> I don't know enough about the specific device (or even which device it was)
> to know whether the IMPL and VALID fields are correct.

They are correct.  The device was usb-uhci, FWIW.

>> So I could change access_size_min to
>> mr->ops->valid.min_access_size, which would also fix Markus's problem.
> 
> No, you can't.  At least not without changing all of the callers.
> 
> If you do as you suggest, the callers will invoke the device with a value of
> SIZE that is illegal according to IMPL.  We might as well crash now than later.

No, it won't.  access_with_adjusted_size will take care of taking a size
that IMPL rejects, and producing one or more accesses in a size that
IMPL accepts.

Now of course access_with_adjusted_size may have bugs handling
misaligned addresses.  That's possible.

> There are three possible solutions:
> 
> (1) Return an error from memory_access_size, change the callers to propagate
>     the error in some fashion.  This isn't ideal, since in this case VALID
>     indicates that the guest access is correct.

Agreed.

> (2) Return the implementation minimum, change the callers to interact with
>     the device using that minimum.  With this scenario, we should likely
>     share code with access_with_adjusted_size.

I think you misunderstand what the impl.*_access_size are.
impl.min/max_access_size is a private interface between the device and
memory.c, to avoid having code all over the place to combine/split MMIO
accesses.  The public interface of the device is valid.*_access_size.

>> erroneous accesses must not crash 
>> QEMU, they should trigger exceptions in the guest or just return 
>> garbage (depending on the CPU).
> 
> I completely agree -- if we were talking about VALID.  Since this is IMPL, it's
> not an "erroneous access", but rather QEMU not being self-consistent.

Actually, no, for two reasons:

- address_space_rw memory accesses are exactly the same as memory
accesses started by the guest.  In many cases, they use addr/range pairs
passed directly by the guest.  It is not acceptable to crash on these.

- as said above, impl.*_access_size is not visible outside the device
itself, the public interface of the device is valid.*_access_size.

Paolo

Re: [Qemu-devel] [PULL 3/5] exec: Support 64-bit operations in address_space_rw

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 17/07/2013 11:50, Markus Armbruster ha scritto:
>> Richard Henderson <rth@twiddle.net> writes:
>> 
>>> Honor the implementation maximum access size, and at least check
>>> the minimum access size.
>>>
>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Signed-off-by: Richard Henderson <rth@twiddle.net>
>> 
>> Fails for me:
>> 
>> qemu-system-x86_64: /work/armbru/qemu/exec.c:1927: memory_access_size: Assertion `l >= access_size_min' failed.
>
> This:
>
>     unsigned access_size_min = mr->ops->impl.min_access_size;
>     unsigned access_size_max = mr->ops->impl.max_access_size;
>
> must be respectively:
>
>     unsigned access_size_min = 1;
>     unsigned access_size_max = mr->ops->valid.max_access_size;
>
> access_size_min can be 1 because erroneous accesses must not crash 
> QEMU, they should trigger exceptions in the guest or just return 
> garbage (depending on the CPU).  I'm not sure I understand the comment, 
> placing a 4-byte field at the last byte of a region makes no sense 
> (unless impl.unaligned is true).
>
> access_size_max can be mr->ops->valid.max_access_size because memory.c 
> can and will still break accesses bigger than 
> mr->ops->impl.max_access_size.
>
> Markus, can you try the minimal patch above?  Or this one that also
> does the consequent simplifications.

FYI, the reproducer is very simple:

qemu-system-x86_64 -usb

Regards,

Anthony Liguori

>
> diff --git a/exec.c b/exec.c
> index c99a883..0904283 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1898,14 +1898,8 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
>  
>  static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
>  {
> -    unsigned access_size_min = mr->ops->impl.min_access_size;
> -    unsigned access_size_max = mr->ops->impl.max_access_size;
> +    unsigned access_size_max = mr->ops->valid.max_access_size;
>  
> -    /* Regions are assumed to support 1-4 byte accesses unless
> -       otherwise specified.  */
> -    if (access_size_min == 0) {
> -        access_size_min = 1;
> -    }
>      if (access_size_max == 0) {
>          access_size_max = 4;
>      }
> @@ -1922,9 +1916,6 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
>      if (l > access_size_max) {
>          l = access_size_max;
>      }
> -    /* ??? The users of this function are wrong, not supporting minimums larger
> -       than the remaining length.  C.f. memory.c:access_with_adjusted_size.  */
> -    assert(l >= access_size_min);
>  
>      return l;
>  }
>
> Paolo

Re: [Qemu-devel] [PULL 3/5] exec: Support 64-bit operations in address_space_rw

[Paolo Bonzini]

Il 17/07/2013 17:50, Anthony Liguori ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Il 17/07/2013 11:50, Markus Armbruster ha scritto:
>>> Richard Henderson <rth@twiddle.net> writes:
>>>
>>>> Honor the implementation maximum access size, and at least check
>>>> the minimum access size.
>>>>
>>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> Signed-off-by: Richard Henderson <rth@twiddle.net>
>>>
>>> Fails for me:
>>>
>>> qemu-system-x86_64: /work/armbru/qemu/exec.c:1927: memory_access_size: Assertion `l >= access_size_min' failed.
>>
>> This:
>>
>>     unsigned access_size_min = mr->ops->impl.min_access_size;
>>     unsigned access_size_max = mr->ops->impl.max_access_size;
>>
>> must be respectively:
>>
>>     unsigned access_size_min = 1;
>>     unsigned access_size_max = mr->ops->valid.max_access_size;
>>
>> access_size_min can be 1 because erroneous accesses must not crash 
>> QEMU, they should trigger exceptions in the guest or just return 
>> garbage (depending on the CPU).  I'm not sure I understand the comment, 
>> placing a 4-byte field at the last byte of a region makes no sense 
>> (unless impl.unaligned is true).
>>
>> access_size_max can be mr->ops->valid.max_access_size because memory.c 
>> can and will still break accesses bigger than 
>> mr->ops->impl.max_access_size.
>>
>> Markus, can you try the minimal patch above?  Or this one that also
>> does the consequent simplifications.
> 
> FYI, the reproducer is very simple:
> 
> qemu-system-x86_64 -usb

My patch works.

Paolo

> Regards,
> 
> Anthony Liguori
> 
>>
>> diff --git a/exec.c b/exec.c
>> index c99a883..0904283 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -1898,14 +1898,8 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
>>  
>>  static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
>>  {
>> -    unsigned access_size_min = mr->ops->impl.min_access_size;
>> -    unsigned access_size_max = mr->ops->impl.max_access_size;
>> +    unsigned access_size_max = mr->ops->valid.max_access_size;
>>  
>> -    /* Regions are assumed to support 1-4 byte accesses unless
>> -       otherwise specified.  */
>> -    if (access_size_min == 0) {
>> -        access_size_min = 1;
>> -    }
>>      if (access_size_max == 0) {
>>          access_size_max = 4;
>>      }
>> @@ -1922,9 +1916,6 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
>>      if (l > access_size_max) {
>>          l = access_size_max;
>>      }
>> -    /* ??? The users of this function are wrong, not supporting minimums larger
>> -       than the remaining length.  C.f. memory.c:access_with_adjusted_size.  */
>> -    assert(l >= access_size_min);
>>  
>>      return l;
>>  }
>>
>> Paolo
> 

Re: [Qemu-devel] [PULL 3/5] exec: Support 64-bit operations in address_space_rw

[Richard Henderson]

On 07/17/2013 10:32 AM, Paolo Bonzini wrote:
> My patch works.

You patch doesn't crash for this device, which isn't quite the same thing.

But it's certainly no worse than we had before my patch, so I'll not object so
long as a fixme sort of comment is installed too.


r~

Re: [Qemu-devel] [PULL 3/5] exec: Support 64-bit operations in address_space_rw

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 17/07/2013 17:50, Anthony Liguori ha scritto:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> Il 17/07/2013 11:50, Markus Armbruster ha scritto:
>>>> Richard Henderson <rth@twiddle.net> writes:
>>>>
>>>>> Honor the implementation maximum access size, and at least check
>>>>> the minimum access size.
>>>>>
>>>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>>>> Signed-off-by: Richard Henderson <rth@twiddle.net>
>>>>
>>>> Fails for me:
>>>>
>>>> qemu-system-x86_64: /work/armbru/qemu/exec.c:1927: memory_access_size: Assertion `l >= access_size_min' failed.
>>>
>>> This:
>>>
>>>     unsigned access_size_min = mr->ops->impl.min_access_size;
>>>     unsigned access_size_max = mr->ops->impl.max_access_size;
>>>
>>> must be respectively:
>>>
>>>     unsigned access_size_min = 1;
>>>     unsigned access_size_max = mr->ops->valid.max_access_size;
>>>
>>> access_size_min can be 1 because erroneous accesses must not crash 
>>> QEMU, they should trigger exceptions in the guest or just return 
>>> garbage (depending on the CPU).  I'm not sure I understand the comment, 
>>> placing a 4-byte field at the last byte of a region makes no sense 
>>> (unless impl.unaligned is true).
>>>
>>> access_size_max can be mr->ops->valid.max_access_size because memory.c 
>>> can and will still break accesses bigger than 
>>> mr->ops->impl.max_access_size.
>>>
>>> Markus, can you try the minimal patch above?  Or this one that also
>>> does the consequent simplifications.
>> 
>> FYI, the reproducer is very simple:
>> 
>> qemu-system-x86_64 -usb
>
> My patch works.

Yes, can you send a SoB and submit as a top level?

Right now uhci is completely broken.

Regards,

Anthony Liguori

>
> Paolo
>
>> Regards,
>> 
>> Anthony Liguori
>> 
>>>
>>> diff --git a/exec.c b/exec.c
>>> index c99a883..0904283 100644
>>> --- a/exec.c
>>> +++ b/exec.c
>>> @@ -1898,14 +1898,8 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
>>>  
>>>  static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
>>>  {
>>> -    unsigned access_size_min = mr->ops->impl.min_access_size;
>>> -    unsigned access_size_max = mr->ops->impl.max_access_size;
>>> +    unsigned access_size_max = mr->ops->valid.max_access_size;
>>>  
>>> -    /* Regions are assumed to support 1-4 byte accesses unless
>>> -       otherwise specified.  */
>>> -    if (access_size_min == 0) {
>>> -        access_size_min = 1;
>>> -    }
>>>      if (access_size_max == 0) {
>>>          access_size_max = 4;
>>>      }
>>> @@ -1922,9 +1916,6 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
>>>      if (l > access_size_max) {
>>>          l = access_size_max;
>>>      }
>>> -    /* ??? The users of this function are wrong, not supporting minimums larger
>>> -       than the remaining length.  C.f. memory.c:access_with_adjusted_size.  */
>>> -    assert(l >= access_size_min);
>>>  
>>>      return l;
>>>  }
>>>
>>> Paolo
>> 

Re: [Qemu-devel] [PULL 3/5] exec: Support 64-bit operations in address_space_rw

[Paolo Bonzini]

Il 17/07/2013 20:26, Richard Henderson ha scritto:
> On 07/17/2013 10:32 AM, Paolo Bonzini wrote:
>> My patch works.
> 
> You patch doesn't crash for this device, which isn't quite the same thing.
> 
> But it's certainly no worse than we had before my patch, so I'll not object so
> long as a fixme sort of comment is installed too.

I'm still not sure what the bug is (so what the FIXME comment would
be)... except of course that there may be bug in access_with_adjusted_size.

Paolo

Re: [Qemu-devel] [PULL 3/5] exec: Support 64-bit operations in address_space_rw

[Richard Henderson]

On 07/17/2013 11:57 AM, Paolo Bonzini wrote:
> I'm still not sure what the bug is (so what the FIXME comment would
> be)... except of course that there may be bug in access_with_adjusted_size.

The code here in exec.c is not using access_with_adjusted_size.

Unfortunately, access_with_adjusted_size only handles single copies,
one direction at a time.  We're attempting a sort of "memcpy", which
calls for some amount of caching across the loop...


r~

Re: [Qemu-devel] [PULL 3/5] exec: Support 64-bit operations in address_space_rw

[Paolo Bonzini]

Il 17/07/2013 21:28, Richard Henderson ha scritto:
> On 07/17/2013 11:57 AM, Paolo Bonzini wrote:
>> I'm still not sure what the bug is (so what the FIXME comment would
>> be)... except of course that there may be bug in access_with_adjusted_size.
> 
> The code here in exec.c is not using access_with_adjusted_size.

It is:

cpu_outb
-> address_space_write
-> address_space_rw
-> io_mem_write
-> memory_region_dispatch_write
-> access_with_adjusted_size

memory_access_size is just returning a length that makes sense when
passed to io_mem_write and ultimately to access_with_adjusted_size.

Paolo

> Unfortunately, access_with_adjusted_size only handles single copies,
> one direction at a time.  We're attempting a sort of "memcpy", which
> calls for some amount of caching across the loop...
> 
> 
> r~
> 

Re: [Qemu-devel] [PULL 3/5] exec: Support 64-bit operations in address_space_rw

[Richard Henderson]

On 07/17/2013 12:56 PM, Paolo Bonzini wrote:
> It is:
> 
> cpu_outb
> -> address_space_write
> -> address_space_rw
> -> io_mem_write
> -> memory_region_dispatch_write
> -> access_with_adjusted_size
> 
> memory_access_size is just returning a length that makes sense when
> passed to io_mem_write and ultimately to access_with_adjusted_size.

Ah, ok.  Sorry for being dense about the full context here.

I agree ignoring impl.minimum is ok here, since a real assert ought
to be lower down in access_with_adjusted_size, right before actually
dispatching to the device code.


r~