Re: [Qemu-devel] Re: Extending qemu_irq for reset signals

["Blue Swirl" <blauwirbel@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2683 bytes --]

On 8/15/07, Blue Swirl <blauwirbel@gmail.com> wrote:
> On 8/15/07, Paul Brook <paul@codesourcery.com> wrote:
> > On Wednesday 15 August 2007, Blue Swirl wrote:
> > > On 8/15/07, Paul Brook <paul@codesourcery.com> wrote:
> > > > On Wednesday 15 August 2007, Paul Brook wrote:
> > > > > > Okay, more explaining. This is the case where I'd want to use the
> > > > > > signal: DMA controller ("upstream") can reset the slave device (ESP
> > > > > > or Lance). DMA controller is created first and I also want to
> > > > > > allocate reset signals at that point. Later when ESP is created, it
> > > > > > should be possible to put ESP reset function and opaque data to the
> > > > > > signal given but this is not possible with current API. Currently the
> > > > > > DMA data would be passed to qemu_allocate_irqs.
> > > > >
> > > > > Ah, I see. The problem here is that you've got a cyclic dependency. For
> > > > > DMA operations the ESP is in charge, so it makes sense to create the
> > > > > subservient DMA device first. For the reset signals the DMA controller
> > > > > is in charge so ideally you create the ESP device first. Because the
> > > > > DMA interface is most complicated, it's probably takes precedence.
> > > > >
> > > > > I think you need to modify or use sparc32_dma_set_reset_data to take a
> > > > > qemu_irq rather than a callback and opaque argument. Alternatively you
> > > > > can move things around a bit and have the sun4m code do something
> > > > > similar. i.e. the ESP and lance devices return the reset lines, then
> > > > > the sun4m code pokes into the DMA device state.
> > > >
> > > > Oh, or you can pass a pointer to a qemu_irq from the DMA to the ESP and
> > > > have the ESP poke its reset object in there that way.
> > >
> > > That's what I had in mind. Should I just extend the API for example with
> > > /* Change the callback function and/or data */
> > > void qemu_irq_change(qemu_irq irq, qemu_irq_handler handler,
> > >                      void *opaque);
> >
> > I'm not so keen on that. It breaks the nice property of having the IRQ lines
> > owned by the receiver. I was thinking something like:
> >
> > struct DMAState {
> >   qemu_irq device_reset;
> > };
> > qemu_irq *dma_init()
> > {
> >   struct DMAState * d = malloc();
> >   return &d->device_reset;
> > }
> >
> > void esp_init(qemu_irq *resetp)
> > {
> >   ESPState *d = malloc();
> >   *reset = *qemu_irq_alloc(d, 1);
> > }
> >
> > void sun4m_init()
> > {
> >   qemu_irq *p = dma_init();
> >   esp_init(p);
> > }
>
> Yes, that would work. I wasn't too happy about the change function either.
>

I implemented reset that way with qemu_irq, the result survives some
quick tests.

[-- Attachment #2: sparc32_reset.diff --]
[-- Type: text/x-diff, Size: 7244 bytes --]

Index: qemu/hw/esp.c
===================================================================
--- qemu.orig/hw/esp.c	2007-08-15 18:58:02.000000000 +0000
+++ qemu/hw/esp.c	2007-08-15 19:22:38.000000000 +0000
@@ -344,6 +344,11 @@
     s->do_cmd = 0;
 }
 
+static void parent_esp_reset(void *opaque, int irq, int level)
+{
+    esp_reset(opaque);
+}
+
 static uint32_t esp_mem_readb(void *opaque, target_phys_addr_t addr)
 {
     ESPState *s = opaque;
@@ -569,7 +574,7 @@
 }
 
 void *esp_init(BlockDriverState **bd, target_phys_addr_t espaddr,
-               void *dma_opaque, qemu_irq irq, qemu_dma *parent_dma)
+               qemu_irq irq, qemu_dma *parent_dma, qemu_irq *reset)
 {
     ESPState *s;
     int esp_io_memory;
@@ -581,7 +586,6 @@
     s->bd = bd;
     s->irq = irq;
     s->parent_dma = parent_dma;
-    sparc32_dma_set_reset_data(dma_opaque, esp_reset, s);
 
     esp_io_memory = cpu_register_io_memory(0, esp_mem_read, esp_mem_write, s);
     cpu_register_physical_memory(espaddr, ESP_SIZE, esp_io_memory);
@@ -591,5 +595,7 @@
     register_savevm("esp", espaddr, 3, esp_save, esp_load, s);
     qemu_register_reset(esp_reset, s);
 
+    *reset = *qemu_allocate_irqs(parent_esp_reset, s, 1);
+
     return s;
 }
