[PATCH 00/15] accel/tcg: Fix for #360 and other i/o alignment issues

[PATCH 00/15] accel/tcg: Fix for #360 and other i/o alignment issues

[Richard Henderson]

Short story is that the first two patches resolve the observed
problem, by completely bypassing quite a lot of code in memory.c.

Longer story is that we should either use that code in memory.c,
or we should bypass it to an even lower level, so that we don't
have multiple locations doing the partial-read assembly thing.

Patch 13 exposes a number of obvious device bugs via make check.
I'm sure there are more in devices that are less well tested.

Patch 15 has an obvious drawback: it breaks the original #360.
But it starts the conversation as to whether the check in memory.c
is in fact broken.


r~


Mark Cave-Ayland (2):
  NOTFORMERGE q800: test case for do_unaligned_access issue
  accel/tcg: Use byte ops for unaligned loads

Philippe Mathieu-Daudé (1):
  accel/tcg: Extract load_helper_unaligned from load_helper

Richard Henderson (12):
  accel/tcg: Don't test for watchpoints for code read
  accel/tcg: Handle page span access before i/o access
  softmmu/memory: Inline memory_region_dispatch_read1
  softmmu/memory: Simplify access_with_adjusted_size interface
  hw/net/e1000e: Fix size of io operations
  hw/net/e1000e: Fix impl.min_access_size
  hw/pci-host/q35: Improve blackhole_ops
  hw/scsi/megasas: Fix megasas_mmio_ops sizes
  hw/scsi/megasas: Improve megasas_queue_ops min_access_size
  softmmu/memory: Disallow short writes
  softmmu/memory: Support some unaligned access
  RFC accel/tcg: Defer some unaligned accesses to memory subsystem

 accel/tcg/cputlb.c | 147 +++++++++++++----------------
 hw/m68k/q800.c     | 131 ++------------------------
 hw/net/e1000e.c    |   8 +-
 hw/pci-host/q35.c  |   9 +-
 hw/scsi/megasas.c  |   6 +-
 softmmu/memory.c   | 226 +++++++++++++++++++++++++++++++++------------
 6 files changed, 251 insertions(+), 276 deletions(-)

-- 
2.25.1

[PATCH 01/15] NOTFORMERGE q800: test case for do_unaligned_access issue

[Richard Henderson]

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

