Re: [Qemu-trivial] [PATCH] ioport/memory: check that both .read and .write callbacks are defined

[Michael Tokarev]

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

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

> Signed-off-by: Hervé Poussineau <address@hidden>
> ---
> 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(+)
> 
> diff --git a/ioport.c b/ioport.c
> index a0ac2a0..8dd9d50 100644
> --- a/ioport.c
> +++ b/ioport.c
> @@ -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;
>      }
>  
> diff --git a/memory.c b/memory.c
> index 5cb8f4a..654d1ce 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -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;
>