| Message ID | 1320041218-30487-6-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
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. Kind regards, Stefan
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 > > >
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
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.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. 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 > > > > > >
On Tue, Nov 01, 2011 at 03:24:28PM -0500, Anthony Liguori wrote: > >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 Further, eepro100 now needs to be fixed to use pci_dma_write/read instead of stXXX/ldXXX, as address passed in apparently can be unaligned.
On 11/02/2011 02:40 AM, 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<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. > > What does it get us, applying it before freeze? > It's an api change without new functionality. > Seems better on -next branch. It was posted before soft freeze, had minimal feedback, and touches a lot of files (is not pleasant to rebase). Since it's just an internal change, it seems low risk to me. We need a DMA API. There's a lot of work moving forward to convert devices to use a DMA API. I'd rather start with an imperfect API and get the ball rolling on conversions. Regards, Anthony Liguori > >> But David, please follow up with a patch to >> remove the unnecessary type casts. >> >> Regards, >> >> Anthony Liguori >> >>> >>> Kind regards, >>> Stefan >>> >>> >>> >
On 11/02/2011 12:56 PM, Alexander Graf wrote: > Michael S. Tsirkin wrote: >> On Tue, Nov 01, 2011 at 03:24:28PM -0500, Anthony Liguori wrote: >> >> 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. I was in violent agreement up until this last sentence ;-) Having new bidirectional memory APIs is a really big positive infrastructure change for us. It's been something we've needed for a long time. Regards, Anthony Liguori
Anthony Liguori wrote: > On 11/02/2011 12:56 PM, Alexander Graf wrote: >> Michael S. Tsirkin wrote: >>> On Tue, Nov 01, 2011 at 03:24:28PM -0500, Anthony Liguori wrote: >>> >>> 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. > > I was in violent agreement up until this last sentence ;-) Heh :). Well, I'm just saying that if the patch ends up being trivial and really really useful and necessary for users, we at least have the chance now. Alex
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<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. >> > > 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
On Wed, Nov 02, 2011 at 06:56:55PM +0100, Alexander Graf wrote: > 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. Anyway, I hope at least what looks like the alignment bug in eepro100 introduced by this patch gets fixed before 1.0.
On Wed, Nov 02, 2011 at 09:11:57PM +0200, Michael S. Tsirkin wrote: > On Wed, Nov 02, 2011 at 06:56:55PM +0100, Alexander Graf wrote: > > 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. > > Anyway, I hope at least what looks like the alignment bug > in eepro100 introduced by this patch gets fixed before 1.0. Yeah, yeah, I'm getting to it. I've been slowed down by illness and have had higher priority bugs to work on first.
On Wed, Nov 02, 2011 at 09:16:34AM +0200, Michael S. Tsirkin wrote: > On Mon, Oct 31, 2011 at 05:06:49PM +1100, David Gibson wrote: > > From: Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro> [snip] > > @@ -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. Because it seemed like a good idea at the time. When I first wrote this, the possibility of unaligned addresses wasn't obvious to me. So, I'm working on fixing this now. I can take one of two approaches: - Simply revert this part of the change, reinstate the e100_stl functions as calling into dma_write(). - Alter the stX_dma() functions to work for unaligned addresses (by falling back to dma_rw() in that case). This is a little more involved but might make device writing safer in future. Anthony, Michael, any preferred direction here?
On Thu, Nov 03, 2011 at 04:16:34PM +1100, David Gibson wrote: > On Wed, Nov 02, 2011 at 09:16:34AM +0200, Michael S. Tsirkin wrote: > > On Mon, Oct 31, 2011 at 05:06:49PM +1100, David Gibson wrote: > > > From: Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro> > [snip] > > > @@ -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. > > Because it seemed like a good idea at the time. When I first wrote > this, the possibility of unaligned addresses wasn't obvious to me. > So, I'm working on fixing this now. I can take one of two approaches: > > - Simply revert this part of the change, reinstate the e100_stl > functions as calling into dma_write(). > > - Alter the stX_dma() functions to work for unaligned addresses (by > falling back to dma_rw() in that case). This is a little more > involved but might make device writing safer in future. Yes but then we lose the atomicity guarantee. So this might still result in subtle emulation bugs. > Anthony, Michael, any preferred direction here? For 1.0 I'd go for option 1 as the simplest. > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson
On Thu, Nov 03, 2011 at 02:25:45PM +0200, Michael S. Tsirkin wrote: > On Thu, Nov 03, 2011 at 04:16:34PM +1100, David Gibson wrote: > > On Wed, Nov 02, 2011 at 09:16:34AM +0200, Michael S. Tsirkin wrote: > > > On Mon, Oct 31, 2011 at 05:06:49PM +1100, David Gibson wrote: > > > > From: Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro> > > [snip] > > > > @@ -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. > > > > Because it seemed like a good idea at the time. When I first wrote > > this, the possibility of unaligned addresses wasn't obvious to me. > > So, I'm working on fixing this now. I can take one of two approaches: > > > > - Simply revert this part of the change, reinstate the e100_stl > > functions as calling into dma_write(). > > > > - Alter the stX_dma() functions to work for unaligned addresses (by > > falling back to dma_rw() in that case). This is a little more > > involved but might make device writing safer in future. > > Yes but then we lose the atomicity guarantee. So this might > still result in subtle emulation bugs. Sorry, I should have been clearer - I was planning to fall back to dma_rw() *only* for unaligned addresses. Aligned addresses would still have the atomicity guarantee. > > Anthony, Michael, any preferred direction here? > > For 1.0 I'd go for option 1 as the simplest.
On Wed, Nov 02, 2011 at 09:16:34AM +0200, Michael S. Tsirkin wrote: > On Mon, Oct 31, 2011 at 05:06:49PM +1100, David Gibson wrote: >> From: Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro> [snip] >> @@ -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. Hello, I just sent a patch for eepro100.c which enforces aligned addresses for s->statsaddr, so most of David's changes are now safe. Please include the patch in QEMU 1.0. Thanks, Stefan Weil
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); #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);