From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1N8Xgi-00007n-RN for qemu-devel@nongnu.org; Thu, 12 Nov 2009 06:18:04 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1N8Xgd-0008Vk-98 for qemu-devel@nongnu.org; Thu, 12 Nov 2009 06:18:03 -0500 Received: from [199.232.76.173] (port=40548 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1N8Xgc-0008Vb-Rz for qemu-devel@nongnu.org; Thu, 12 Nov 2009 06:17:59 -0500 Received: from mx1.redhat.com ([209.132.183.28]:24663) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1N8Xgc-0001GD-CA for qemu-devel@nongnu.org; Thu, 12 Nov 2009 06:17:58 -0500 Date: Thu, 12 Nov 2009 13:15:25 +0200 From: "Michael S. Tsirkin" Message-ID: <20091112111525.GB4815@redhat.com> References: <1258005528-25383-1-git-send-email-yamahata@valinux.co.jp> <1258005528-25383-4-git-send-email-yamahata@valinux.co.jp> <20091112110109.GA4769@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20091112110109.GA4769@redhat.com> Subject: [Qemu-devel] Re: [PATCH 03/20] pci: simplify pci_data_read(), pcie_mmcfg_data_read(). List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: qemu-devel@nongnu.org On Thu, Nov 12, 2009 at 01:01:09PM +0200, Michael S. Tsirkin wrote: > On Thu, Nov 12, 2009 at 02:58:31PM +0900, Isaku Yamahata wrote: > > simplify ugly switch by memcpy trick. > > And add one assert(). > > > > Signed-off-by: Isaku Yamahata > > In fact, there's no reason to be so careful about > zeroing out high bits as far as I can tell: > in the end, the value is assigned to uint32/uint16/uint8 > as appropriate and high bits are truncated. > So this can simply be val = 0xffffffff; > > > --- > > hw/pci_host.c | 16 ++++------------ > > hw/pcie_host.c | 16 ++++------------ > > 2 files changed, 8 insertions(+), 24 deletions(-) > > > > diff --git a/hw/pci_host.c b/hw/pci_host.c > > index f4518dc..4196ebc 100644 > > --- a/hw/pci_host.c > > +++ b/hw/pci_host.c > > @@ -71,19 +71,11 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len) > > uint32_t config_addr = pci_addr_to_config(addr); > > uint32_t val; > > > > + assert(len == 1 || len == 2 || len == 4); > > if (!pci_dev) { > > - switch(len) { > > - case 1: > > - val = 0xff; > > - break; > > - case 2: > > - val = 0xffff; > > - break; > > - default: > > - case 4: > > - val = 0xffffffff; > > - break; > > - } > > + val = 0; > > + memset(&val, 0xff, len); > > + val = le32_to_cpu(val); > > > So the above will become simply > if (!pci_dev) { > return ~0x0; > } > > > > } else { > > val = pci_dev->config_read(pci_dev, config_addr, len); > > PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n", > > diff --git a/hw/pcie_host.c b/hw/pcie_host.c > > index b52fec6..61285da 100644 > > --- a/hw/pcie_host.c > > +++ b/hw/pcie_host.c > > @@ -71,19 +71,11 @@ static uint32_t pcie_mmcfg_data_read(PCIBus *s, > > PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, mmcfg_addr); > > uint32_t val; > > > > + assert(len == 1 || len == 2 || len == 4); > > if (!pci_dev) { > > - switch(len) { > > - case 1: > > - val = 0xff; > > - break; > > - case 2: > > - val = 0xffff; > > - break; > > - default: > > - case 4: > > - val = 0xffffffff; > > - break; > > - } > > + val = 0; > > + memset(&val, 0xff, len); > > + val = le32_to_cpu(val); > > } else { > > val = pci_dev->config_read(pci_dev, > > PCIE_MMCFG_CONFOFFSET(mmcfg_addr), len); > > -- > > 1.6.0.2 I put this on my tree instead: ---> pci: simplify (pci_/pcie_mmcfg_)data_read() Remove switch on length: we don't care about high bits for value, so just return all ones if no device. And add one assert(). Signed-off-by: Isaku Yamahata Signed-off-by: Michael S. Tsirkin --- diff --git a/hw/pci_host.c b/hw/pci_host.c index f4518dc..4a29f44 100644 --- a/hw/pci_host.c +++ b/hw/pci_host.c @@ -71,25 +71,15 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len) uint32_t config_addr = pci_addr_to_config(addr); uint32_t val; + assert(len == 1 || len == 2 || len == 4); if (!pci_dev) { - switch(len) { - case 1: - val = 0xff; - break; - case 2: - val = 0xffff; - break; - default: - case 4: - val = 0xffffffff; - break; - } - } else { - val = pci_dev->config_read(pci_dev, config_addr, len); - PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n", - __func__, pci_dev->name, config_addr, val, len); + return ~0x0; } + val = pci_dev->config_read(pci_dev, config_addr, len); + PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n", + __func__, pci_dev->name, config_addr, val, len); + return val; } diff --git a/hw/pcie_host.c b/hw/pcie_host.c index b52fec6..9a5f135 100644 --- a/hw/pcie_host.c +++ b/hw/pcie_host.c @@ -65,31 +65,16 @@ static void pcie_mmcfg_data_write(PCIBus *s, PCIE_MMCFG_CONFOFFSET(mmcfg_addr), val, len); } -static uint32_t pcie_mmcfg_data_read(PCIBus *s, - uint32_t mmcfg_addr, int len) +static uint32_t pcie_mmcfg_data_read(PCIBus *s, uint32_t addr, int len) { - PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, mmcfg_addr); + PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, addr); uint32_t val; + assert(len == 1 || len == 2 || len == 4); if (!pci_dev) { - switch(len) { - case 1: - val = 0xff; - break; - case 2: - val = 0xffff; - break; - default: - case 4: - val = 0xffffffff; - break; - } - } else { - val = pci_dev->config_read(pci_dev, - PCIE_MMCFG_CONFOFFSET(mmcfg_addr), len); + return ~0x0; } - - return val; + return pci_dev->config_read(pci_dev, PCIE_MMCFG_CONFOFFSET(addr), len); } static void pcie_mmcfg_data_writeb(void *opaque,