[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [PATCH] ioport/memory: check that both .read and .wri
From: |
Michael Tokarev |
Subject: |
Re: [Qemu-trivial] [PATCH] ioport/memory: check that both .read and .write callbacks are defined |
Date: |
Sat, 08 Jun 2013 11:51:11 +0400 |
User-agent: |
Mozilla/5.0 (X11; Linux i686 on x86_64; rv:17.0) Gecko/17.0 Icedove/17.0 |
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;
>