Patchwork [03/13] ide: add support for ide bus ops

login
register
mail settings
Submitter Alexander Graf
Date Nov. 26, 2010, 7:17 p.m.
Message ID <1290799053-27282-4-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/73225/
State New
Headers show

Comments

Alexander Graf - Nov. 26, 2010, 7:17 p.m.
From: Roland Elek <elek.roland@gmail.com>

We need to hook into some of the core IDE functionality for AHCI. To
do that, the easiest way is to make explicit functions calls be implicit
through a function call struct.

Signed-off-by: Roland Elek <elek.roland@gmail.com>
Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - rename IDEExtender to IDEBusOps and make a pointer (kraxel)
---
 hw/ide/core.c     |   36 +++++++++++++++++++++++++++++++++++-
 hw/ide/internal.h |   26 +++++++++++++++++++-------
 2 files changed, 54 insertions(+), 8 deletions(-)
Stefan Hajnoczi - Dec. 1, 2010, 1:09 p.m.
On Fri, Nov 26, 2010 at 7:17 PM, Alexander Graf <agraf@suse.de> wrote:
Just some cosmetic suggestions.

> @@ -2716,6 +2736,12 @@ static void ide_init1(IDEBus *bus, int unit)
>                                            ide_sector_write_timer_cb, s);
>  }
>
> +static IDEBusOps ide_bus_ops = {

Since the functions are all pata_* I think it makes sense for this to
be called pata_bus_ops, not ide_bus_ops.

> +    .transfer_start_fn = pata_transfer_start,
> +    .irq_set_fn = pata_set_irq,

irq_set or set_irq? :)  Let's consistently go with set_irq.

Stefan
Kevin Wolf - Dec. 1, 2010, 1:17 p.m.
Am 01.12.2010 14:09, schrieb Stefan Hajnoczi:
> On Fri, Nov 26, 2010 at 7:17 PM, Alexander Graf <agraf@suse.de> wrote:
> Just some cosmetic suggestions.
> 
>> @@ -2716,6 +2736,12 @@ static void ide_init1(IDEBus *bus, int unit)
>>                                            ide_sector_write_timer_cb, s);
>>  }
>>
>> +static IDEBusOps ide_bus_ops = {
> 
> Since the functions are all pata_* I think it makes sense for this to
> be called pata_bus_ops, not ide_bus_ops.
> 
>> +    .transfer_start_fn = pata_transfer_start,
>> +    .irq_set_fn = pata_set_irq,
> 
> irq_set or set_irq? :)  Let's consistently go with set_irq.

And drop the _fn suffix? It's really obvious that these are functions
(if this is what it's meant to say).

Kevin
Alexander Graf - Dec. 1, 2010, 1:20 p.m.
On 01.12.2010, at 14:17, Kevin Wolf wrote:

> Am 01.12.2010 14:09, schrieb Stefan Hajnoczi:
>> On Fri, Nov 26, 2010 at 7:17 PM, Alexander Graf <agraf@suse.de> wrote:
>> Just some cosmetic suggestions.
>> 
>>> @@ -2716,6 +2736,12 @@ static void ide_init1(IDEBus *bus, int unit)
>>>                                           ide_sector_write_timer_cb, s);
>>> }
>>> 
>>> +static IDEBusOps ide_bus_ops = {
>> 
>> Since the functions are all pata_* I think it makes sense for this to
>> be called pata_bus_ops, not ide_bus_ops.
>> 
>>> +    .transfer_start_fn = pata_transfer_start,
>>> +    .irq_set_fn = pata_set_irq,
>> 
>> irq_set or set_irq? :)  Let's consistently go with set_irq.
> 
> And drop the _fn suffix? It's really obvious that these are functions
> (if this is what it's meant to say).

Good points :)


Alex

Patch

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 1849069..c8d7810 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -67,6 +67,8 @@  static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret);
 static int ide_handle_rw_error(IDEState *s, int error, int op);
 static void ide_flush_cache(IDEState *s);
 
+static void pata_dma_start(IDEState *s, BlockDriverCompletionFunc *dma_cb);
+
 static void padstr(char *str, const char *src, int len)
 {
     int i, v;
@@ -325,6 +327,12 @@  static inline void ide_dma_submit_check(IDEState *s,
 static void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
                                EndTransferFunc *end_transfer_func)
 {
+    s->bus->ops->transfer_start_fn(s,buf,size,end_transfer_func);
+}
+
+static void pata_transfer_start(IDEState *s, uint8_t *buf, int size,
+                               EndTransferFunc *end_transfer_func)
+{
     s->end_transfer_func = end_transfer_func;
     s->data_ptr = buf;
     s->data_end = buf + size;
@@ -2580,6 +2588,18 @@  static void ide_dummy_transfer_stop(IDEState *s)
     s->io_buffer[3] = 0xff;
 }
 
+static void pata_set_irq(IDEBus *bus)
+{
+    BMDMAState *bm = bus->bmdma;
+
+    if (!(bus->cmd & IDE_CMD_DISABLE_IRQ)) {
+        if (bm) {
+            bm->status |= BM_STATUS_INT;
+        }
+        qemu_irq_raise(bus->irq);
+    }
+}
+
 static void ide_reset(IDEState *s)
 {
 #ifdef DEBUG_IDE
@@ -2716,6 +2736,12 @@  static void ide_init1(IDEBus *bus, int unit)
                                            ide_sector_write_timer_cb, s);
 }
 