A hack so that the built-in rom for -M a800 triggers the
problem within the first two instructions.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/q800.c | 131 ++++---------------------------------------------
 1 file changed, 9 insertions(+), 122 deletions(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 11376daa85..9795ceb06a 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -174,26 +174,13 @@ static void main_cpu_reset(void *opaque)
     CPUState *cs = CPU(cpu);
 
     cpu_reset(cs);
-    cpu->env.aregs[7] = ldl_phys(cs->as, 0);
-    cpu->env.pc = ldl_phys(cs->as, 4);
+    cpu->env.aregs[7] = 0x1000;
+    cpu->env.pc = MACROM_ADDR;
 }
 
 static uint8_t fake_mac_rom[] = {
-    0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
-
-    /* offset: 0xa - mac_reset */
-
-    /* via2[vDirB] |= VIA2B_vPower */
-    0x20, 0x7C, 0x50, 0xF0, 0x24, 0x00, /* moveal VIA2_BASE+vDirB,%a0 */
-    0x10, 0x10,                         /* moveb %a0@,%d0 */
-    0x00, 0x00, 0x00, 0x04,             /* orib #4,%d0 */
-    0x10, 0x80,                         /* moveb %d0,%a0@ */
-
-    /* via2[vBufB] &= ~VIA2B_vPower */
-    0x20, 0x7C, 0x50, 0xF0, 0x20, 0x00, /* moveal VIA2_BASE+vBufB,%a0 */
-    0x10, 0x10,                         /* moveb %a0@,%d0 */
-    0x02, 0x00, 0xFF, 0xFB,             /* andib #-5,%d0 */
-    0x10, 0x80,                         /* moveb %d0,%a0@ */
+    0x41, 0xf9, 0x50, 0x00, 0x00, 0x00,  /* lea 0x50000000,%a0 */
+    0x30, 0x28, 0x11, 0xff,              /* movew %a0@(4607),%d0 */
 
     /* while (true) ; */
     0x60, 0xFE                          /* bras [self] */
@@ -202,24 +189,11 @@ static uint8_t fake_mac_rom[] = {
 static void q800_init(MachineState *machine)
 {
     M68kCPU *cpu = NULL;
-    int linux_boot;
-    int32_t kernel_size;
-    uint64_t elf_entry;
-    char *filename;
-    int bios_size;
-    ram_addr_t initrd_base;
-    int32_t initrd_size;
     MemoryRegion *rom;
     MemoryRegion *io;
     const int io_slice_nb = (IO_SIZE / IO_SLICE) - 1;
     int i;
     ram_addr_t ram_size = machine->ram_size;
-    const char *kernel_filename = machine->kernel_filename;
-    const char *initrd_filename = machine->initrd_filename;
-    const char *kernel_cmdline = machine->kernel_cmdline;
-    const char *bios_name = machine->firmware ?: MACROM_FILENAME;
-    hwaddr parameters_base;
-    CPUState *cs;
     DeviceState *dev;
     DeviceState *via_dev;
     DeviceState *escc_orgate;
@@ -231,8 +205,6 @@ static void q800_init(MachineState *machine)
     DeviceState *glue;
     DriveInfo *dinfo;
 
-    linux_boot = (kernel_filename != NULL);
-
     if (ram_size > 1 * GiB) {
         error_report("Too much memory for this machine: %" PRId64 " MiB, "
                      "maximum 1024 MiB", ram_size / MiB);
@@ -392,96 +364,11 @@ static void q800_init(MachineState *machine)
     qdev_prop_set_uint8(dev, "depth", graphic_depth);
     qdev_realize_and_unref(dev, BUS(nubus), &error_fatal);
 
-    cs = CPU(cpu);
-    if (linux_boot) {
-        uint64_t high;
-        kernel_size = load_elf(kernel_filename, NULL, NULL, NULL,
-                               &elf_entry, NULL, &high, NULL, 1,
-                               EM_68K, 0, 0);
-        if (kernel_size < 0) {
-            error_report("could not load kernel '%s'", kernel_filename);
-            exit(1);
-        }
-        stl_phys(cs->as, 4, elf_entry); /* reset initial PC */
-        parameters_base = (high + 1) & ~1;
-
-        BOOTINFO1(cs->as, parameters_base, BI_MACHTYPE, MACH_MAC);
-        BOOTINFO1(cs->as, parameters_base, BI_FPUTYPE, FPU_68040);
-        BOOTINFO1(cs->as, parameters_base, BI_MMUTYPE, MMU_68040);
-        BOOTINFO1(cs->as, parameters_base, BI_CPUTYPE, CPU_68040);
-        BOOTINFO1(cs->as, parameters_base, BI_MAC_CPUID, CPUB_68040);
-        BOOTINFO1(cs->as, parameters_base, BI_MAC_MODEL, MAC_MODEL_Q800);
-        BOOTINFO1(cs->as, parameters_base,
-                  BI_MAC_MEMSIZE, ram_size >> 20); /* in MB */
-        BOOTINFO2(cs->as, parameters_base, BI_MEMCHUNK, 0, ram_size);
-        BOOTINFO1(cs->as, parameters_base, BI_MAC_VADDR, VIDEO_BASE);
-        BOOTINFO1(cs->as, parameters_base, BI_MAC_VDEPTH, graphic_depth);
-        BOOTINFO1(cs->as, parameters_base, BI_MAC_VDIM,
-                  (graphic_height << 16) | graphic_width);
-        BOOTINFO1(cs->as, parameters_base, BI_MAC_VROW,
-                  (graphic_width * graphic_depth + 7) / 8);
-        BOOTINFO1(cs->as, parameters_base, BI_MAC_SCCBASE, SCC_BASE);
-
-        rom = g_malloc(sizeof(*rom));
-        memory_region_init_ram_ptr(rom, NULL, "m68k_fake_mac.rom",
-                                   sizeof(fake_mac_rom), fake_mac_rom);
-        memory_region_set_readonly(rom, true);
-        memory_region_add_subregion(get_system_memory(), MACROM_ADDR, rom);
-
-        if (kernel_cmdline) {
-            BOOTINFOSTR(cs->as, parameters_base, BI_COMMAND_LINE,
-                        kernel_cmdline);
-        }
-
-        /* load initrd */
-        if (initrd_filename) {
-            initrd_size = get_image_size(initrd_filename);
-            if (initrd_size < 0) {
-                error_report("could not load initial ram disk '%s'",
-                             initrd_filename);
-                exit(1);
-            }
-
-            initrd_base = (ram_size - initrd_size) & TARGET_PAGE_MASK;
-            load_image_targphys(initrd_filename, initrd_base,
-                                ram_size - initrd_base);
-            BOOTINFO2(cs->as, parameters_base, BI_RAMDISK, initrd_base,
-                      initrd_size);
-        } else {
-            initrd_base = 0;
-            initrd_size = 0;
-        }
-        BOOTINFO0(cs->as, parameters_base, BI_LAST);
-    } else {
-        uint8_t *ptr;
-        /* allocate and load BIOS */
-        rom = g_malloc(sizeof(*rom));
-        memory_region_init_rom(rom, NULL, "m68k_mac.rom", MACROM_SIZE,
-                               &error_abort);
-        filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
-        memory_region_add_subregion(get_system_memory(), MACROM_ADDR, rom);
-
-        /* Load MacROM binary */
-        if (filename) {
-            bios_size = load_image_targphys(filename, MACROM_ADDR, MACROM_SIZE);
-            g_free(filename);
-        } else {
-            bios_size = -1;
-        }
-
-        /* Remove qtest_enabled() check once firmware files are in the tree */
-        if (!qtest_enabled()) {
-            if (bios_size < 0 || bios_size > MACROM_SIZE) {
-                error_report("could not load MacROM '%s'", bios_name);
-                exit(1);
-            }
-
-            ptr = rom_ptr(MACROM_ADDR, MACROM_SIZE);
-            stl_phys(cs->as, 0, ldl_p(ptr));    /* reset initial SP */
-            stl_phys(cs->as, 4,
-                     MACROM_ADDR + ldl_p(ptr + 4)); /* reset initial PC */
-        }
-    }
+    rom = g_malloc(sizeof(*rom));
+    memory_region_init_ram_ptr(rom, NULL, "m68k_fake_mac.rom",
+                               sizeof(fake_mac_rom), fake_mac_rom);
+    memory_region_set_readonly(rom, true);
+    memory_region_add_subregion(get_system_memory(), MACROM_ADDR, rom);
 }
 
 static void q800_machine_class_init(ObjectClass *oc, void *data)
-- 
2.25.1

[PATCH 02/15] accel/tcg: Extract load_helper_unaligned from load_helper

[Richard Henderson]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Replace a goto statement by an inlined function for easier review.
No logical change intended.

Inspired-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20210609141010.1066750-2-f4bug@amsat.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/cputlb.c | 52 ++++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 20 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index f24348e979..a94de90099 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1851,6 +1851,34 @@ load_memop(const void *haddr, MemOp op)
     }
 }
 
+static inline uint64_t QEMU_ALWAYS_INLINE
+load_helper_unaligned(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
+                      uintptr_t retaddr, MemOp op, bool code_read,
+                      FullLoadHelper *full_load)
+{
+    size_t size = memop_size(op);
+    target_ulong addr1, addr2;
+    uint64_t res;
+    uint64_t r1, r2;
+    unsigned shift;
+
+    addr1 = addr & ~((target_ulong)size - 1);
+    addr2 = addr1 + size;
+    r1 = full_load(env, addr1, oi, retaddr);
+    r2 = full_load(env, addr2, oi, retaddr);
+    shift = (addr & (size - 1)) * 8;
+
+    if (memop_big_endian(op)) {
+        /* Big-endian combine.  */
+        res = (r1 << shift) | (r2 >> ((size * 8) - shift));
+    } else {
+        /* Little-endian combine.  */
+        res = (r1 >> shift) | (r2 << ((size * 8) - shift));
+    }
+
+    return res & MAKE_64BIT_MASK(0, size * 8);
+}
+
 static inline uint64_t QEMU_ALWAYS_INLINE
 load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
             uintptr_t retaddr, MemOp op, bool code_read,
@@ -1866,7 +1894,6 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
         code_read ? MMU_INST_FETCH : MMU_DATA_LOAD;
     unsigned a_bits = get_alignment_bits(get_memop(oi));
     void *haddr;
-    uint64_t res;
     size_t size = memop_size(op);
 
     /* Handle CPU specific unaligned behaviour */
@@ -1895,7 +1922,8 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
 
         /* For anything that is unaligned, recurse through full_load.  */
         if ((addr & (size - 1)) != 0) {
-            goto do_unaligned_access;
+            return load_helper_unaligned(env, addr, oi, retaddr, op,
+                                         code_read, full_load);
         }
 
         iotlbentry = &env_tlb(env)->d[mmu_idx].iotlb[index];
@@ -1932,24 +1960,8 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
     if (size > 1
         && unlikely((addr & ~TARGET_PAGE_MASK) + size - 1
                     >= TARGET_PAGE_SIZE)) {
-        target_ulong addr1, addr2;
-        uint64_t r1, r2;
-        unsigned shift;
-    do_unaligned_access:
-        addr1 = addr & ~((target_ulong)size - 1);
-        addr2 = addr1 + size;
-        r1 = full_load(env, addr1, oi, retaddr);
-        r2 = full_load(env, addr2, oi, retaddr);
-        shift = (addr & (size - 1)) * 8;
-
-        if (memop_big_endian(op)) {
-            /* Big-endian combine.  */
-            res = (r1 << shift) | (r2 >> ((size * 8) - shift));
-        } else {
-            /* Little-endian combine.  */
-            res = (r1 >> shift) | (r2 << ((size * 8) - shift));
-        }
-        return res & MAKE_64BIT_MASK(0, size * 8);
+        return load_helper_unaligned(env, addr, oi, retaddr, op,
+                                     code_read, full_load);
     }
 
     haddr = (void *)((uintptr_t)addr + entry->addend);
-- 
2.25.1

[PATCH 03/15] accel/tcg: Use byte ops for unaligned loads

[Richard Henderson]

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

If an unaligned load is required then the load is split into
two separate accesses and combined.  This does not work correctly
with MMIO accesses because the I/O subsystem may use a different
endianness than we are expecting.

Use byte loads to obviate I/O endianness.  We already use byte
stores in store_helper_unaligned, so this solution has precedent.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/360
Message-Id: <20210609093528.9616-1-mark.cave-ayland@ilande.co.uk>
[PMD: Extract load_helper_unaligned() in earlier patch]
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20210609141010.1066750-3-f4bug@amsat.org>
[rth: Drop all of the stuff we do for stores not required by loads.]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/cputlb.c | 93 ++++++++++++++++++----------------------------
 1 file changed, 36 insertions(+), 57 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index a94de90099..ba21487138 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1854,35 +1854,36 @@ load_memop(const void *haddr, MemOp op)
 static inline uint64_t QEMU_ALWAYS_INLINE
 load_helper_unaligned(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
                       uintptr_t retaddr, MemOp op, bool code_read,
-                      FullLoadHelper *full_load)
+                      FullLoadHelper *byte_load)
 {
+    uintptr_t mmu_idx = get_mmuidx(oi);
     size_t size = memop_size(op);
-    target_ulong addr1, addr2;
-    uint64_t res;
-    uint64_t r1, r2;
-    unsigned shift;
-
-    addr1 = addr & ~((target_ulong)size - 1);
-    addr2 = addr1 + size;
-    r1 = full_load(env, addr1, oi, retaddr);
-    r2 = full_load(env, addr2, oi, retaddr);
-    shift = (addr & (size - 1)) * 8;
+    uint64_t val = 0;
+    int i;
 
+    /* XXX: not efficient, but simple. */
+    oi = make_memop_idx(MO_UB, mmu_idx);
     if (memop_big_endian(op)) {
-        /* Big-endian combine.  */
-        res = (r1 << shift) | (r2 >> ((size * 8) - shift));
+        for (i = 0; i < size; ++i) {
+            /* Big-endian load.  */
+            uint64_t val8 = byte_load(env, addr + i, oi, retaddr);
+            val = (val << 8) | val8;
+        }
     } else {
-        /* Little-endian combine.  */
-        res = (r1 >> shift) | (r2 << ((size * 8) - shift));
+        for (i = 0; i < size; ++i) {
+            /* Little-endian load.  */
+            uint64_t val8 = byte_load(env, addr + i, oi, retaddr);
+            val |= val8 << (i * 8);
+        }
     }
 
-    return res & MAKE_64BIT_MASK(0, size * 8);
+    return val;
 }
 
 static inline uint64_t QEMU_ALWAYS_INLINE
 load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
             uintptr_t retaddr, MemOp op, bool code_read,
