Re: [Qemu-devel] [Qemu-trivial] [PATCH] ioport/memory: check that both .read and .write callbacks are defined
Hi, On Mon, Jun 10, 2013 at 3:27 PM, Gerd Hoffmann kra...@redhat.com wrote: Hi, 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 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. 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 cheers, Gerd
Re: [Qemu-devel] [Qemu-trivial] [PATCH] ioport/memory: check that both .read and .write callbacks are defined
On Mon, Jun 10, 2013 at 07:14:45PM +1000, Peter Crosthwaite wrote: Hi, On Mon, Jun 10, 2013 at 3:27 PM, Gerd Hoffmann kra...@redhat.com wrote: Hi, 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 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. 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. cheers, Gerd
Re: [Qemu-devel] [Qemu-trivial] [PATCH] ioport/memory: check that both .read and .write callbacks are defined
Hi Michael, On Tue, Jun 11, 2013 at 3:06 AM, Michael S. Tsirkin m...@redhat.com wrote: On Mon, Jun 10, 2013 at 07:14:45PM +1000, Peter Crosthwaite wrote: Hi, On Mon, Jun 10, 2013 at 3:27 PM, Gerd Hoffmann kra...@redhat.com wrote: Hi, 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 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. 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. 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 cheers, Gerd
Re: [Qemu-devel] [Qemu-trivial] [PATCH] ioport/memory: check that both .read and .write callbacks are defined
On Tue, Jun 11, 2013 at 08:30:12AM +1000, Peter Crosthwaite wrote: Hi Michael, On Tue, Jun 11, 2013 at 3:06 AM, Michael S. Tsirkin m...@redhat.com wrote: On Mon, Jun 10, 2013 at 07:14:45PM +1000, Peter Crosthwaite wrote: Hi, On Mon, Jun 10, 2013 at 3:27 PM, Gerd Hoffmann kra...@redhat.com wrote: Hi, 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 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. 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. 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. 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
Re: [Qemu-devel] [Qemu-trivial] [PATCH] ioport/memory: check that both .read and .write callbacks are defined
Hi, 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 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
Re: [Qemu-devel] [Qemu-trivial] [PATCH] ioport/memory: check that both .read and .write callbacks are defined
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 hpous...@reactos.org
---
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;
