[Qemu-devel] [PATCH v2 38/39] hw/dma/i82374: avoid multiple creations on the same ISA bus

Philippe Mathieu-Daudé posted 39 patches 8 years, 3 months ago
There is a newer version of this series
[Qemu-devel] [PATCH v2 38/39] hw/dma/i82374: avoid multiple creations on the same ISA bus
Posted by Philippe Mathieu-Daudé 8 years, 3 months ago
$ ./ppc64-softmmu/qemu-system-ppc64 -S -machine 40p,accel=tcg -device i82374
qemu-system-ppc64: -device i82374: DMA already initialized on ISA bus

Reported-by: Eduardo Otubo <otubo@redhat.com>
Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/dma/i82374.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
index 6c0f975df0..280e64f0fa 100644
--- a/hw/dma/i82374.c
+++ b/hw/dma/i82374.c
@@ -23,6 +23,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "hw/isa/isa.h"
 
 #define TYPE_I82374 "i82374"
@@ -117,13 +118,19 @@ static const MemoryRegionPortio i82374_portio_list[] = {
 static void i82374_realize(DeviceState *dev, Error **errp)
 {
     I82374State *s = I82374(dev);
+    ISABus *isa_bus = isa_bus_from_device(ISA_DEVICE(dev));
+
+    if (isa_bus->dma[0] || isa_bus->dma[1]) {
+        error_setg(errp, "DMA already initialized on ISA bus");
+        return;
+    }
 
     portio_list_init(&s->port_list, OBJECT(s), i82374_portio_list, s,
                      "i82374");
     portio_list_add(&s->port_list, isa_address_space_io(&s->parent_obj),
                     s->iobase);
 
-    DMA_init(isa_bus_from_device(ISA_DEVICE(dev)), 1);
+    DMA_init(isa_bus, 1);
     memset(s->commands, 0, sizeof(s->commands));
 }
 
-- 
2.15.0.rc0


Re: [Qemu-devel] [PATCH v2 38/39] hw/dma/i82374: avoid multiple creations on the same ISA bus
Posted by Thomas Huth 8 years, 3 months ago
On 17.10.2017 02:12, Philippe Mathieu-Daudé wrote:
> $ ./ppc64-softmmu/qemu-system-ppc64 -S -machine 40p,accel=tcg -device i82374
> qemu-system-ppc64: -device i82374: DMA already initialized on ISA bus
> 
> Reported-by: Eduardo Otubo <otubo@redhat.com>
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/dma/i82374.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/dma/i82374.c b/hw/dma/i82374.c
> index 6c0f975df0..280e64f0fa 100644
> --- a/hw/dma/i82374.c
> +++ b/hw/dma/i82374.c
> @@ -23,6 +23,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qapi/error.h"
>  #include "hw/isa/isa.h"
>  
>  #define TYPE_I82374 "i82374"
> @@ -117,13 +118,19 @@ static const MemoryRegionPortio i82374_portio_list[] = {
>  static void i82374_realize(DeviceState *dev, Error **errp)
>  {
>      I82374State *s = I82374(dev);
> +    ISABus *isa_bus = isa_bus_from_device(ISA_DEVICE(dev));
> +
> +    if (isa_bus->dma[0] || isa_bus->dma[1]) {
> +        error_setg(errp, "DMA already initialized on ISA bus");
> +        return;
> +    }

I think it'd be somewhat cleaner to do this check within DMA_init() -
and DMA_init() then should be provided with an errp parameter, too.
Since you then have to touch all callers, it would then maybe also make
sense to merge this with the next patch where you've got to touch all
callers due to the renaming of the function anyway.

>      portio_list_init(&s->port_list, OBJECT(s), i82374_portio_list, s,
>                       "i82374");
>      portio_list_add(&s->port_list, isa_address_space_io(&s->parent_obj),
>                      s->iobase);
>  
> -    DMA_init(isa_bus_from_device(ISA_DEVICE(dev)), 1);
> +    DMA_init(isa_bus, 1);
>      memset(s->commands, 0, sizeof(s->commands));
>  }

 Thomas