-            FullLoadHelper *full_load)
+            FullLoadHelper *byte_load)
 {
     uintptr_t mmu_idx = get_mmuidx(oi);
     uintptr_t index = tlb_index(env, mmu_idx, addr);
@@ -1920,10 +1921,10 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
         CPUIOTLBEntry *iotlbentry;
         bool need_swap;
 
-        /* For anything that is unaligned, recurse through full_load.  */
+        /* For anything that is unaligned, recurse through byte_load.  */
         if ((addr & (size - 1)) != 0) {
             return load_helper_unaligned(env, addr, oi, retaddr, op,
-                                         code_read, full_load);
+                                         code_read, byte_load);
         }
 
         iotlbentry = &env_tlb(env)->d[mmu_idx].iotlb[index];
@@ -1961,7 +1962,7 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
         && unlikely((addr & ~TARGET_PAGE_MASK) + size - 1
                     >= TARGET_PAGE_SIZE)) {
         return load_helper_unaligned(env, addr, oi, retaddr, op,
-                                     code_read, full_load);
+                                     code_read, byte_load);
     }
 
     haddr = (void *)((uintptr_t)addr + entry->addend);
@@ -1978,8 +1979,9 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
  * We don't bother with this widened value for SOFTMMU_CODE_ACCESS.
  */
 
-static uint64_t full_ldub_mmu(CPUArchState *env, target_ulong addr,
-                              TCGMemOpIdx oi, uintptr_t retaddr)
+static uint64_t __attribute__((noinline))
+full_ldub_mmu(CPUArchState *env, target_ulong addr,
+              TCGMemOpIdx oi, uintptr_t retaddr)
 {
     return load_helper(env, addr, oi, retaddr, MO_UB, false, full_ldub_mmu);
 }