Index: qemu/hw/sun4m.c
===================================================================
--- qemu.orig/hw/sun4m.c	2007-08-15 18:58:02.000000000 +0000
+++ qemu/hw/sun4m.c	2007-08-15 19:05:57.000000000 +0000
@@ -322,6 +322,7 @@
     qemu_irq *cpu_irqs[MAX_CPUS], *slavio_irq, *slavio_cpu_irq,
         *espdma_irq, *ledma_irq;
     qemu_dma *physical_dma, *dvma, *esp_dvma, *le_dvma;
+    qemu_irq *esp_reset, *le_reset;
 
     /* init CPUs */
     sparc_find_by_name(cpu_model, &def);
@@ -362,11 +363,11 @@
                                        hwdef->clock_irq);
 
     espdma = sparc32_dma_init(hwdef->dma_base, slavio_irq[hwdef->esp_irq],
-                              &espdma_irq, dvma, &esp_dvma, 1);
+                              &espdma_irq, dvma, &esp_dvma, 1, &esp_reset);
 
     ledma = sparc32_dma_init(hwdef->dma_base + 16ULL,
                              slavio_irq[hwdef->le_irq], &ledma_irq, dvma,
-                             &le_dvma, 0);
+                             &le_dvma, 0, &le_reset);
 
     if (graphic_depth != 8 && graphic_depth != 24) {
         fprintf(stderr, "qemu: Unsupported depth: %d\n", graphic_depth);
@@ -377,7 +378,7 @@
 
     if (nd_table[0].model == NULL
         || strcmp(nd_table[0].model, "lance") == 0) {
-        lance_init(&nd_table[0], hwdef->le_base, ledma, *ledma_irq, le_dvma);
+        lance_init(&nd_table[0], hwdef->le_base, *ledma_irq, le_dvma, le_reset);
     } else if (strcmp(nd_table[0].model, "?") == 0) {
         fprintf(stderr, "qemu: Supported NICs: lance\n");
         exit (1);
@@ -402,8 +403,8 @@
                        serial_hds[1], serial_hds[0]);
     fdctrl_init(slavio_irq[hwdef->fd_irq], 0, 1, hwdef->fd_base, fd_table);
 
-    main_esp = esp_init(bs_table, hwdef->esp_base, espdma, *espdma_irq,
-                        esp_dvma);
+    main_esp = esp_init(bs_table, hwdef->esp_base, *espdma_irq, esp_dvma,
+                        esp_reset);
 
     for (i = 0; i < MAX_DISKS; i++) {
         if (bs_table[i]) {
Index: qemu/vl.h
===================================================================
--- qemu.orig/vl.h	2007-08-15 18:58:02.000000000 +0000
+++ qemu/vl.h	2007-08-15 19:05:57.000000000 +0000
@@ -1097,8 +1097,8 @@
 /* pcnet.c */
 
 void pci_pcnet_init(PCIBus *bus, NICInfo *nd, int devfn);
-void lance_init(NICInfo *nd, target_phys_addr_t leaddr, void *dma_opaque,
-                qemu_irq irq, qemu_dma *parent_dma);
+void lance_init(NICInfo *nd, target_phys_addr_t leaddr, qemu_irq irq,
+                qemu_dma *parent_dma, qemu_irq *reset);
 
 /* vmmouse.c */
 void *vmmouse_init(void *m);
@@ -1303,14 +1303,12 @@
 /* esp.c */
 void esp_scsi_attach(void *opaque, BlockDriverState *bd, int id);
 void *esp_init(BlockDriverState **bd, target_phys_addr_t espaddr,
-               void *dma_opaque, qemu_irq irq, qemu_dma *parent_dma);
+               qemu_irq irq, qemu_dma *parent_dma, qemu_irq *reset);
 
 /* sparc32_dma.c */
 void *sparc32_dma_init(target_phys_addr_t daddr, qemu_irq parent_irq,
                        qemu_irq **dev_irq, qemu_dma *parent_dma,
-                       qemu_dma **dev_dma, int is_espdma);
-void sparc32_dma_set_reset_data(void *opaque, void (*dev_reset)(void *opaque),
-                                void *dev_opaque);
+                       qemu_dma **dev_dma, int is_espdma, qemu_irq **reset);
 
 /* cs4231.c */
 void cs_init(target_phys_addr_t base, int irq, void *intctl);
Index: qemu/hw/pcnet.c
===================================================================
--- qemu.orig/hw/pcnet.c	2007-08-15 18:58:02.000000000 +0000
+++ qemu/hw/pcnet.c	2007-08-15 19:05:57.000000000 +0000
@@ -2052,6 +2052,11 @@
     }
 }
 
