Re: [Qemu-devel] [PATCH 05/14] eepro100: Use PCI DMA stub functions

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Oct 31, 2011 at 05:06:49PM +1100, David Gibson wrote:
> From: Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro>
> 
> This updates the eepro100 device emulation to use the explicit PCI DMA
> functions, instead of directly calling physical memory access functions.
> 
> Signed-off-by: Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro>
> Signed-off-by: David Gibson <dwg@au1.ibm.com>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  hw/eepro100.c |  121 +++++++++++++++++++++++----------------------------------
>  1 files changed, 49 insertions(+), 72 deletions(-)
> 
> diff --git a/hw/eepro100.c b/hw/eepro100.c
> index 4e3c52f..7d59e71 100644
> --- a/hw/eepro100.c
> +++ b/hw/eepro100.c
> @@ -46,6 +46,7 @@
>  #include "net.h"
>  #include "eeprom93xx.h"
>  #include "sysemu.h"
> +#include "dma.h"
>  
>  /* QEMU sends frames smaller than 60 bytes to ethernet nics.
>   * Such frames are rejected by real nics and their emulations.
> @@ -315,38 +316,6 @@ static const uint16_t eepro100_mdi_mask[] = {
>      0xffff, 0xffff, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000, 0x0000,
>  };
>  
> -/* Read a 16 bit little endian value from physical memory. */
> -static uint16_t e100_ldw_le_phys(target_phys_addr_t addr)
> -{
> -    /* Load 16 bit (little endian) word from emulated hardware. */
> -    uint16_t val;
> -    cpu_physical_memory_read(addr, &val, sizeof(val));
> -    return le16_to_cpu(val);
> -}
> -
> -/* Read a 32 bit little endian value from physical memory. */
> -static uint32_t e100_ldl_le_phys(target_phys_addr_t addr)
> -{
> -    /* Load 32 bit (little endian) word from emulated hardware. */
> -    uint32_t val;
> -    cpu_physical_memory_read(addr, &val, sizeof(val));
> -    return le32_to_cpu(val);
> -}
> -
> -/* Write a 16 bit little endian value to physical memory. */
> -static void e100_stw_le_phys(target_phys_addr_t addr, uint16_t val)
> -{
> -    val = cpu_to_le16(val);
> -    cpu_physical_memory_write(addr, &val, sizeof(val));
> -}
> -
> -/* Write a 32 bit little endian value to physical memory. */
> -static void e100_stl_le_phys(target_phys_addr_t addr, uint32_t val)
> -{
> -    val = cpu_to_le32(val);
> -    cpu_physical_memory_write(addr, &val, sizeof(val));
> -}
> -
>  #define POLYNOMIAL 0x04c11db6
>  
>  /* From FreeBSD */
> @@ -744,21 +713,26 @@ static void dump_statistics(EEPRO100State * s)
>       * values which really matter.
>       * Number of data should check configuration!!!
>       */
> -    cpu_physical_memory_write(s->statsaddr, &s->statistics, s->stats_size);
> -    e100_stl_le_phys(s->statsaddr + 0, s->statistics.tx_good_frames);
> -    e100_stl_le_phys(s->statsaddr + 36, s->statistics.rx_good_frames);
> -    e100_stl_le_phys(s->statsaddr + 48, s->statistics.rx_resource_errors);
> -    e100_stl_le_phys(s->statsaddr + 60, s->statistics.rx_short_frame_errors);
> +    pci_dma_write(&s->dev, s->statsaddr,
> +                  (uint8_t *) &s->statistics, s->stats_size);
> +    stl_le_pci_dma(&s->dev, s->statsaddr + 0,
> +                   s->statistics.tx_good_frames);
> +    stl_le_pci_dma(&s->dev, s->statsaddr + 36,
> +                   s->statistics.rx_good_frames);
> +    stl_le_pci_dma(&s->dev, s->statsaddr + 48,
> +                   s->statistics.rx_resource_errors);
> +    stl_le_pci_dma(&s->dev, s->statsaddr + 60,
> +                   s->statistics.rx_short_frame_errors);