@@ -1993,8 +1995,7 @@ tcg_target_ulong helper_ret_ldub_mmu(CPUArchState *env, target_ulong addr,
 static uint64_t full_le_lduw_mmu(CPUArchState *env, target_ulong addr,
                                  TCGMemOpIdx oi, uintptr_t retaddr)
 {
-    return load_helper(env, addr, oi, retaddr, MO_LEUW, false,
-                       full_le_lduw_mmu);
+    return load_helper(env, addr, oi, retaddr, MO_LEUW, false, full_ldub_mmu);
 }
 
 tcg_target_ulong helper_le_lduw_mmu(CPUArchState *env, target_ulong addr,
@@ -2006,8 +2007,7 @@ tcg_target_ulong helper_le_lduw_mmu(CPUArchState *env, target_ulong addr,
 static uint64_t full_be_lduw_mmu(CPUArchState *env, target_ulong addr,
                                  TCGMemOpIdx oi, uintptr_t retaddr)
 {
-    return load_helper(env, addr, oi, retaddr, MO_BEUW, false,
-                       full_be_lduw_mmu);
+    return load_helper(env, addr, oi, retaddr, MO_BEUW, false, full_ldub_mmu);
 }
 
 tcg_target_ulong helper_be_lduw_mmu(CPUArchState *env, target_ulong addr,
@@ -2019,8 +2019,7 @@ tcg_target_ulong helper_be_lduw_mmu(CPUArchState *env, target_ulong addr,
 static uint64_t full_le_ldul_mmu(CPUArchState *env, target_ulong addr,
                                  TCGMemOpIdx oi, uintptr_t retaddr)
 {
-    return load_helper(env, addr, oi, retaddr, MO_LEUL, false,
-                       full_le_ldul_mmu);
+    return load_helper(env, addr, oi, retaddr, MO_LEUL, false, full_ldub_mmu);
 }
 
 tcg_target_ulong helper_le_ldul_mmu(CPUArchState *env, target_ulong addr,
@@ -2032,8 +2031,7 @@ tcg_target_ulong helper_le_ldul_mmu(CPUArchState *env, target_ulong addr,
 static uint64_t full_be_ldul_mmu(CPUArchState *env, target_ulong addr,
                                  TCGMemOpIdx oi, uintptr_t retaddr)
 {
-    return load_helper(env, addr, oi, retaddr, MO_BEUL, false,
-                       full_be_ldul_mmu);
+    return load_helper(env, addr, oi, retaddr, MO_BEUL, false, full_ldub_mmu);
 }
 
 tcg_target_ulong helper_be_ldul_mmu(CPUArchState *env, target_ulong addr,
@@ -2045,15 +2043,13 @@ tcg_target_ulong helper_be_ldul_mmu(CPUArchState *env, target_ulong addr,
 uint64_t helper_le_ldq_mmu(CPUArchState *env, target_ulong addr,
                            TCGMemOpIdx oi, uintptr_t retaddr)
 {
-    return load_helper(env, addr, oi, retaddr, MO_LEQ, false,
-                       helper_le_ldq_mmu);
+    return load_helper(env, addr, oi, retaddr, MO_LEQ, false, full_ldub_mmu);
 }
 
 uint64_t helper_be_ldq_mmu(CPUArchState *env, target_ulong addr,
                            TCGMemOpIdx oi, uintptr_t retaddr)
 {
-    return load_helper(env, addr, oi, retaddr, MO_BEQ, false,
-                       helper_be_ldq_mmu);
+    return load_helper(env, addr, oi, retaddr, MO_BEQ, false, full_ldub_mmu);
 }
 
 /*
@@ -2732,8 +2728,9 @@ void cpu_stq_le_data(CPUArchState *env, target_ulong ptr, uint64_t val)
 
 /* Code access functions.  */
 
-static uint64_t full_ldub_code(CPUArchState *env, target_ulong addr,
-                               TCGMemOpIdx oi, uintptr_t retaddr)
+static uint64_t __attribute__((noinline))
+full_ldub_code(CPUArchState *env, target_ulong addr,
+               TCGMemOpIdx oi, uintptr_t retaddr)
 {
     return load_helper(env, addr, oi, retaddr, MO_8, true, full_ldub_code);
 }
@@ -2744,38 +2741,20 @@ uint32_t cpu_ldub_code(CPUArchState *env, abi_ptr addr)
     return full_ldub_code(env, addr, oi, 0);
 }
 
-static uint64_t full_lduw_code(CPUArchState *env, target_ulong addr,
-                               TCGMemOpIdx oi, uintptr_t retaddr)
-{
-    return load_helper(env, addr, oi, retaddr, MO_TEUW, true, full_lduw_code);
-}
-
 uint32_t cpu_lduw_code(CPUArchState *env, abi_ptr addr)
 {
     TCGMemOpIdx oi = make_memop_idx(MO_TEUW, cpu_mmu_index(env, true));
-    return full_lduw_code(env, addr, oi, 0);
-}
-
-static uint64_t full_ldl_code(CPUArchState *env, target_ulong addr,
-                              TCGMemOpIdx oi, uintptr_t retaddr)
-{
-    return load_helper(env, addr, oi, retaddr, MO_TEUL, true, full_ldl_code);
+    return load_helper(env, addr, oi, 0, MO_TEUW, true, full_ldub_code);
 }
 
 uint32_t cpu_ldl_code(CPUArchState *env, abi_ptr addr)
 {
     TCGMemOpIdx oi = make_memop_idx(MO_TEUL, cpu_mmu_index(env, true));
-    return full_ldl_code(env, addr, oi, 0);
-}
-
-static uint64_t full_ldq_code(CPUArchState *env, target_ulong addr,
-                              TCGMemOpIdx oi, uintptr_t retaddr)
-{
-    return load_helper(env, addr, oi, retaddr, MO_TEQ, true, full_ldq_code);
+    return load_helper(env, addr, oi, 0, MO_TEUL, true, full_ldub_code);
 }
 
 uint64_t cpu_ldq_code(CPUArchState *env, abi_ptr addr)
 {
     TCGMemOpIdx oi = make_memop_idx(MO_TEQ, cpu_mmu_index(env, true));
-    return full_ldq_code(env, addr, oi, 0);
+    return load_helper(env, addr, oi, 0, MO_TEQ, true, full_ldub_code);
 }
-- 
2.25.1

[PATCH 04/15] accel/tcg: Don't test for watchpoints for code read

[Richard Henderson]

Data read watchpoints do not apply to code reads.
Watchpoints for code are called breakpoints, and
are handled by the translator.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/cputlb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index ba21487138..23a97849be 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1930,7 +1930,7 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
         iotlbentry = &env_tlb(env)->d[mmu_idx].iotlb[index];
 
         /* Handle watchpoints.  */
-        if (unlikely(tlb_addr & TLB_WATCHPOINT)) {
+        if (!code_read && unlikely(tlb_addr & TLB_WATCHPOINT)) {
             /* On watchpoint hit, this will longjmp out.  */
             cpu_check_watchpoint(env_cpu(env), addr, size,
                                  iotlbentry->attrs, BP_MEM_READ, retaddr);
-- 
2.25.1

[PATCH 05/15] accel/tcg: Handle page span access before i/o access

[Richard Henderson]

At present this is a distinction without much effect.
But this will enable further improvements.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/cputlb.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 23a97849be..6209e00c9b 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1916,6 +1916,14 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
         tlb_addr &= ~TLB_INVALID_MASK;
     }
 
+    /* Handle access that spans two pages. */
+    if (size > 1
+        && unlikely((addr & ~TARGET_PAGE_MASK) + size - 1
+                    >= TARGET_PAGE_SIZE)) {
+        return load_helper_unaligned(env, addr, oi, retaddr, op,
+                                     code_read, byte_load);
+    }
+
     /* Handle anything that isn't just a straight memory access.  */
     if (unlikely(tlb_addr & ~TARGET_PAGE_MASK)) {
         CPUIOTLBEntry *iotlbentry;
@@ -1957,14 +1965,6 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
         return load_memop(haddr, op);
     }
 
-    /* Handle slow unaligned access (it spans two pages or IO).  */
-    if (size > 1
-        && unlikely((addr & ~TARGET_PAGE_MASK) + size - 1
-                    >= TARGET_PAGE_SIZE)) {
-        return load_helper_unaligned(env, addr, oi, retaddr, op,
-                                     code_read, byte_load);
-    }
-
     haddr = (void *)((uintptr_t)addr + entry->addend);
     return load_memop(haddr, op);
 }
@@ -2421,6 +2421,16 @@ store_helper(CPUArchState *env, target_ulong addr, uint64_t val,
         tlb_addr = tlb_addr_write(entry) & ~TLB_INVALID_MASK;
     }
 
+    /* Handle access that spans two pages. */
+    if (size > 1
+        && unlikely((addr & ~TARGET_PAGE_MASK) + size - 1
+                     >= TARGET_PAGE_SIZE)) {
+    do_unaligned_access:
+        store_helper_unaligned(env, addr, val, retaddr, size,
+                               mmu_idx, memop_big_endian(op));
+        return;
+    }
+
     /* Handle anything that isn't just a straight memory access.  */
     if (unlikely(tlb_addr & ~TARGET_PAGE_MASK)) {
         CPUIOTLBEntry *iotlbentry;
@@ -2474,16 +2484,6 @@ store_helper(CPUArchState *env, target_ulong addr, uint64_t val,
         return;
     }
 
-    /* Handle slow unaligned access (it spans two pages or IO).  */
-    if (size > 1
-        && unlikely((addr & ~TARGET_PAGE_MASK) + size - 1
-                     >= TARGET_PAGE_SIZE)) {
-    do_unaligned_access:
-        store_helper_unaligned(env, addr, val, retaddr, size,
-                               mmu_idx, memop_big_endian(op));
-        return;
-    }
-
     haddr = (void *)((uintptr_t)addr + entry->addend);
     store_memop(haddr, val, op);
 }
-- 
2.25.1

[PATCH 06/15] softmmu/memory: Inline memory_region_dispatch_read1

[Richard Henderson]

Inline the body into the only caller of this function.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 softmmu/memory.c | 38 ++++++++++++++------------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index f0161515e9..744c5a80bd 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1408,29 +1408,6 @@ bool memory_region_access_valid(MemoryRegion *mr,
     return true;
 }
 
-static MemTxResult memory_region_dispatch_read1(MemoryRegion *mr,
-                                                hwaddr addr,
-                                                uint64_t *pval,
-                                                unsigned size,
-                                                MemTxAttrs attrs)
-{
-    *pval = 0;
-
-    if (mr->ops->read) {
-        return access_with_adjusted_size(addr, pval, size,
-                                         mr->ops->impl.min_access_size,
-                                         mr->ops->impl.max_access_size,
-                                         memory_region_read_accessor,
-                                         mr, attrs);
-    } else {
-        return access_with_adjusted_size(addr, pval, size,
-                                         mr->ops->impl.min_access_size,
-                                         mr->ops->impl.max_access_size,
-                                         memory_region_read_with_attrs_accessor,
-                                         mr, attrs);
-    }
-}
-
 MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
                                         hwaddr addr,
                                         uint64_t *pval,
@@ -1445,7 +1422,20 @@ MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
         return MEMTX_DECODE_ERROR;
     }
 
-    r = memory_region_dispatch_read1(mr, addr, pval, size, attrs);
+    *pval = 0;
+    if (mr->ops->read) {
+        r = access_with_adjusted_size(addr, pval, size,
+                                      mr->ops->impl.min_access_size,
+                                      mr->ops->impl.max_access_size,
+                                      memory_region_read_accessor,
+                                      mr, attrs);
+    } else {
+        r = access_with_adjusted_size(addr, pval, size,
+                                      mr->ops->impl.min_access_size,
+                                      mr->ops->impl.max_access_size,
+                                      memory_region_read_with_attrs_accessor,
+                                      mr, attrs);
+    }
     adjust_endianness(mr, pval, op);
     return r;
 }
-- 
2.25.1

[PATCH 07/15] softmmu/memory: Simplify access_with_adjusted_size interface

[Richard Henderson]

Create a typedef for the access_fn callback.  Remove the
access_size_{min,max} and access_fn arguments, and instead
derive these from the MemoryRegion argument.  Add a write
boolean argument.  Mark the function inline.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 softmmu/memory.c | 67 +++++++++++++++++++-----------------------------
 1 file changed, 26 insertions(+), 41 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 744c5a80bd..7373d89600 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -509,22 +509,24 @@ static MemTxResult memory_region_write_with_attrs_accessor(MemoryRegion *mr,
     return mr->ops->write_with_attrs(mr->opaque, addr, tmp, size, attrs);
 }
 
+typedef MemTxResult MemoryRegionAccessFn(MemoryRegion *mr,
+                                         hwaddr addr,
+                                         uint64_t *value,
+                                         unsigned size,
+                                         signed shift,
+                                         uint64_t mask,
+                                         MemTxAttrs attrs);
+
 static MemTxResult access_with_adjusted_size(hwaddr addr,
-                                      uint64_t *value,
-                                      unsigned size,
-                                      unsigned access_size_min,
-                                      unsigned access_size_max,
-                                      MemTxResult (*access_fn)
-                                                  (MemoryRegion *mr,
-                                                   hwaddr addr,
-                                                   uint64_t *value,
-                                                   unsigned size,
-                                                   signed shift,
-                                                   uint64_t mask,
-                                                   MemTxAttrs attrs),
-                                      MemoryRegion *mr,
-                                      MemTxAttrs attrs)
+                                             uint64_t *value,
+                                             unsigned size,
+                                             MemoryRegion *mr,
+                                             MemTxAttrs attrs,
+                                             bool write)
 {
+    unsigned access_size_min = mr->ops->impl.min_access_size;
+    unsigned access_size_max = mr->ops->impl.max_access_size;
+    MemoryRegionAccessFn *access_fn;
     uint64_t access_mask;
     unsigned access_size;
     unsigned i;
@@ -537,6 +539,14 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
         access_size_max = 4;
     }
 
+    if (write) {
+        access_fn = (mr->ops->write ? memory_region_write_accessor
+                     : memory_region_write_with_attrs_accessor);
+    } else {
+        access_fn = (mr->ops->read ? memory_region_read_accessor
+                     : memory_region_read_with_attrs_accessor);
+    }
+
     /* FIXME: support unaligned access? */
     access_size = MAX(MIN(size, access_size_max), access_size_min);
     access_mask = MAKE_64BIT_MASK(0, access_size * 8);
@@ -1423,19 +1433,7 @@ MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
     }
 
     *pval = 0;
-    if (mr->ops->read) {
-        r = access_with_adjusted_size(addr, pval, size,
-                                      mr->ops->impl.min_access_size,
-                                      mr->ops->impl.max_access_size,
-                                      memory_region_read_accessor,
-                                      mr, attrs);
-    } else {
-        r = access_with_adjusted_size(addr, pval, size,
-                                      mr->ops->impl.min_access_size,
-                                      mr->ops->impl.max_access_size,
-                                      memory_region_read_with_attrs_accessor,
-                                      mr, attrs);
-    }
+    r = access_with_adjusted_size(addr, pval, size, mr, attrs, false);
     adjust_endianness(mr, pval, op);
     return r;
 }
@@ -1486,20 +1484,7 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
         return MEMTX_OK;
     }
 
-    if (mr->ops->write) {
-        return access_with_adjusted_size(addr, &data, size,
-                                         mr->ops->impl.min_access_size,
-                                         mr->ops->impl.max_access_size,
-                                         memory_region_write_accessor, mr,
-                                         attrs);
-    } else {
-        return
-            access_with_adjusted_size(addr, &data, size,
-                                      mr->ops->impl.min_access_size,
-                                      mr->ops->impl.max_access_size,
-                                      memory_region_write_with_attrs_accessor,
-                                      mr, attrs);
-    }
+    return access_with_adjusted_size(addr, &data, size, mr, attrs, true);
 }
 
 void memory_region_init_io(MemoryRegion *mr,
-- 
2.25.1

[PATCH 08/15] hw/net/e1000e: Fix size of io operations

[Richard Henderson]

The size of the operation is an argument to the respective
functions, and has nothing to do with sizeof(val).

Cc: Jason Wang <jasowang@redhat.com>
Cc: Dmitry Fleytman <dmitry.fleytman@gmail.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/net/e1000e.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index a8a77eca95..ea3347fbb4 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -145,7 +145,7 @@ e1000e_io_read(void *opaque, hwaddr addr, unsigned size)
         return s->ioaddr;
     case E1000_IODATA:
         if (e1000e_io_get_reg_index(s, &idx)) {
-            val = e1000e_core_read(&s->core, idx, sizeof(val));
+            val = e1000e_core_read(&s->core, idx, size);
             trace_e1000e_io_read_data(idx, val);
             return val;
         }
@@ -171,7 +171,7 @@ e1000e_io_write(void *opaque, hwaddr addr,
     case E1000_IODATA:
         if (e1000e_io_get_reg_index(s, &idx)) {
             trace_e1000e_io_write_data(idx, val);
-            e1000e_core_write(&s->core, idx, val, sizeof(val));
+            e1000e_core_write(&s->core, idx, val, size);
         }
         return;
     default:
-- 
2.25.1

[PATCH 09/15] hw/net/e1000e: Fix impl.min_access_size

[Richard Henderson]

There are certainly architectural 2 byte writes, as evidenced
by the e1000e_set_16bit function.  I also saw a 1 byte write,
though that may have been the fuzzer.

Cc: Jason Wang <jasowang@redhat.com> 
Cc: Dmitry Fleytman <dmitry.fleytman@gmail.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/net/e1000e.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index ea3347fbb4..ad73e39ebc 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -185,7 +185,7 @@ static const MemoryRegionOps mmio_ops = {
     .write = e1000e_mmio_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .impl = {
-        .min_access_size = 4,
+        .min_access_size = 1,
         .max_access_size = 4,
     },
 };
@@ -195,7 +195,7 @@ static const MemoryRegionOps io_ops = {
     .write = e1000e_io_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .impl = {
-        .min_access_size = 4,
+        .min_access_size = 1,
         .max_access_size = 4,
     },
 };
-- 
2.25.1

[PATCH 10/15] hw/pci-host/q35: Improve blackhole_ops

[Richard Henderson]

There is nothing about the blackhole that requires 4 byte
operations.  Decrease the min size to 1, increase the max
size to 8.  Drop duplicate endianness spec.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/pci-host/q35.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 2eb729dff5..133be67e4f 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -268,7 +268,7 @@ static const TypeInfo q35_host_info = {
 
 static uint64_t blackhole_read(void *ptr, hwaddr reg, unsigned size)
 {
-    return 0xffffffff;
+    return UINT64_MAX;
 }
 
 static void blackhole_write(void *opaque, hwaddr addr, uint64_t val,
@@ -282,10 +282,9 @@ static const MemoryRegionOps blackhole_ops = {
     .write = blackhole_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
     .valid.min_access_size = 1,
-    .valid.max_access_size = 4,
-    .impl.min_access_size = 4,
-    .impl.max_access_size = 4,
-    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid.max_access_size = 8,
+    .impl.min_access_size = 1,
+    .impl.max_access_size = 8,
 };
 
 /* PCIe MMCFG */
-- 
2.25.1

[PATCH 11/15] hw/scsi/megasas: Fix megasas_mmio_ops sizes

[Richard Henderson]

All of the megasas mmio registers are 32-bit not 64-bit.

Cc: qemu-block@nongnu.org
Cc: Fam Zheng <fam@euphon.net>
Cc: Hannes Reinecke <hare@suse.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/scsi/megasas.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 8f2389d2c6..c98cb7a499 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2163,8 +2163,8 @@ static const MemoryRegionOps megasas_mmio_ops = {
     .write = megasas_mmio_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .impl = {
-        .min_access_size = 8,
-        .max_access_size = 8,
+        .min_access_size = 4,
+        .max_access_size = 4,
     }
 };
 
-- 
2.25.1

[PATCH 12/15] hw/scsi/megasas: Improve megasas_queue_ops min_access_size

[Richard Henderson]

This device is a read-zero-write-ignored device.
We can trivially support any size operation.

Cc: qemu-block@nongnu.org
Cc: Fam Zheng <fam@euphon.net> 
Cc: Hannes Reinecke <hare@suse.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/scsi/megasas.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index c98cb7a499..197b75225f 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2207,7 +2207,7 @@ static const MemoryRegionOps megasas_queue_ops = {
     .write = megasas_queue_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .impl = {
-        .min_access_size = 8,
+        .min_access_size = 1,
         .max_access_size = 8,
     }
 };
-- 
2.25.1

[PATCH 13/15] softmmu/memory: Disallow short writes

[Richard Henderson]

Writes smaller than impl.min_access_size would require a
read-modify-write cycle, which could have side effects.

The present behaviour seems to be to extend the current write
to min_access_size.  While we could continue that, so far all
of the instances I have seen have been either device model
errors or the fuzzer intentionally doing bad things.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 softmmu/memory.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 7373d89600..2fe237327d 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -548,6 +548,26 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
     }
 
     /* FIXME: support unaligned access? */
+    /*
+     * Check for a small access.
+     */
+    if (unlikely(size < access_size_min)) {
+        /*
+         * Logically, we cannot support short writes without a read-modify
+         * cycle, and many mmio registers have side-effects on read.
+         * In practice, this appears to be either (1) model error,
+         * or (2) guest error via the fuzzer.
+         */
+        if (write) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid short write: %s "
+                          "hwaddr: 0x%" HWADDR_PRIx " size: %u "
+                          "min: %u max: %u\n", __func__,
+                          memory_region_name(mr), addr, size,
+                          access_size_min, access_size_max);
+            return MEMTX_ERROR;
+        }
+    }
+
     access_size = MAX(MIN(size, access_size_max), access_size_min);
     access_mask = MAKE_64BIT_MASK(0, access_size * 8);
     if (memory_region_big_endian(mr)) {
-- 
2.25.1

[PATCH 14/15] softmmu/memory: Support some unaligned access

[Richard Henderson]

Decline to support writes that cannot be covered by access_size_min.
Decline to support unaligned reads that require extraction from more
than two reads.

Do support exact size match when the model supports unaligned.
Do support reducing the operation size to match the alignment.
Do support loads that extract from 1 or 2 larger loads.

Diagnose anything that we do not handle via LOG_GUEST_ERROR,
as any of these cases are probably model or guest errors.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 softmmu/memory.c | 127 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 121 insertions(+), 6 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 2fe237327d..baf8573f1b 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -529,7 +529,7 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
     MemoryRegionAccessFn *access_fn;
     uint64_t access_mask;
     unsigned access_size;
-    unsigned i;
+    signed access_sh;
     MemTxResult r = MEMTX_OK;
 
     if (!access_size_min) {
@@ -547,7 +547,6 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
                      : memory_region_read_with_attrs_accessor);
     }
 
-    /* FIXME: support unaligned access? */
     /*
      * Check for a small access.
      */
@@ -557,6 +556,10 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
          * cycle, and many mmio registers have side-effects on read.
          * In practice, this appears to be either (1) model error,
          * or (2) guest error via the fuzzer.
+         *
+         * TODO: Are all short reads also guest or model errors, because
+         * of said side effects?  Or is this valid read-for-effect then
+         * discard the (complete) result via narrow destination register?
          */
         if (write) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid short write: %s "
@@ -566,22 +569,134 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
                           access_size_min, access_size_max);
             return MEMTX_ERROR;
         }
