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<eduard.munte...@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.munte...@linux360.ro> > >>Signed-off-by: David Gibson<d...@au1.ibm.com> > >>Signed-off-by: Alexey Kardashevskiy<a...@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.
What does it get us, applying it before freeze? It's an api change without new functionality. Seems better on -next branch. > But David, please follow up with a patch to > remove the unnecessary type casts. > > Regards, > > Anthony Liguori > > > > >Kind regards, > >Stefan > > > > > >