[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] pci: Length-align config space accesses
From: |
Isaku Yamahata |
Subject: |
Re: [Qemu-devel] [PATCH] pci: Length-align config space accesses |
Date: |
Wed, 20 Jul 2011 21:00:27 +0900 |
User-agent: |
Mutt/1.5.19 (2009-01-05) |
Hi. This clean up looks good basically.
But when conventional pci device is accessed via MMCONFIG area,
addr &= addr_mask doesn't work as expected.
The config area of [256, 4K) of conventional pci should have no effect.
thanks,
On Tue, Jul 19, 2011 at 11:39:02PM +0200, Jan Kiszka wrote:
> From: Jan Kiszka <address@hidden>
>
> Introduce pci_config_read/write helpers to split up config space
> accesses that are not length-aligned. This particularly avoids that each
> and every device needs to check for config space overruns. Also move the
> access length assertion to the new helpers.
>
> Signed-off-by: Jan Kiszka <address@hidden>
> ---
>
> This will also simplify my cap refactorings for the device-assignment
> code in qemu-kvm.
>
> hw/pci.c | 43 +++++++++++++++++++++++++++++++++++++++----
> hw/pci.h | 5 +++++
> hw/pci_host.c | 14 ++++++--------
> hw/pcie_host.c | 12 ++++++------
> 4 files changed, 56 insertions(+), 18 deletions(-)
>
> diff --git a/hw/pci.c b/hw/pci.c
> index b904a4e..6957ece 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1104,12 +1104,48 @@ static void pci_update_irq_disabled(PCIDevice *d, int
> was_irq_disabled)
> }
> }
>
> +void pci_config_write(PCIDevice *pci_dev, uint32_t addr, uint32_t addr_mask,
> + uint32_t val, uint32_t len)
> +{
> + assert(len == 1 || len == 2 || len == 4);
> +
> + addr &= addr_mask;
> +
> + if (addr & (len - 1)) {
> + len >>= 1;
> + pci_config_write(pci_dev, addr, addr_mask, val, len);
> + pci_config_write(pci_dev, addr + len, addr_mask,
> + val >> (len * 8), len);
> + } else {
> + pci_dev->config_write(pci_dev, addr, val, len);
> + }
> +}
> +
> +uint32_t pci_config_read(PCIDevice *pci_dev, uint32_t addr, uint32_t
> addr_mask,
> + uint32_t len)
> +{
> + uint32_t val;
> +
> + assert(len == 1 || len == 2 || len == 4);
> +
> + addr &= addr_mask;
> +
> + if (addr & (len - 1)) {
> + len >>= 1;
> + val = pci_config_read(pci_dev, addr, addr_mask, len);
> + val |= pci_config_read(pci_dev, addr + len,
> + addr_mask, len) << (len * 8);
> + } else {
> + val = pci_dev->config_read(pci_dev, addr, len);
> + }
> + return val;
> +}
> +
> uint32_t pci_default_read_config(PCIDevice *d,
> uint32_t address, int len)
> {
> uint32_t val = 0;
> - assert(len == 1 || len == 2 || len == 4);
> - len = MIN(len, pci_config_size(d) - address);
> +
> memcpy(&val, d->config + address, len);
> return le32_to_cpu(val);
> }
> @@ -1117,9 +1153,8 @@ uint32_t pci_default_read_config(PCIDevice *d,
> void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int
> l)
> {
> int i, was_irq_disabled = pci_irq_disabled(d);
> - uint32_t config_size = pci_config_size(d);
>
> - for (i = 0; i < l && addr + i < config_size; val >>= 8, ++i) {
> + for (i = 0; i < l; val >>= 8, ++i) {
> uint8_t wmask = d->wmask[addr + i];
> uint8_t w1cmask = d->w1cmask[addr + i];
> assert(!(wmask & w1cmask));
> diff --git a/hw/pci.h b/hw/pci.h
> index c220745..3d5b39c 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -482,4 +482,9 @@ static inline uint32_t pci_config_size(const PCIDevice *d)
> return pci_is_express(d) ? PCIE_CONFIG_SPACE_SIZE :
> PCI_CONFIG_SPACE_SIZE;
> }
>
> +void pci_config_write(PCIDevice *pci_dev, uint32_t addr, uint32_t addr_mask,
> + uint32_t val, uint32_t len);
> +uint32_t pci_config_read(PCIDevice *pci_dev, uint32_t addr, uint32_t
> addr_mask,
> + uint32_t len);
> +
> #endif
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index 728e2d4..db888fb 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -50,30 +50,28 @@ static inline PCIDevice *pci_dev_find_by_addr(PCIBus
> *bus, uint32_t addr)
> void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
> {
> PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
> - uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>
> - if (!pci_dev)
> + if (!pci_dev) {
> return;
> + }
>
> PCI_DPRINTF("%s: %s: addr=%02" PRIx32 " val=%08" PRIx32 " len=%d\n",
> - __func__, pci_dev->name, config_addr, val, len);
> - pci_dev->config_write(pci_dev, config_addr, val, len);
> + __func__, pci_dev->name, addr, val, len);
> + pci_config_write(pci_dev, addr, PCI_CONFIG_SPACE_SIZE - 1, val, len);
> }
>
> uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
> {
> PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
> - uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
> uint32_t val;
>
> - assert(len == 1 || len == 2 || len == 4);
> if (!pci_dev) {
> return ~0x0;
> }
>
> - val = pci_dev->config_read(pci_dev, config_addr, len);
> + val = pci_config_read(pci_dev, addr, PCI_CONFIG_SPACE_SIZE - 1, len);
> PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
> - __func__, pci_dev->name, config_addr, val, len);
> + __func__, pci_dev->name, addr, val, len);
>
> return val;
> }
> diff --git a/hw/pcie_host.c b/hw/pcie_host.c
> index b749865..e10d7a3 100644
> --- a/hw/pcie_host.c
> +++ b/hw/pcie_host.c
> @@ -57,22 +57,22 @@ static void pcie_mmcfg_data_write(PCIBus *s,
> {
> PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, mmcfg_addr);
>
> - if (!pci_dev)
> + if (!pci_dev) {
> return;
> -
> - pci_dev->config_write(pci_dev,
> - PCIE_MMCFG_CONFOFFSET(mmcfg_addr), val, len);
> + }
> + pci_config_write(pci_dev, PCIE_MMCFG_CONFOFFSET(mmcfg_addr),
> + PCIE_MMCFG_CONFOFFSET_MASK, val, len);
> }
>
> static uint32_t pcie_mmcfg_data_read(PCIBus *s, uint32_t addr, int len)
> {
> PCIDevice *pci_dev = pcie_dev_find_by_mmcfg_addr(s, addr);
>
> - assert(len == 1 || len == 2 || len == 4);
> if (!pci_dev) {
> return ~0x0;
> }
> - return pci_dev->config_read(pci_dev, PCIE_MMCFG_CONFOFFSET(addr), len);
> + return pci_config_read(pci_dev, PCIE_MMCFG_CONFOFFSET(addr),
> + PCIE_MMCFG_CONFOFFSET_MASK, len);
> }
>
> static void pcie_mmcfg_data_writeb(void *opaque,
> --
> 1.7.3.4
>
--
yamahata
- [Qemu-devel] [PATCH] pci: Length-align config space accesses, Jan Kiszka, 2011/07/19
- Re: [Qemu-devel] [PATCH] pci: Length-align config space accesses,
Isaku Yamahata <=
- Re: [Qemu-devel] [PATCH] pci: Length-align config space accesses, Jan Kiszka, 2011/07/20
- Re: [Qemu-devel] [PATCH] pci: Length-align config space accesses, Jan Kiszka, 2011/07/20
- Re: [Qemu-devel] [PATCH] pci: Length-align config space accesses, Isaku Yamahata, 2011/07/20
- Re: [Qemu-devel] [PATCH] pci: Length-align config space accesses, Michael S. Tsirkin, 2011/07/20
- Re: [Qemu-devel] [PATCH] pci: Length-align config space accesses, Jan Kiszka, 2011/07/20
- Re: [Qemu-devel] [PATCH] pci: Length-align config space accesses, Isaku Yamahata, 2011/07/20
- Re: [Qemu-devel] [PATCH] pci: Length-align config space accesses, Michael S. Tsirkin, 2011/07/20
- Re: [Qemu-devel] [PATCH] pci: Length-align config space accesses, Jan Kiszka, 2011/07/20
- Re: [Qemu-devel] [PATCH] pci: Length-align config space accesses, Michael S. Tsirkin, 2011/07/20
Re: [Qemu-devel] [PATCH] pci: Length-align config space accesses, Michael S. Tsirkin, 2011/07/20