+
+        /*
+         * If the original access is aligned, we can always extract
+         * from a single larger load.
+         */
+        access_size = access_size_min;
+        if (likely((addr & (size - 1)) == 0)) {
+            goto extract;
+        }
+
+        /*
+         * TODO: We could search for a larger load that happens to
+         * cover the unaligned load, but at some point we will always
+         * require two operations.  Extract from two loads.
+         */
+        goto extract2;
     }
 
+    /*
+     * Check for size in range.
+     */
+    if (likely(size <= access_size_max)) {
+        /*
+         * If the access is aligned or if the model supports
+         * unaligned accesses, use one operation directly.
+         */
+        if (likely((addr & (size - 1)) == 0) || mr->ops->impl.unaligned) {
+            access_size = size;
+            access_sh = 0;
+            goto direct;
+        }
+    }
+
+    /*
+     * It is certain that we require multiple operations.
+     * If the access is aligned (or the model supports unaligned),
+     * then we will perform N accesses which exactly cover the
+     * operation requested.
+     */
     access_size = MAX(MIN(size, access_size_max), access_size_min);
+    if (unlikely(addr & (access_size - 1))) {
+        unsigned lsb = addr & -addr;
+        if (lsb >= access_size_min) {
+            /*
+             * The model supports small enough loads that we can
+             * exactly match the operation requested.  For reads,
+             * this is preferable to touching more than requested.
+             * For writes, this is mandatory.
+             */
+            access_size = lsb;
+        } else if (write) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid unaligned write: %s "
+                          "hwaddr: 0x%" HWADDR_PRIx " size: %u "
+                          "min: %u max: %u\n", __func__,
+                          memory_region_name(mr), addr, size,
+                          access_size_min, access_size_max);
+            return MEMTX_ERROR;
+        } else if (size <= access_size_max) {
+            /* As per above, we can use two loads to implement. */
+            access_size = size;
+            goto extract2;
+        } else {
+            /*
+             * TODO: becaseu access_size_max is small, this case requires
+             * more than 2 loads to assemble and extract.  Bail out.
+             */
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: Unhandled unaligned read: %s "
+                          "hwaddr: 0x%" HWADDR_PRIx " size: %u "
+                          "min: %u max: %u\n", __func__,
+                          memory_region_name(mr), addr, size,
+                          access_size_min, access_size_max);
+            return MEMTX_ERROR;
+        }
+    }
+
     access_mask = MAKE_64BIT_MASK(0, access_size * 8);
     if (memory_region_big_endian(mr)) {
-        for (i = 0; i < size; i += access_size) {
+        for (unsigned i = 0; i < size; i += access_size) {
             r |= access_fn(mr, addr + i, value, access_size,
-                        (size - access_size - i) * 8, access_mask, attrs);
+                           (size - access_size - i) * 8, access_mask, attrs);
         }
     } else {
-        for (i = 0; i < size; i += access_size) {
+        for (unsigned i = 0; i < size; i += access_size) {
             r |= access_fn(mr, addr + i, value, access_size, i * 8,
-                        access_mask, attrs);
+                           access_mask, attrs);
         }
     }
     return r;
