From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:43831) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RLaD4-0003aP-0o for qemu-devel@nongnu.org; Wed, 02 Nov 2011 08:46:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RLaD0-0001FX-TQ for qemu-devel@nongnu.org; Wed, 02 Nov 2011 08:46:25 -0400 Received: from mail-gy0-f173.google.com ([209.85.160.173]:53960) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RLaD0-0001FR-MS for qemu-devel@nongnu.org; Wed, 02 Nov 2011 08:46:22 -0400 Received: by gya6 with SMTP id 6so98749gya.4 for ; Wed, 02 Nov 2011 05:46:21 -0700 (PDT) Message-ID: <4EB13B98.8040006@codemonkey.ws> Date: Wed, 02 Nov 2011 07:46:16 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1320041218-30487-1-git-send-email-david@gibson.dropbear.id.au> <1320041218-30487-6-git-send-email-david@gibson.dropbear.id.au> <4EAED0A6.4030808@weilnetz.de> <4EB0557C.70408@redhat.com> <20111102074020.GC5613@redhat.com> In-Reply-To: <20111102074020.GC5613@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 05/14] eepro100: Use PCI DMA stub functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Stefan Weil , eduard.munteanu@linux360.ro, agraf@suse.de, qemu-devel@nongnu.org, avi@redhat.com, joerg.roedel@amd.com, David Gibson , rth@twiddle.net 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 >>>> >>>> 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 >>>> Signed-off-by: David Gibson >>>> Signed-off-by: Alexey Kardashevskiy >>>> --- >>>> 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 >>> >>> >>> >