[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH 03/20] pci: simplify pci_data_read(), pcie_mmcfg
From: |
Isaku Yamahata |
Subject: |
[Qemu-devel] Re: [PATCH 03/20] pci: simplify pci_data_read(), pcie_mmcfg_data_read(). |
Date: |
Thu, 12 Nov 2009 21:14:53 +0900 |
User-agent: |
Mutt/1.5.6i |
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 <address@hidden>
> > >
> > > 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 <address@hidden>
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> >
> > ---
> >
> > 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 <address@hidden>
>
> --->
>
> 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 <address@hidden>
>
> ---
>
> 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,
>
--
yamahata
- [Qemu-devel] Re: [PATCH 02/20] pci: move pci_data_{read, write}() declaration from pci.h to pci_host.h, (continued)
[Qemu-devel] [PATCH 05/20] pci: rename pci_addr_to_dev(), pcie_mmcfg_addr_to_dev()., Isaku Yamahata, 2009/11/12
[Qemu-devel] [PATCH 11/20] pci: clean up of pci_init_wmask()., Isaku Yamahata, 2009/11/12
[Qemu-devel] [PATCH 07/20] pci: remove pci_sub_bus() by open coding., Isaku Yamahata, 2009/11/12
[Qemu-devel] [PATCH 18/20] pci: fix pci_config_get_io_base()., Isaku Yamahata, 2009/11/12
[Qemu-devel] [PATCH 16/20] pci: kill goto in pci_update_mappings(), Isaku Yamahata, 2009/11/12