+
+ extract2:
+    /*
+     * Extract from one or two loads to produce the result.
+     * Validate that we need two loads before performing them.
+     */
+    access_sh = addr & (access_size - 1);
+    if (access_sh + size > access_size) {
+        addr &= ~(access_size - 1);
+        if (memory_region_big_endian(mr)) {
+            access_sh = (access_size - access_sh) * 8;
+            r |= access_fn(mr, addr, value, access_size, access_sh, -1, attrs);
+            access_sh -= access_size * 8;
+            r |= access_fn(mr, addr, value, access_size, access_sh, -1, attrs);
+        } else {
+            access_sh = (access_sh - access_size) * 8;
+            r |= access_fn(mr, addr, value, access_size, access_sh, -1, attrs);
+            access_sh += access_size * 8;
+            r |= access_fn(mr, addr, value, access_size, access_sh, -1, attrs);
+        }
+        *value &= MAKE_64BIT_MASK(0, size * 8);
+        return r;
+    }
+
+ extract:
+    /*
+     * Extract from one larger load to produce the result.
+     */
+    access_sh = addr & (access_size - 1);
+    addr &= ~(access_size - 1);
+    if (memory_region_big_endian(mr)) {
+        access_sh = access_size - size - access_sh;
+    }
+    /* Note that with this interface, right shift is negative. */
+    access_sh *= -8;
+
+ direct:
+    access_mask = MAKE_64BIT_MASK(0, size * 8);
+    return access_fn(mr, addr, value, access_size, access_sh,
+                     access_mask, attrs);
 }
 
 static AddressSpace *memory_region_to_address_space(MemoryRegion *mr)
