[Qemu-devel] Re: [PATCH 03/20] pci: simplify pci_data_read(), pcie_mmcfg_data_read().

["Michael S. Tsirkin" <mst@redhat.com>]

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 <yamahata@valinux.co.jp>
> > 
> > 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 <yamahata@valinux.co.jp>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> ---
> 
> 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 <mst@redhat.com>

---

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,