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

[Alexander Graf]

Michael S. Tsirkin wrote:
> On Tue, Nov 01, 2011 at 03:24:28PM -0500, Anthony Liguori wrote:
>   
>> On 10/31/2011 11:45 AM, Stefan Weil wrote:
>>     
>>> Am 31.10.2011 07:06, schrieb David Gibson:
>>>       
>>>> From: Eduard - Gabriel Munteanu<address@hidden>
>>>>
>>>> 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<address@hidden>
>>>> Signed-off-by: David Gibson<address@hidden>
>>>> Signed-off-by: Alexey Kardashevskiy<address@hidden>
>>>> ---
>>>> 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.
>>     
>
> What does it get us, applying it before freeze?
> It's an api change without new functionality.
> Seems better on -next branch.
>   

It gets us that downstreams can convert to the API for 1.0. It just
feels a lot more right to change APIs for a 1.0 release than a 1.1 release.

Also, it gives it more exposure and if anyone is inclined to have a
small patch to add IOMMU support for a specific platform downstream,
they can do so.

Overall, I think it's a good idea to pull it in for 1.0. It certainly
makes David's life easier :). Since we also have PCI support in -M
pseries now we could even declare the non-usage of an IOMMU there as a
bug and fix it for 1.0.1. Without, no PCI device except for virtio works.


Alex