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

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

[Hervé Poussineau]

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

Signed-off-by: Hervé Poussineau <hpoussin@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;
-- 
1.7.10.4

Re: [Qemu-devel] [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 <hpoussin@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;
> 

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

[Gerd Hoffmann]

  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

[Peter Crosthwaite]

Hi,

On Mon, Jun 10, 2013 at 3:27 PM, Gerd Hoffmann <kraxel@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

[Michael S. Tsirkin]

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 <kraxel@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

[Peter Crosthwaite]

Hi Michael,

On Tue, Jun 11, 2013 at 3:06 AM, Michael S. Tsirkin <mst@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 <kraxel@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

[Edgar E. Iglesias]

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 <mst@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 <kraxel@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