-- 
2.25.1

[PATCH 15/15] RFC accel/tcg: Defer some unaligned accesses to memory subsystem

[Richard Henderson]

For unaligned i/o accesses that do not cross pages, do not handle
the misalignment in cputlb, but let the memory system deal with it.

RFC because this, for the first time, exposes many guests to the
existing mr->ops->valid.unaligned checks in the memory subsystem.
Previously this code was only reachable when guest code explicitly
calls memory_region_dispatch_*.

This does in fact trip up the original m68k q800 testcase, #360.

Since this hasn't really been reachable, I'm willing to bet that
every device is wrong wrt mr->ops->valid.unaligned, and possibly
that we shouldn't even have it.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/cputlb.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 6209e00c9b..905edab19b 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1929,12 +1929,6 @@ load_helper(CPUArchState *env, target_ulong addr, TCGMemOpIdx oi,
         CPUIOTLBEntry *iotlbentry;
         bool need_swap;
 
-        /* For anything that is unaligned, recurse through byte_load.  */
-        if ((addr & (size - 1)) != 0) {
-            return load_helper_unaligned(env, addr, oi, retaddr, op,
-                                         code_read, byte_load);
-        }
-
         iotlbentry = &env_tlb(env)->d[mmu_idx].iotlb[index];
 
         /* Handle watchpoints.  */