+static IDEBusOps ide_bus_ops = {
+    .transfer_start_fn = pata_transfer_start,
+    .irq_set_fn = pata_set_irq,
+    .dma_start_fn = pata_dma_start,
+};
+
 void ide_init2(IDEBus *bus, qemu_irq irq)
 {
     int i;
@@ -2725,6 +2751,7 @@  void ide_init2(IDEBus *bus, qemu_irq irq)
         ide_reset(&bus->ifs[i]);
     }
     bus->irq = irq;
+    bus->ops = &ide_bus_ops;
 }
 
 /* TODO convert users to qdev and remove */
@@ -2748,6 +2775,7 @@  void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
         }
     }
     bus->irq = irq;
+    bus->ops = &ide_bus_ops;
 }
 
 void ide_init_ioport(IDEBus *bus, int iobase, int iobase2)
@@ -2919,9 +2947,10 @@  const VMStateDescription vmstate_ide_bus = {
 /***********************************************************/
 /* PCI IDE definitions */
 
-static void ide_dma_start(IDEState *s, BlockDriverCompletionFunc *dma_cb)
+static void pata_dma_start(IDEState *s, BlockDriverCompletionFunc *dma_cb)
 {
     BMDMAState *bm = s->bus->bmdma;
+
     if(!bm)
         return;
     bm->unit = s->unit;
@@ -2936,6 +2965,11 @@  static void ide_dma_start(IDEState *s, BlockDriverCompletionFunc *dma_cb)
     }
 }
 
+static void ide_dma_start(IDEState *s, BlockDriverCompletionFunc *dma_cb)
+{
+    s->bus->ops->dma_start_fn(s,dma_cb);
+}
+
 static void ide_dma_restart(IDEState *s, int is_read)
 {
     BMDMAState *bm = s->bus->bmdma;
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index e7e1f80..ee7e13e 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -20,6 +20,7 @@  typedef struct IDEDevice IDEDevice;
 typedef struct IDEDeviceInfo IDEDeviceInfo;
 typedef struct IDEState IDEState;
 typedef struct BMDMAState BMDMAState;
+typedef struct IDEBusOps IDEBusOps;
 
 /* Bits of HD_STATUS */
 #define ERR_STAT		0x01
@@ -366,6 +367,14 @@  typedef enum { IDE_HD, IDE_CD, IDE_CFATA } IDEDriveKind;
 
 typedef void EndTransferFunc(IDEState *);
 
+
+typedef void TransferStartFunc(IDEState *,
+                             uint8_t *,
+                             int,
+                             EndTransferFunc *);
+typedef void IRQSetFunc(IDEBus *);
+typedef void DMAStartFunc(IDEState *, BlockDriverCompletionFunc *);
+
 /* NOTE: IDEState represents in fact one drive */
 struct IDEState {
     IDEBus *bus;
@@ -442,12 +451,21 @@  struct IDEState {
     uint8_t *smart_selftest_data;
 };
 
+/* This struct represents a device that uses an IDE bus, but requires
+ * modifications to how it works. An example is AHCI. */
+struct IDEBusOps {
+    TransferStartFunc *transfer_start_fn;
+    IRQSetFunc *irq_set_fn;
+    DMAStartFunc *dma_start_fn;
+};
+
 struct IDEBus {
     BusState qbus;
     IDEDevice *master;
     IDEDevice *slave;
     BMDMAState *bmdma;
     IDEState ifs[2];
+    IDEBusOps *ops;
     uint8_t unit;
     uint8_t cmd;
     qemu_irq irq;
@@ -512,13 +530,7 @@  static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
 
 static inline void ide_set_irq(IDEBus *bus)
 {
-    BMDMAState *bm = bus->bmdma;
-    if (!(bus->cmd & IDE_CMD_DISABLE_IRQ)) {
-        if (bm) {
-            bm->status |= BM_STATUS_INT;
-        }
-        qemu_irq_raise(bus->irq);
-    }
+    bus->ops->irq_set_fn(bus);
 }
 
 /* hw/ide/core.c */