+static void parent_lance_reset(void *opaque, int irq, int level)
+{
+    pcnet_h_reset(opaque);
+}
+
 static void lance_mem_writew(void *opaque, target_phys_addr_t addr,
                              uint32_t val)
 {
@@ -2087,8 +2092,8 @@
     lance_mem_writew,
 };
 
-void lance_init(NICInfo *nd, target_phys_addr_t leaddr, void *dma_opaque,
-                qemu_irq irq, qemu_dma *parent_dma)
+void lance_init(NICInfo *nd, target_phys_addr_t leaddr, qemu_irq irq,
+                qemu_dma *parent_dma, qemu_irq *reset)
 {
     PCNetState *d;
     int lance_io_memory;
@@ -2101,7 +2106,7 @@
         cpu_register_io_memory(0, lance_mem_read, lance_mem_write, d);
 
     d->dma_opaque = parent_dma;
-    sparc32_dma_set_reset_data(dma_opaque, pcnet_h_reset, d);
+    *reset = *qemu_allocate_irqs(parent_lance_reset, d, 1);
 
     cpu_register_physical_memory(leaddr, 4, lance_io_memory);
 
Index: qemu/hw/sparc32_dma.c
===================================================================
--- qemu.orig/hw/sparc32_dma.c	2007-08-15 18:58:02.000000000 +0000
+++ qemu/hw/sparc32_dma.c	2007-08-15 19:25:46.000000000 +0000
@@ -58,9 +58,8 @@
 struct DMAState {
     uint32_t dmaregs[DMA_REGS];
     qemu_irq irq;
-    void *dev_opaque;
-    void (*dev_reset)(void *dev_opaque);
     qemu_irq *pic;
+    qemu_irq dev_reset;
     qemu_dma *dma;
 };
 
@@ -132,7 +131,7 @@
             qemu_irq_lower(s->irq);
         }
         if (val & DMA_RESET) {
-            s->dev_reset(s->dev_opaque);
+            qemu_irq_raise(s->dev_reset);
         } else if (val & DMA_DRAIN_FIFO) {
             val &= ~DMA_DRAIN_FIFO;
         } else if (val == 0)
@@ -193,7 +192,7 @@
 
 void *sparc32_dma_init(target_phys_addr_t daddr, qemu_irq parent_irq,
                        qemu_irq **dev_irq, qemu_dma *parent_dma,
-                       qemu_dma **dev_dma, int is_espdma)
+                       qemu_dma **dev_dma, int is_espdma, qemu_irq **reset)
 {
     DMAState *s;
     int dma_io_memory;
@@ -217,14 +216,7 @@
     else
         *dev_dma = qemu_init_dma(ledma_memory_rw, s);
 
-    return s;
-}
-
-void sparc32_dma_set_reset_data(void *opaque, void (*dev_reset)(void *opaque),
-                                void *dev_opaque)
-{
-    DMAState *s = opaque;
+    *reset = &s->dev_reset;
 
-    s->dev_reset = dev_reset;
-    s->dev_opaque = dev_opaque;
+    return s;
 }