From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1N8YQX-0001Dm-F7 for qemu-devel@nongnu.org; Thu, 12 Nov 2009 07:05:25 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1N8YQS-0001As-8l for qemu-devel@nongnu.org; Thu, 12 Nov 2009 07:05:24 -0500 Received: from [199.232.76.173] (port=39069 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1N8YQS-0001An-3O for qemu-devel@nongnu.org; Thu, 12 Nov 2009 07:05:20 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45836) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1N8YQQ-0000fE-UM for qemu-devel@nongnu.org; Thu, 12 Nov 2009 07:05:19 -0500 Date: Thu, 12 Nov 2009 14:02:45 +0200 From: "Michael S. Tsirkin" Message-ID: <20091112120245.GA11106@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> <20091112111525.GB4815@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20091112111525.GB4815@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:15:25PM +0200, Michael S. Tsirkin wrote: > 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; val is now unused, so it can be killed. so I ended up with the below: ACK? ---> 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: 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..1dbc94e 100644 --- a/hw/pcie_host.c +++ b/hw/pcie_host.c @@ -65,31 +65,15 @@ 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); - uint32_t val; + PCIDevice *pci_dev = pcie_mmcfg_addr_to_dev(s, addr); + 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,