From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35959) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XleNH-0004YQ-3b for qemu-devel@nongnu.org; Tue, 04 Nov 2014 08:42:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XleNC-0002Z0-0S for qemu-devel@nongnu.org; Tue, 04 Nov 2014 08:42:19 -0500 Received: from mail-lb0-x22a.google.com ([2a00:1450:4010:c04::22a]:57619) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XleN9-0002Xd-CJ for qemu-devel@nongnu.org; Tue, 04 Nov 2014 08:42:13 -0500 Received: by mail-lb0-f170.google.com with SMTP id z12so2737114lbi.15 for ; Tue, 04 Nov 2014 05:42:08 -0800 (PST) Message-ID: <1415108471.2327.44.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Tue, 04 Nov 2014 15:41:11 +0200 In-Reply-To: <27c47334bab155b7219a6fcc4547701e2a6e2060.1415091929.git.hutao@cn.fujitsu.com> References: <27c47334bab155b7219a6fcc4547701e2a6e2060.1415091929.git.hutao@cn.fujitsu.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/5] pci: introduce PC_PCI_CONFIG_ENABLED() Reply-To: marcel.a@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hu Tao Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" Hi, On Tue, 2014-11-04 at 17:12 +0800, Hu Tao wrote: > This makes code more readable. > > Signed-off-by: Hu Tao > --- > hw/mips/gt64xxx_pci.c | 4 ++-- > hw/pci/pci_host.c | 5 +++-- > include/hw/pci/pci.h | 2 ++ > 3 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c > index 1f2fe5f..a49dbd7 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 (PC_PCI_CONFIG_ENABLED(phb->config_reg)) I really like readable MACROS instead of magic numbers, however I have 3 suggestions: 1. PC_PCI_CONFIG_ENABLED is still not really clear, because of the "PC" prefix that is too wide in my IMHO (maybe PCI_HOST_BRIDGE_CONFIG_ENABLED) when this macro refers only to host bridges. 2. Maybe go the "extra mile" and let the macro receive a host bridge as parameter PCI_HOST_BRIDGE_CONFIG_ENABLED(host_bridge). 3. Maybe make it an inline function? Just wondering > 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 (!PC_PCI_CONFIG_ENABLED(phb->config_reg)) { > 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..f2a69ea 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 (PC_PCI_CONFIG_ENABLED(s->config_reg)) { > 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 (!PC_PCI_CONFIG_ENABLED(s->config_reg)) { > return 0xffffffff; > } > val = pci_data_read(s->bus, s->config_reg | (addr & 3), len); > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index c352c7b..3d42d7f 100644 > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -13,6 +13,8 @@ > > #include "hw/pci/pcie.h" > > +#define PC_PCI_CONFIG_ENABLED(addr) (addr & (1U << 31)) Again, maybe move this to "hw/pci/pci_host" since is specific to host bridges? Thanks, Marcel > + > /* PCI bus */ > > #define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07))