@@ -2425,7 +2419,6 @@ store_helper(CPUArchState *env, target_ulong addr, uint64_t val,
     if (size > 1
         && unlikely((addr & ~TARGET_PAGE_MASK) + size - 1
                      >= TARGET_PAGE_SIZE)) {
-    do_unaligned_access:
         store_helper_unaligned(env, addr, val, retaddr, size,
                                mmu_idx, memop_big_endian(op));
         return;
@@ -2436,11 +2429,6 @@ store_helper(CPUArchState *env, target_ulong addr, uint64_t val,
         CPUIOTLBEntry *iotlbentry;
         bool need_swap;
 
-        /* For anything that is unaligned, recurse through byte stores.  */
-        if ((addr & (size - 1)) != 0) {
-            goto do_unaligned_access;
-        }
-
         iotlbentry = &env_tlb(env)->d[mmu_idx].iotlb[index];
 
         /* Handle watchpoints.  */
-- 
2.25.1

Re: [PATCH 00/15] accel/tcg: Fix for #360 and other i/o alignment issues

[Mark Cave-Ayland]

On 19/06/2021 18:26, Richard Henderson wrote:

> Short story is that the first two patches resolve the observed
> problem, by completely bypassing quite a lot of code in memory.c.
> 
> Longer story is that we should either use that code in memory.c,
> or we should bypass it to an even lower level, so that we don't
> have multiple locations doing the partial-read assembly thing.
> 
> Patch 13 exposes a number of obvious device bugs via make check.
> I'm sure there are more in devices that are less well tested.
> 
> Patch 15 has an obvious drawback: it breaks the original #360.
> But it starts the conversation as to whether the check in memory.c
> is in fact broken.
> 
> 
> r~
> 
> 
> Mark Cave-Ayland (2):
>    NOTFORMERGE q800: test case for do_unaligned_access issue
>    accel/tcg: Use byte ops for unaligned loads
> 
> Philippe Mathieu-Daudé (1):
>    accel/tcg: Extract load_helper_unaligned from load_helper
> 
> Richard Henderson (12):
>    accel/tcg: Don't test for watchpoints for code read
>    accel/tcg: Handle page span access before i/o access
>    softmmu/memory: Inline memory_region_dispatch_read1
>    softmmu/memory: Simplify access_with_adjusted_size interface
>    hw/net/e1000e: Fix size of io operations
>    hw/net/e1000e: Fix impl.min_access_size
>    hw/pci-host/q35: Improve blackhole_ops
>    hw/scsi/megasas: Fix megasas_mmio_ops sizes
>    hw/scsi/megasas: Improve megasas_queue_ops min_access_size
>    softmmu/memory: Disallow short writes
>    softmmu/memory: Support some unaligned access
>    RFC accel/tcg: Defer some unaligned accesses to memory subsystem
> 
>   accel/tcg/cputlb.c | 147 +++++++++++++----------------
>   hw/m68k/q800.c     | 131 ++------------------------
>   hw/net/e1000e.c    |   8 +-
>   hw/pci-host/q35.c  |   9 +-
>   hw/scsi/megasas.c  |   6 +-
>   softmmu/memory.c   | 226 +++++++++++++++++++++++++++++++++------------
>   6 files changed, 251 insertions(+), 276 deletions(-)

Hi Richard,

I've had a look over this patchset, and based upon my work leading up to #360 this 
does feel like an improvement: in particular separating out page spanning accesses, 
and removing the duplicate cputlb code for splitting/combining unaligned accesses 
seem like wins to me.

As mentioned in the report itself, cputlb has effectively been bypassing 
.min_access_size but I'm not sure about the consequences of this - does it mean that 
the access size check should also be bypassed for the head/tail of unaligned 
accesses? I'm also not completely sure how this behaviour can change based upon the 
target CPU.

The comment in patch 15 re: mr->ops->valid.unaligned is interesting: if the memory 
subsystem can split the accesses beforehand so that they are accepted by the device, 
does this check then become obsolete?

My general feeling is that there are still some discussions to be had around defining 
how unaligned accesses should work, and including a qtest to cover the various 
unaligned/page span cases would help here. Other than that I do feel the patch is 
worthwhile, and it may be there is a situation similar to the 
memory_region_access_valid() changes in 5d971f9e67 whereby we accept the change in 
behaviour will have some fallout but allow plenty of time for regressions to be fixed.


ATB,

Mark.

Re: [PATCH 00/15] accel/tcg: Fix for #360 and other i/o alignment issues

[Peter Maydell]

On Sat, 19 Jun 2021 at 18:28, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Short story is that the first two patches resolve the observed
> problem, by completely bypassing quite a lot of code in memory.c.
>
> Longer story is that we should either use that code in memory.c,
> or we should bypass it to an even lower level, so that we don't
> have multiple locations doing the partial-read assembly thing.

I haven't read the patchset yet, but my initial reaction is that
we ought to be handling this stuff in memory.c, because that
code is shared across all accelerators -- we would want the
same behaviour for accesses to a device that doesn't itself
handle a misalignment or a small access, whether we are using
TCG or KVM or HVF or whatever...

thanks
-- PMM

Re: [PATCH 09/15] hw/net/e1000e: Fix impl.min_access_size

[Jason Wang]

在 2021/6/20 上午1:26, Richard Henderson 写道:
> There are certainly architectural 2 byte writes, as evidenced
> by the e1000e_set_16bit function.  I also saw a 1 byte write,
> though that may have been the fuzzer.
>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Dmitry Fleytman <dmitry.fleytman@gmail.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   hw/net/e1000e.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> index ea3347fbb4..ad73e39ebc 100644
> --- a/hw/net/e1000e.c
> +++ b/hw/net/e1000e.c
> @@ -185,7 +185,7 @@ static const MemoryRegionOps mmio_ops = {
>       .write = e1000e_mmio_write,
>       .endianness = DEVICE_LITTLE_ENDIAN,
>       .impl = {
> -        .min_access_size = 4,
> +        .min_access_size = 1,


I'm not sure this can work. Looks like at least 
e1000e_get_reg_index_with_offset() was wrote with the assumption that 
min_access_size is 4:

static inline uint16_t
e1000e_get_reg_index_with_offset(const uint16_t *mac_reg_access, hwaddr 
addr)
{
     uint16_t index = (addr & 0x1ffff) >> 2;
     return index + (mac_reg_access[index] & 0xfffe);
}

Thanks


>           .max_access_size = 4,
>       },
>   };
> @@ -195,7 +195,7 @@ static const MemoryRegionOps io_ops = {
>       .write = e1000e_io_write,
>       .endianness = DEVICE_LITTLE_ENDIAN,
>       .impl = {
> -        .min_access_size = 4,
> +        .min_access_size = 1,
>           .max_access_size = 4,
>       },
>   };

Re: [PATCH 06/15] softmmu/memory: Inline memory_region_dispatch_read1

[Philippe Mathieu-Daudé]

On 6/19/21 7:26 PM, Richard Henderson wrote:
> Inline the body into the only caller of this function.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  softmmu/memory.c | 38 ++++++++++++++------------------------
>  1 file changed, 14 insertions(+), 24 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 07/15] softmmu/memory: Simplify access_with_adjusted_size interface

[Philippe Mathieu-Daudé]

On 6/19/21 7:26 PM, Richard Henderson wrote:
> Create a typedef for the access_fn callback.  Remove the
> access_size_{min,max} and access_fn arguments, and instead
> derive these from the MemoryRegion argument.  Add a write
> boolean argument.  Mark the function inline.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  softmmu/memory.c | 67 +++++++++++++++++++-----------------------------
>  1 file changed, 26 insertions(+), 41 deletions(-)

Nice simplification :)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 04/15] accel/tcg: Don't test for watchpoints for code read

[Philippe Mathieu-Daudé]

On 6/19/21 7:26 PM, Richard Henderson wrote:
> Data read watchpoints do not apply to code reads.
> Watchpoints for code are called breakpoints, and
> are handled by the translator.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  accel/tcg/cputlb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 10/15] hw/pci-host/q35: Improve blackhole_ops

[Philippe Mathieu-Daudé]

On 6/19/21 7:26 PM, Richard Henderson wrote:
> There is nothing about the blackhole that requires 4 byte
> operations.  Decrease the min size to 1, increase the max
> size to 8.  Drop duplicate endianness spec.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  hw/pci-host/q35.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>