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

[Thomas Huth <thuth@redhat.com>]

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