Re: [Qemu-devel] [PATCH 0/3] dp8393x update

[Laurent Vivier <Laurent@Vivier.EU>]

[-- Attachment #1.1: Type: text/plain, Size: 1846 bytes --]

Le 02/01/2015 02:34, Laurent Vivier a écrit :
> Hi Hervé,
> 
> Le 01/01/2015 22:01, Hervé Poussineau a écrit :
>> Hi Laurent,
>>
>> Le 29/12/2014 01:39, Laurent Vivier a écrit :
>>> This is a series of patches I wrote to use dp8393x (SONIC) with
>>> Quadra 800 emulation. I think it is interesting to share them with the
>>> mainline.
>>>
>>> Qdev'ifying allows to remove the annoying warning:
>>> "requested NIC (anonymous, model dp83932) was not created
>>>   (not supported by this machine?)"
>>>
>>> [PATCH 1/3] dp8393x: add registers offset
>>> [PATCH 2/3] dp8393x: add PROM to store MAC address
>>> [PATCH 3/3] qdev'ify dp8393x
>>>
>>
>> I also had some patches to QOM'ify dp8393x.
>> Those are available at
>> http://repo.or.cz/w/qemu/hpoussin.git/shortlog/refs/heads/sonic
>>
>> Main differences are:
>> - dp8393x uses an AddressSpace, instead of an offset in a MemoryRegion
>> in yours
>> - no PROM support, but should be easy to add
>> - rc4030 (MIPS Jazz chipset) also converted to QOM (but that was not the
>> goal of your patch series)
>>
>> Minor points are:
>> - have load/save support
>> - all functions have the same dp8393x_ prefix
>> - old_mmio-style functions are not used anymore
>>
>> What do you think of them?
> 
> I don't know if it's a good idea to use AddressSpace into device. For
> me, AddressSpace must stay in the machine definition. SysBus is there
> for that. But it seems to be a good way to do DMA. I have to think about
> that...

Using AddressSpace is really a very good idea, in fact, but I don't like
the way you pass it to the device (a qdev_set_prop()).

I think we should do as it is done in PCI. This must be managed at
sysbus level, not at the device level.

Find attached a (not tested) patch to show what I'm thinking about.

Regards,
Laurent



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: sysbus_dma_rw.patch --]
[-- Type: text/x-patch; name="sysbus_dma_rw.patch", Size: 9358 bytes --]

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 69960ac..8902a4f 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -148,7 +148,6 @@ typedef struct dp8393xState {
 
     /* Hardware */
     uint8_t it_shift;
-    void *as_opaque;
     qemu_irq irq;
 #ifdef DEBUG_SONIC
     int irq_level;
@@ -200,7 +199,7 @@ static void dp8393x_do_load_cam(dp8393xState *s)
 
     while (s->regs[SONIC_CDC] & 0x1f) {
         /* Fill current entry */
-        address_space_rw(s->as,
+        sysbus_dma_rw(SYS_BUS_DEVICE(s),
             (s->regs[SONIC_URRA] << 16) | s->regs[SONIC_CDP],
             (uint8_t *)data, size, 0);
         s->cam[index][0] = data[1 * width] & 0xff;
@@ -219,7 +218,7 @@ static void dp8393x_do_load_cam(dp8393xState *s)
     }
 
     /* Read CAM enable */
-    address_space_rw(s->as,
+    sysbus_dma_rw(SYS_BUS_DEVICE(s),
         (s->regs[SONIC_URRA] << 16) | s->regs[SONIC_CDP],
         (uint8_t *)data, size, 0);
     s->regs[SONIC_CE] = data[0 * width];
@@ -239,7 +238,7 @@ static void dp8393x_do_read_rra(dp8393xState *s)
     /* Read memory */
     width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
     size = sizeof(uint16_t) * 4 * width;
-    address_space_rw(s->as,
+    sysbus_dma_rw(SYS_BUS_DEVICE(s),
         (s->regs[SONIC_URRA] << 16) | s->regs[SONIC_RRP],
         (uint8_t *)data, size, 0);
 
@@ -352,7 +351,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
                 (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_CTDA]);
         size = sizeof(uint16_t) * 6 * width;
         s->regs[SONIC_TTDA] = s->regs[SONIC_CTDA];
-        address_space_rw(s->as,
+        sysbus_dma_rw(SYS_BUS_DEVICE(s),
             ((s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA]) + sizeof(uint16_t) * width,
             (uint8_t *)data, size, 0);
         tx_len = 0;
@@ -378,7 +377,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
             if (tx_len + len > sizeof(s->tx_buffer)) {
                 len = sizeof(s->tx_buffer) - tx_len;
             }
-            address_space_rw(s->as,
+            sysbus_dma_rw(SYS_BUS_DEVICE(s),
                 (s->regs[SONIC_TSA1] << 16) | s->regs[SONIC_TSA0],
                 &s->tx_buffer[tx_len], len, 0);
             tx_len += len;
@@ -387,7 +386,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
             if (i != s->regs[SONIC_TFC]) {
                 /* Read next fragment details */
                 size = sizeof(uint16_t) * 3 * width;
-                address_space_rw(s->as,
+                sysbus_dma_rw(SYS_BUS_DEVICE(s),
                     ((s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA]) + sizeof(uint16_t) * (4 + 3 * i) * width,
                     (uint8_t *)data, size, 0);
                 s->regs[SONIC_TSA0] = data[0 * width];
@@ -421,14 +420,14 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
         /* Write status */
         data[0 * width] = s->regs[SONIC_TCR] & 0x0fff; /* status */
         size = sizeof(uint16_t) * width;
-        address_space_rw(s->as,
+        sysbus_dma_rw(SYS_BUS_DEVICE(s),
             (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA],
             (uint8_t *)data, size, 1);
 
         if (!(s->regs[SONIC_CR] & SONIC_CR_HTX)) {
             /* Read footer of packet */
             size = sizeof(uint16_t) * width;
-            address_space_rw(s->as,
+            sysbus_dma_rw(SYS_BUS_DEVICE(s),
                 ((s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA]) + sizeof(uint16_t) * (4 + 3 * s->regs[SONIC_TFC]) * width,
                 (uint8_t *)data, size, 0);
             s->regs[SONIC_CTDA] = data[0 * width] & ~0x1;
@@ -695,7 +694,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
         /* Are we still in resource exhaustion? */
         size = sizeof(uint16_t) * 1 * width;
         address = ((s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA]) + sizeof(uint16_t) * 5 * width;
-        address_space_rw(s->as, address, (uint8_t*)data, size, 0);
+        sysbus_dma_rw(SYS_BUS_DEVICE(s), address, (uint8_t*)data, size, 0);
         if (data[0 * width] & 0x1) {
             /* Still EOL ; stop reception */
             return -1;
@@ -714,9 +713,9 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     /* Put packet into RBA */
     DPRINTF("Receive packet at %08x\n", (s->regs[SONIC_CRBA1] << 16) | s->regs[SONIC_CRBA0]);
     address = (s->regs[SONIC_CRBA1] << 16) | s->regs[SONIC_CRBA0];
-    address_space_rw(s->as, address, (uint8_t*)buf, rx_len, 1);
+    sysbus_dma_rw(SYS_BUS_DEVICE(s), address, (uint8_t*)buf, rx_len, 1);
     address += rx_len;
-    address_space_rw(s->as, address, (uint8_t*)&checksum, 4, 1);
+    sysbus_dma_rw(SYS_BUS_DEVICE(s), address, (uint8_t*)&checksum, 4, 1);
     rx_len += 4;
     s->regs[SONIC_CRBA1] = address >> 16;
     s->regs[SONIC_CRBA0] = address & 0xffff;
@@ -744,11 +743,12 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
     data[3 * width] = s->regs[SONIC_TRBA1]; /* pkt_ptr1 */
     data[4 * width] = s->regs[SONIC_RSC]; /* seq_no */
     size = sizeof(uint16_t) * 5 * width;
-    address_space_rw(s->as, (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA], (uint8_t *)data, size, 1);
+    sysbus_dma_rw(SYS_BUS_DEVICE(s),
+                  (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA], (uint8_t *)data, size, 1);
 
     /* Move to next descriptor */
     size = sizeof(uint16_t) * width;
-    address_space_rw(s->as,
+    sysbus_dma_rw(SYS_BUS_DEVICE(s),
         ((s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA]) + sizeof(uint16_t) * 5 * width,
         (uint8_t *)data, size, 0);
     s->regs[SONIC_LLFA] = data[0 * width];
@@ -757,7 +757,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
         s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
     } else {
         data[0 * width] = 0; /* in_use */
-        address_space_rw(s->as,
+        sysbus_dma_rw(SYS_BUS_DEVICE(s),
             ((s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA]) + sizeof(uint16_t) * 6 * width,
             (uint8_t *)data, size, 1);
         s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
@@ -828,12 +828,12 @@ void dp83932_init(NICInfo *nd, hwaddr base, int it_shift,
 
     dev = qdev_create(NULL, "dp83932");
     qdev_prop_set_uint8(dev, "it-shift", it_shift);
-    qdev_prop_set_ptr(dev, "address-space", as);
     qdev_set_nic_properties(dev, nd);
     qdev_init_nofail(dev);
     sysbus = SYS_BUS_DEVICE(dev);
     sysbus_mmio_map(sysbus, 0, base);
     sysbus_connect_irq(sysbus, 0, irq);
+    sysbus_set_address_space(sysbus, as);
 }
 
 static int dp8393x_initfn(SysBusDevice *sysbus)
@@ -841,7 +841,6 @@ static int dp8393x_initfn(SysBusDevice *sysbus)
     DeviceState *dev = DEVICE(sysbus);
     dp8393xState *s = DP8393X(sysbus);
 
-    s->as = s->as_opaque; /* cast address space to right type */
     s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
     s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
 
@@ -871,7 +870,6 @@ static const VMStateDescription vmstate_dp8393x = {
 
 static Property dp8393x_properties[] = {
     DEFINE_PROP_UINT8("it-shift", dp8393xState, it_shift, 2),
-    DEFINE_PROP_PTR("address-space", dp8393xState, as_opaque),
     DEFINE_NIC_PROPERTIES(dp8393xState, conf),
     DEFINE_PROP_END_OF_LIST(),
 };
diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index d1f3f00..d1e027b 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -3,6 +3,7 @@
 
 /* Devices attached directly to the main system bus.  */
 
+#include "sysemu/dma.h"
 #include "hw/qdev.h"
 #include "exec/memory.h"
 
@@ -53,6 +54,7 @@ struct SysBusDevice {
         hwaddr addr;
         MemoryRegion *memory;
     } mmio[QDEV_MAX_MMIO];
+    AddressSpace *sysbus_as;
     int num_pio;
     pio_addr_t pio[QDEV_MAX_PIO];
 };
@@ -78,6 +80,37 @@ void sysbus_add_io(SysBusDevice *dev, hwaddr addr,
                    MemoryRegion *mem);
 MemoryRegion *sysbus_address_space(SysBusDevice *dev);
 
+static inline AddressSpace *sysbus_get_address_space(SysBusDevice *dev)
+{
+    return dev->sysbus_as;
+}
+
+static inline void sysbus_set_address_space(SysBusDevice *dev,
+                                            AddressSpace *as)
+{
+     dev->sysbus_as = as;
+}
+
+static inline int sysbus_dma_rw(SysBusDevice *dev, dma_addr_t addr,
+                                void *buf, dma_addr_t len, DMADirection dir)
+{
+    dma_memory_rw(sysbus_get_address_space(dev), addr, buf, len, dir);
+    return 0;
+}
+
+static inline int sysbus_dma_read(SysBusDevice *dev, dma_addr_t addr,
+                               void *buf, dma_addr_t len)
+{
+    return sysbus_dma_rw(dev, addr, buf, len, DMA_DIRECTION_TO_DEVICE);
+}
+
+static inline int sysbus_dma_write(SysBusDevice *dev, dma_addr_t addr,
+                                const void *buf, dma_addr_t len)
+{
+    return sysbus_dma_rw(dev, addr, (void *) buf, len,
+                         DMA_DIRECTION_FROM_DEVICE);
+}
+
 /* Call func for every dynamically created sysbus device in the system */
 void foreach_dynamic_sysbus_device(FindSysbusDeviceFunc *func, void *opaque);
 

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]