ioport/memory: check that both .read and .write callbacks are defined

If that's not the case, QEMU will may during execution. This has recently been fixed for:
- acpi (2d3b989529727ccace243b953a181fbae04a30d1)
- kvmapic (0c1cd0ae2a4faabeb948b9a07ea1696e853de174)
- xhci (6d3bc22e31bcee74dc1e05a5370cabb33b7c3fda)

---
I started all current QEMU system emulations with qemu-system-{arch} -M {machine} , and none broke on these additionnal asserts.
However, lots of them exited for other reasons, like not having the right number of CPUs, no -kernel argument, or fetching invalid instructions from RAM.

 ioport.c |    1 +
 memory.c |    2 ++
 2 files changed, 3 insertions(+)

index a0ac2a0..8dd9d50 100644
@@ -337,6 +337,7 @@ void portio_list_init(PortioList *piolist,
     unsigned n = 0;
 
     while (callbacks[n].size) {
+        assert(callbacks[n].read && callbacks[n].write);
         ++n;
     }
 

index 5cb8f4a..654d1ce 100644
@@ -1008,6 +1008,8 @@ void memory_region_init_io(MemoryRegion *mr,
                            uint64_t size)
 {
     memory_region_init(mr, name, size);
+    assert(ops->read || ops->old_mmio.read);
+    assert(ops->write || ops->old_mmio.write);
     mr->ops = ops;
     mr->opaque = opaque;
     mr->terminates = true;

1.7.10.4

And apparently we also have alot of other cases like this, which will trigger this new assert:

hw/ssi/xilinx_spips.c:static const MemoryRegionOps lqspi_ops = {

hw/ssi/xilinx_spips.c-    .read = lqspi_read,
hw/ssi/xilinx_spips.c-    .endianness = DEVICE_NATIVE_ENDIAN,
hw/ssi/xilinx_spips.c-    .valid = {

hw/ssi/xilinx_spips.c- .min_access_size = 1,
hw/ssi/xilinx_spips.c- .max_access_size = 4
hw/ssi/xilinx_spips.c- }

hw/usb/hcd-ehci.c:static const MemoryRegionOps ehci_mmio_caps_ops = {

hw/usb/hcd-ehci.c-    .read = ehci_caps_read,
hw/usb/hcd-ehci.c-    .valid.min_access_size = 1,
hw/usb/hcd-ehci.c-    .valid.max_access_size = 4,
hw/usb/hcd-ehci.c-    .impl.min_access_size = 1,
hw/usb/hcd-ehci.c-    .impl.max_access_size = 1,
hw/usb/hcd-ehci.c-    .endianness = DEVICE_LITTLE_ENDIAN,

hw/usb/hcd-ehci.c-};

hw/scsi/megasas.c:static const MemoryRegionOps megasas_queue_ops = {

hw/scsi/megasas.c-    .read = megasas_queue_read,
hw/scsi/megasas.c-    .endianness = DEVICE_LITTLE_ENDIAN,
hw/scsi/megasas.c-    .impl = {

hw/scsi/megasas.c- .min_access_size = 8,
hw/scsi/megasas.c- .max_access_size = 8,
hw/scsi/megasas.c- }
hw/scsi/megasas.c-};

hw/pci/msix.c:static const MemoryRegionOps msix_pba_mmio_ops = {

hw/pci/msix.c-    .read = msix_pba_mmio_read,
hw/pci/msix.c-    .endianness = DEVICE_LITTLE_ENDIAN,
hw/pci/msix.c-    .valid = {

hw/pci/msix.c- .min_access_size = 4,
hw/pci/msix.c- .max_access_size = 4,
hw/pci/msix.c- },
hw/pci/msix.c-};

hw/misc/lm32_sys.c:static const MemoryRegionOps sys_ops = {

hw/misc/lm32_sys.c-    .write = sys_write,
hw/misc/lm32_sys.c-    .valid.accepts = sys_ops_accepts,
hw/misc/lm32_sys.c-    .endianness = DEVICE_NATIVE_ENDIAN,

hw/misc/lm32_sys.c-};

hw/misc/vfio.c:static const MemoryRegionOps vfio_ati_3c3_quirk = {
hw/misc/vfio.c- .read = vfio_ati_3c3_quirk_read,
hw/misc/vfio.c- .endianness = DEVICE_LITTLE_ENDIAN,
hw/misc/vfio.c-};

hw/misc/debugexit.c:static const MemoryRegionOps debug_exit_ops = {

hw/misc/debugexit.c-    .write = debug_exit_write,
hw/misc/debugexit.c-    .valid.min_access_size = 1,
hw/misc/debugexit.c-    .valid.max_access_size = 4,
hw/misc/debugexit.c-    .endianness = DEVICE_LITTLE_ENDIAN,

hw/misc/debugexit.c-};

hw/misc/pc-testdev.c:static const MemoryRegionOps test_irq_ops = {

hw/misc/pc-testdev.c-    .write = test_irq_line,
hw/misc/pc-testdev.c-    .valid.min_access_size = 1,
hw/misc/pc-testdev.c-    .valid.max_access_size = 1,
hw/misc/pc-testdev.c-    .endianness = DEVICE_LITTLE_ENDIAN,

hw/misc/pc-testdev.c-};

hw/misc/pc-testdev.c:static const MemoryRegionOps test_flush_ops = {

hw/misc/pc-testdev.c-    .write = test_flush_page,
hw/misc/pc-testdev.c-    .valid.min_access_size = 4,
hw/misc/pc-testdev.c-    .valid.max_access_size = 4,
hw/misc/pc-testdev.c-    .endianness = DEVICE_LITTLE_ENDIAN,

hw/misc/pc-testdev.c-};

Maybe instead (or in addition to), we should provide a dummy read or write functions -- instead of fixing each such occurence to use its own dummy function -- and maybe even a function to map old_mmio.{read,write} into {read,write} (so we'll have less ifs in there).

Adding some Cc's. I don't think this is "trivial enough" having in mind all the above :)

Thanks,

/mjt

Hi,

Makes sense, especially for write where we can just ignore what the guest attempts to write. Not sure we can have a generic handler for reads. Maybe two, one which returns 0xff and one which returns 0x00.

cheers,
Gerd

Hi,

FWIW, I have one in my tree that qemu_log(LOG_GUEST_ERROR's such accesses that I use for unimplemented devices. It's worthwhile to trap such accesses and speaking for the Xilinx LQSPI case, my preference is for some form of failure rather than silent write-ignore. And can we have an option where a invalid writes have consistent behavior with unassigned accesses?

Regards,
Peter

Probably not a good idea. Ignoring unassigned addresses is very handy for compatibility: we can run new guests on old qemu and They don't crash or log errors.

Hi Michael,

Log errors do not crash QEMU even if they are enabled. They just make noise and even then only if you pass -d guest_errors (which we do as pretty much habit now for this reason). It is the compromise solution between those of us who want to ignore them and those of us who need to know about them. The default behavior will still be to ignore accesses with no action.

Regards,
Peter

Hi Peter,

I agree that it's very useful to be able to track these accesses. My impression was that we could track accesses to unmapped (by spec) areas via guest-errors and unmapped/unimplemented areas (due to lack of qemu models) via LOG_UNIMP? As the latter are not really guest-errors..

Cheers,
Edgar