Re: [Qemu-devel] [PATCH 1/5] pci: introduce PC_PCI_CONFIG_ENABLED()

[Hu Tao <hutao@cn.fujitsu.com>]

On Tue, Nov 04, 2014 at 03:41:11PM +0200, Marcel Apfelbaum wrote:
> Hi,
> 
> On Tue, 2014-11-04 at 17:12 +0800, Hu Tao wrote:
> > This makes code more readable.
> > 
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  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).

Sounds reasonable. I changed it to an inline function and prefixed it
with pci_host_ to follow the convention in pci_host.c.

> 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?

Done.

Thanks!

> 
> Thanks,
> Marcel
> > +
> >  /* PCI bus */
> >  
> >  #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))
> 
> 
>