From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53008) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YDtoR-0000fd-I7 for qemu-devel@nongnu.org; Wed, 21 Jan 2015 06:51:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YDtoO-0006ni-AP for qemu-devel@nongnu.org; Wed, 21 Jan 2015 06:51:07 -0500 Received: from mx1.redhat.com ([209.132.183.28]:40892) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YDtoO-0006nW-2j for qemu-devel@nongnu.org; Wed, 21 Jan 2015 06:51:04 -0500 Date: Wed, 21 Jan 2015 13:50:59 +0200 From: "Michael S. Tsirkin" Message-ID: <20150121115059.GB15439@redhat.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v3 2/6] pci: introduce pci_host_config_enabled() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hu Tao Cc: qemu-devel@nongnu.org, Marcel Apfelbaum On Thu, Dec 11, 2014 at 10:20:24AM +0800, Hu Tao wrote: > This makes code more readable. > > Signed-off-by: Hu Tao > Reviewed-by: Marcel Apfelbaum > --- > hw/mips/gt64xxx_pci.c | 4 ++-- > hw/pci/pci_host.c | 5 +++-- > include/hw/pci/pci_host.h | 5 +++++ > 3 files changed, 10 insertions(+), 4 deletions(-) We have a ton of other places hard-coding 1<<31, why special-case these? > diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c > index 1f2fe5f..f118c9c 100644 > --- a/hw/mips/gt64xxx_pci.c > +++ b/hw/mips/gt64xxx_pci.c > @@ -564,7 +564,7 @@ static void gt64120_writel (void *opaque, hwaddr addr, > if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) { > val = bswap32(val); > } > - if (phb->config_reg & (1u << 31)) { > + if (pci_host_config_enabled(phb)) { > pci_data_write(phb->bus, phb->config_reg, val, 4); > } > break; > @@ -804,7 +804,7 @@ static uint64_t gt64120_readl (void *opaque, > val = phb->config_reg; > break; > case GT_PCI0_CFGDATA: > - if (!(phb->config_reg & (1 << 31))) { > + if (!pci_host_config_enabled(phb)) { > val = 0xffffffff; > } else { > val = pci_data_read(phb->bus, phb->config_reg, 4); > diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c > index 3e26f92..9bc47d8 100644 > --- a/hw/pci/pci_host.c > +++ b/hw/pci/pci_host.c > @@ -133,8 +133,9 @@ static void pci_host_data_write(void *opaque, hwaddr addr, > PCIHostState *s = opaque; > PCI_DPRINTF("write addr " TARGET_FMT_plx " len %d val %x\n", > addr, len, (unsigned)val); > - if (s->config_reg & (1u << 31)) > + if (pci_host_config_enabled(s)) { > pci_data_write(s->bus, s->config_reg | (addr & 3), val, len); > + } > } > > static uint64_t pci_host_data_read(void *opaque, > @@ -142,7 +143,7 @@ static uint64_t pci_host_data_read(void *opaque, > { > PCIHostState *s = opaque; > uint32_t val; > - if (!(s->config_reg & (1U << 31))) { > + if (!pci_host_config_enabled(s)) { > return 0xffffffff; > } > val = pci_data_read(s->bus, s->config_reg | (addr & 3), len); > diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h > index ba31595..b48791d 100644 > --- a/include/hw/pci/pci_host.h > +++ b/include/hw/pci/pci_host.h > @@ -65,6 +65,11 @@ uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr, > void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len); > uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len); > > +static inline bool pci_host_config_enabled(struct PCIHostState *pci_host) > +{ > + return pci_host->config_reg & (1U << 31); > +} > + Better: #define PCI_HOST_CONFIG_ENABLE (1U << 31) then everyone can just do s->config_reg & PCI_HOST_CONFIG_ENABLE better as it'll work for code like this: phb->config_reg = (pciaddr) | (1u << 31); > extern const MemoryRegionOps pci_host_conf_le_ops; > extern const MemoryRegionOps pci_host_conf_be_ops; > extern const MemoryRegionOps pci_host_data_le_ops; > -- > 1.9.3