Message ID | 20091112120245.GA11106@redhat.com |
---|---|
State | New |
Headers | show |
On Thu, Nov 12, 2009 at 02:02:45PM +0200, Michael S. Tsirkin wrote: > 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); > > > > 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? Yes. Acked-by: Isaku Yamahata <yamahata@valinux.co.jp> > > ---> > > 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, >
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,