[Qemu-devel] Extending qemu_irq for reset signals

[Qemu-devel] Extending qemu_irq for reset signals

[Blue Swirl]

Hi,

I'd like to use similar mechanism as qemu_irq as reset signal for
devices. The difference is that the opaque data and callback are not
specified at the time of creating the signal at upstream device, but
when the receiving device is created. This does not fit current
qemu_irq model.

When you implemented qemu_irq there was some discussion about
extending qemu_irq to support arbitrary signals. How should we
proceed?

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

[Paul Brook]

On Wednesday 15 August 2007, Blue Swirl wrote:
> Hi,
>
> I'd like to use similar mechanism as qemu_irq as reset signal for
> devices. The difference is that the opaque data and callback are not
> specified at the time of creating the signal at upstream device, but
> when the receiving device is created. This does not fit current
> qemu_irq model.

Either your confused, or not expressing what you want very well. What you 
describe is how qemu_irq works.

The "upstream device" and "receiving device" are the same thing.

The whole point of qemu_irq is to be able to send a single-bit signal without 
having to know or care where it's going. ie. when creating the board/cpu, you 
create the qemu_irq object with appropriate parameters. Then the device 
initiating the reset uses that qemu_irq object.

If the device initiating the reset needs to pass additional information, then 
you're no longer dealing with a single bit of state, so I don't think 
qemu_irq is really appropriate.

Paul

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

[Blue Swirl]

On 8/15/07, Paul Brook <paul@codesourcery.com> wrote:
> On Wednesday 15 August 2007, Blue Swirl wrote:
> > Hi,
> >
> > I'd like to use similar mechanism as qemu_irq as reset signal for
> > devices. The difference is that the opaque data and callback are not
> > specified at the time of creating the signal at upstream device, but
> > when the receiving device is created. This does not fit current
> > qemu_irq model.
>
> Either your confused, or not expressing what you want very well. What you
> describe is how qemu_irq works.
>
> The "upstream device" and "receiving device" are the same thing.

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.

> The whole point of qemu_irq is to be able to send a single-bit signal without
> having to know or care where it's going. ie. when creating the board/cpu, you
> create the qemu_irq object with appropriate parameters. Then the device
> initiating the reset uses that qemu_irq object.
>
> If the device initiating the reset needs to pass additional information, then
> you're no longer dealing with a single bit of state, so I don't think
> qemu_irq is really appropriate.

Agreed. No data needs to be passed in my case and the qemu_irq model
works except for the API.

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

[Paul Brook]

> 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.

Paul

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

[Paul Brook]

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.

Paul

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

[Blue Swirl]

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);
?

There is a small problem that DMA may not assert the reset signal
before ESP gets the data in place. Usually devices perform reset when
created.

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

[Paul Brook]

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);
}

Paul

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

[Blue Swirl]

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.

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

[Blue Swirl]

[-- 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;
 }

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

[Paul Brook]

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

I'd probably make it take notice of the level parameter, and lower the line 
immediately after raising it. Future implementations of qemu_irq may cache 
the value internally.

Other than that, looks ok, though your patch doesn't apply against current 
cvs.

Paul

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

[Blue Swirl]

Preferably also the callback function type should be QEMUResetHandler,
not the slightly different qemu_irq_handler.