This might introduce a bug: stlXX APIs assume aligned addresses,
an address in statsaddr is user-controlled so I'm not sure
it's always aligned.

Why isn't the patch simply replacing cpu_physical_memory_read
with pci_XXX ? Any cleanups should be done separately.


>  #if 0
> -    e100_stw_le_phys(s->statsaddr + 76, s->statistics.xmt_tco_frames);
> -    e100_stw_le_phys(s->statsaddr + 78, s->statistics.rcv_tco_frames);
> +    stw_le_pci_dma(&s->dev, s->statsaddr + 76, s->statistics.xmt_tco_frames);
> +    stw_le_pci_dma(&s->dev, s->statsaddr + 78, s->statistics.rcv_tco_frames);
>      missing("CU dump statistical counters");
>  #endif
>  }
>  
>  static void read_cb(EEPRO100State *s)
>  {
> -    cpu_physical_memory_read(s->cb_address, &s->tx, sizeof(s->tx));
> +    pci_dma_read(&s->dev, s->cb_address, (uint8_t *) &s->tx, sizeof(s->tx));
>      s->tx.status = le16_to_cpu(s->tx.status);
>      s->tx.command = le16_to_cpu(s->tx.command);
>      s->tx.link = le32_to_cpu(s->tx.link);
> @@ -788,18 +762,17 @@ static void tx_command(EEPRO100State *s)
>      }
>      assert(tcb_bytes <= sizeof(buf));
>      while (size < tcb_bytes) {
> -        uint32_t tx_buffer_address = e100_ldl_le_phys(tbd_address);
> -        uint16_t tx_buffer_size = e100_ldw_le_phys(tbd_address + 4);
> +        uint32_t tx_buffer_address = ldl_le_pci_dma(&s->dev, tbd_address);
> +        uint16_t tx_buffer_size = lduw_le_pci_dma(&s->dev, tbd_address + 4);
>  #if 0
> -        uint16_t tx_buffer_el = e100_ldw_le_phys(tbd_address + 6);
> +        uint16_t tx_buffer_el = lduw_le_pci_dma(&s->dev, tbd_address + 6);
>  #endif
>          tbd_address += 8;
>          TRACE(RXTX, logout
>              ("TBD (simplified mode): buffer address 0x%08x, size 0x%04x\n",
>               tx_buffer_address, tx_buffer_size));
>          tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
> -        cpu_physical_memory_read(tx_buffer_address, &buf[size],
> -                                 tx_buffer_size);
> +        pci_dma_read(&s->dev, tx_buffer_address, &buf[size], tx_buffer_size);
>          size += tx_buffer_size;
>      }
>      if (tbd_array == 0xffffffff) {
> @@ -810,16 +783,19 @@ static void tx_command(EEPRO100State *s)
>          if (s->has_extended_tcb_support && !(s->configuration[6] & BIT(4))) {
>              /* Extended Flexible TCB. */
>              for (; tbd_count < 2; tbd_count++) {
> -                uint32_t tx_buffer_address = e100_ldl_le_phys(tbd_address);
> -                uint16_t tx_buffer_size = e100_ldw_le_phys(tbd_address + 4);
> -                uint16_t tx_buffer_el = e100_ldw_le_phys(tbd_address + 6);
> +                uint32_t tx_buffer_address = ldl_le_pci_dma(&s->dev,
> +                                                            tbd_address);
> +                uint16_t tx_buffer_size = lduw_le_pci_dma(&s->dev,
> +                                                          tbd_address + 4);
> +                uint16_t tx_buffer_el = lduw_le_pci_dma(&s->dev,
> +                                                        tbd_address + 6);
>                  tbd_address += 8;
>                  TRACE(RXTX, logout
>                      ("TBD (extended flexible mode): buffer address 0x%08x, size 0x%04x\n",
>                       tx_buffer_address, tx_buffer_size));
>                  tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
> -                cpu_physical_memory_read(tx_buffer_address, &buf[size],
> -                                         tx_buffer_size);
> +                pci_dma_read(&s->dev, tx_buffer_address,
> +                             &buf[size], tx_buffer_size);
>                  size += tx_buffer_size;
>                  if (tx_buffer_el & 1) {
>                      break;
> @@ -828,16 +804,16 @@ static void tx_command(EEPRO100State *s)
>          }
>          tbd_address = tbd_array;
>          for (; tbd_count < s->tx.tbd_count; tbd_count++) {
> -            uint32_t tx_buffer_address = e100_ldl_le_phys(tbd_address);
> -            uint16_t tx_buffer_size = e100_ldw_le_phys(tbd_address + 4);
> -            uint16_t tx_buffer_el = e100_ldw_le_phys(tbd_address + 6);
> +            uint32_t tx_buffer_address = ldl_le_pci_dma(&s->dev, tbd_address);
> +            uint16_t tx_buffer_size = lduw_le_pci_dma(&s->dev, tbd_address + 4);
> +            uint16_t tx_buffer_el = lduw_le_pci_dma(&s->dev, tbd_address + 6);
>              tbd_address += 8;
>              TRACE(RXTX, logout
>                  ("TBD (flexible mode): buffer address 0x%08x, size 0x%04x\n",
>                   tx_buffer_address, tx_buffer_size));
>              tx_buffer_size = MIN(tx_buffer_size, sizeof(buf) - size);
> -            cpu_physical_memory_read(tx_buffer_address, &buf[size],
> -                                     tx_buffer_size);
> +            pci_dma_read(&s->dev, tx_buffer_address,
> +                         &buf[size], tx_buffer_size);
>              size += tx_buffer_size;
>              if (tx_buffer_el & 1) {
>                  break;
> @@ -862,7 +838,7 @@ static void set_multicast_list(EEPRO100State *s)
>      TRACE(OTHER, logout("multicast list, multicast count = %u\n", multicast_count));
>      for (i = 0; i < multicast_count; i += 6) {
>          uint8_t multicast_addr[6];
> -        cpu_physical_memory_read(s->cb_address + 10 + i, multicast_addr, 6);
> +        pci_dma_read(&s->dev, s->cb_address + 10 + i, multicast_addr, 6);
>          TRACE(OTHER, logout("multicast entry %s\n", nic_dump(multicast_addr, 6)));
>          unsigned mcast_idx = compute_mcast_idx(multicast_addr);
>          assert(mcast_idx < 64);
> @@ -896,12 +872,12 @@ static void action_command(EEPRO100State *s)
>              /* Do nothing. */
>              break;
>          case CmdIASetup:
> -            cpu_physical_memory_read(s->cb_address + 8, &s->conf.macaddr.a[0], 6);
> +            pci_dma_read(&s->dev, s->cb_address + 8, &s->conf.macaddr.a[0], 6);
>              TRACE(OTHER, logout("macaddr: %s\n", nic_dump(&s->conf.macaddr.a[0], 6)));
>              break;
>          case CmdConfigure:
> -            cpu_physical_memory_read(s->cb_address + 8, &s->configuration[0],
> -                                     sizeof(s->configuration));
> +            pci_dma_read(&s->dev, s->cb_address + 8,
> +                         &s->configuration[0], sizeof(s->configuration));
>              TRACE(OTHER, logout("configuration: %s\n",
>                                  nic_dump(&s->configuration[0], 16)));
>              TRACE(OTHER, logout("configuration: %s\n",
> @@ -938,7 +914,8 @@ static void action_command(EEPRO100State *s)
>              break;
>          }
>          /* Write new status. */
> -        e100_stw_le_phys(s->cb_address, s->tx.status | ok_status | STATUS_C);
> +        stw_le_pci_dma(&s->dev, s->cb_address,
> +                       s->tx.status | ok_status | STATUS_C);
>          if (bit_i) {
>              /* CU completed action. */
>              eepro100_cx_interrupt(s);
> @@ -1005,7 +982,7 @@ static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
>          /* Dump statistical counters. */
>          TRACE(OTHER, logout("val=0x%02x (dump stats)\n", val));
>          dump_statistics(s);
> -        e100_stl_le_phys(s->statsaddr + s->stats_size, 0xa005);
> +        stl_le_pci_dma(&s->dev, s->statsaddr + s->stats_size, 0xa005);
>          break;
>      case CU_CMD_BASE:
>          /* Load CU base. */
> @@ -1016,7 +993,7 @@ static void eepro100_cu_command(EEPRO100State * s, uint8_t val)
>          /* Dump and reset statistical counters. */
>          TRACE(OTHER, logout("val=0x%02x (dump stats and reset)\n", val));
>          dump_statistics(s);
> -        e100_stl_le_phys(s->statsaddr + s->stats_size, 0xa007);
> +        stl_le_pci_dma(&s->dev, s->statsaddr + s->stats_size, 0xa007);
>          memset(&s->statistics, 0, sizeof(s->statistics));
>          break;
>      case CU_SRESUME:
> @@ -1310,10 +1287,10 @@ static void eepro100_write_port(EEPRO100State *s)
>      case PORT_SELFTEST:
>          TRACE(OTHER, logout("selftest address=0x%08x\n", address));
>          eepro100_selftest_t data;
> -        cpu_physical_memory_read(address, &data, sizeof(data));
> +        pci_dma_read(&s->dev, address, (uint8_t *) &data, sizeof(data));
>          data.st_sign = 0xffffffff;
>          data.st_result = 0;
> -        cpu_physical_memory_write(address, &data, sizeof(data));
> +        pci_dma_write(&s->dev, address, (uint8_t *) &data, sizeof(data));
>          break;
>      case PORT_SELECTIVE_RESET:
>          TRACE(OTHER, logout("selective reset, selftest address=0x%08x\n", address));
> @@ -1729,8 +1706,8 @@ static ssize_t nic_receive(VLANClientState *nc, const uint8_t * buf, size_t size
>      }
>      /* !!! */
>      eepro100_rx_t rx;
> -    cpu_physical_memory_read(s->ru_base + s->ru_offset, &rx,
> -                             sizeof(eepro100_rx_t));
> +    pci_dma_read(&s->dev, s->ru_base + s->ru_offset,
> +                 (uint8_t *) &rx, sizeof(eepro100_rx_t));
>      uint16_t rfd_command = le16_to_cpu(rx.command);
>      uint16_t rfd_size = le16_to_cpu(rx.size);
>  
> @@ -1746,10 +1723,10 @@ static ssize_t nic_receive(VLANClientState *nc, const uint8_t * buf, size_t size
>  #endif
>      TRACE(OTHER, logout("command 0x%04x, link 0x%08x, addr 0x%08x, size %u\n",
>            rfd_command, rx.link, rx.rx_buf_addr, rfd_size));
> -    e100_stw_le_phys(s->ru_base + s->ru_offset +
> -                     offsetof(eepro100_rx_t, status), rfd_status);
> -    e100_stw_le_phys(s->ru_base + s->ru_offset +
> -                     offsetof(eepro100_rx_t, count), size);
> +    stw_le_pci_dma(&s->dev, s->ru_base + s->ru_offset +
> +                offsetof(eepro100_rx_t, status), rfd_status);
> +    stw_le_pci_dma(&s->dev, s->ru_base + s->ru_offset +
> +                offsetof(eepro100_rx_t, count), size);
>      /* Early receive interrupt not supported. */
>  #if 0
>      eepro100_er_interrupt(s);
> @@ -1763,8 +1740,8 @@ static ssize_t nic_receive(VLANClientState *nc, const uint8_t * buf, size_t size
>  #if 0
>      assert(!(s->configuration[17] & BIT(0)));
>  #endif
> -    cpu_physical_memory_write(s->ru_base + s->ru_offset +
> -                              sizeof(eepro100_rx_t), buf, size);
> +    pci_dma_write(&s->dev, s->ru_base + s->ru_offset +
> +                  sizeof(eepro100_rx_t), buf, size);
>      s->statistics.rx_good_frames++;
>      eepro100_fr_interrupt(s);
>      s->ru_offset = le32_to_cpu(rx.link);
> -- 
> 1.7.7