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

[Anthony Liguori <anthony@codemonkey.ws>]

On 10/31/2011 11:45 AM, Stefan Weil wrote:
> Am 31.10.2011 07:06, schrieb David Gibson:
>> 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);
> The type cast is not needed and should be removed.
>> + 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);
>> #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));
> The type cast is not needed and should be removed.
>> 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));
> The type cast is not needed and should be removed.
>> 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));
> The type cast is not needed and should be removed.
>> 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));
>
> The type cast is not needed and should be removed.
>
>
>> 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);
>
>
> Hi,
>
> the patch looks reasonable. I only suggest a formal change:
>
> There are lots of unnecessary type casts in several of your patches.
> I marked them here, but they should be removed anywhere.

Agreed.  However, I'm going to apply this series as I'd like to get it in for 
the freeze.  But David, please follow up with a patch to remove the unnecessary 
type casts.

Regards,

Anthony Liguori

>
> Kind regards,
> Stefan
>
>
>