pci: Length-align config space accesses

PATCH In review Jan Kiszka 4 participants 14 messages Jul 19, 2011 – Jul 20, 2011

lore

Share & tools

lore

Jan Kiszka

author

2011-07-19 21:39 UTC #0 lore raw

PATCH 1/1 pci: Length-align config space accesses

From: Jan Kiszka <[email protected]>

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 <[email protected]>

---

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(-)

hw/pci.c +39 -4

index b904a4e..6957ece 100644
@@ -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));

hw/pci.h +5 -0

index c220745..3d5b39c 100644
@@ -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

hw/pci_host.c +6 -8

index 728e2d4..db888fb 100644
@@ -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;
 }

hw/pcie_host.c +6 -6

index b749865..e10d7a3 100644
@@ -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,

Signature · 1 line

1.7.3.4

Isaku Yamahata

2011-07-20 12:00 UTC #1 lore raw

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,

Quoted 176 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

Signed-off-by Jan Kiszka <[email protected]>

---

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(-)

hw/pci.c +39 -4

index b904a4e..6957ece 100644
@@ -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));

hw/pci.h +5 -0

index c220745..3d5b39c 100644
@@ -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

hw/pci_host.c +6 -8

index 728e2d4..db888fb 100644
@@ -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;
 }

hw/pcie_host.c +6 -6

index b749865..e10d7a3 100644
@@ -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,

Signature · 1 line

1.7.3.4

Signature · 1 line

yamahata

Jan Kiszka

2011-07-20 12:15 UTC #2 lore raw

Quoted 1 line · Isaku Yamahata

Hi. This clean up looks good basically.

Oops, forgot to cc you. Sorry.

Quoted 3 lines

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.

Mmh, I see. Looks like we need to split accesses at this boundary and executed them separately.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Jan Kiszka

2011-07-20 14:27 UTC #3 lore raw

Quoted 11 lines · Jan Kiszka

Quoted 1 line · Isaku Yamahata
Hi. This clean up looks good basically.
Oops, forgot to cc you. Sorry.
Quoted 3 lines
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.
Mmh, I see. Looks like we need to split accesses at this boundary and executed them separately.

Nope, no such issue: we already automatically split up accesses that span the legacy/extended boundary. Just like so far, legacy config space handlers have to filter out requests that address regions >= 256.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Isaku Yamahata

2011-07-20 16:17 UTC #4 lore raw

Quoted 10 lines · Jan Kiszka

Quoted 9 lines · Jan Kiszka
Quoted 1 line · Isaku Yamahata
Hi. This clean up looks good basically.
Oops, forgot to cc you. Sorry.
Quoted 3 lines
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.

Quoted 6 lines

Quoted 2 lines
Mmh, I see. Looks like we need to split accesses at this boundary and executed them separately.
Nope, no such issue: we already automatically split up accesses that span the legacy/extended boundary. Just like so far, legacy config space handlers have to filter out requests that address regions >= 256.

For example, when accessing to offset 257 of conventional pci device, the access is routed to offset 1 due to the masking.
Such overwrapping isn't correct.

Signature · 1 line

yamahata

Michael S. Tsirkin

2011-07-20 16:18 UTC #5 lore raw

Quoted 14 lines · Isaku Yamahata

Quoted 13 lines · Jan Kiszka
Quoted 11 lines · Jan Kiszka
Quoted 1 line · Isaku Yamahata
Hi. This clean up looks good basically.
Oops, forgot to cc you. Sorry.
Quoted 3 lines
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.
Mmh, I see. Looks like we need to split accesses at this boundary and executed them separately.

Quoted 7 lines

Quoted 3 lines
Nope, no such issue: we already automatically split up accesses that span the legacy/extended boundary. Just like so far, legacy config space handlers have to filter out requests that address regions >= 256.
For example, when accessing to offset 257 of conventional pci device, the access is routed to offset 1 due to the masking.
Such overwrapping isn't correct.

Can guest trigger this on some systems? I think it can't on a pc, right?

Quoted 2 lines

Signature · 1 line
yamahata

Jan Kiszka

2011-07-20 16:18 UTC #6 lore raw

In reply to Isaku Yamahata

Quoted 14 lines · Isaku Yamahata

Quoted 13 lines · Jan Kiszka
Quoted 11 lines · Jan Kiszka
Quoted 1 line · Isaku Yamahata
Hi. This clean up looks good basically.
Oops, forgot to cc you. Sorry.
Quoted 3 lines
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.
Mmh, I see. Looks like we need to split accesses at this boundary and executed them separately.

Quoted 7 lines

Quoted 3 lines
Nope, no such issue: we already automatically split up accesses that span the legacy/extended boundary. Just like so far, legacy config space handlers have to filter out requests that address regions >= 256.
For example, when accessing to offset 257 of conventional pci device, the access is routed to offset 1 due to the masking.
Such overwrapping isn't correct.

No, it isn't routed like that. The mask used via mmio is 0xfff.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Isaku Yamahata

2011-07-20 16:33 UTC #7 lore raw

Quoted 19 lines · Jan Kiszka

Quoted 18 lines · Isaku Yamahata
Quoted 16 lines · Jan Kiszka
Quoted 11 lines · Jan Kiszka
Quoted 1 line · Isaku Yamahata
Hi. This clean up looks good basically.
Oops, forgot to cc you. Sorry.
Quoted 3 lines
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.
Mmh, I see. Looks like we need to split accesses at this boundary and executed them separately.
Nope, no such issue: we already automatically split up accesses that span the legacy/extended boundary. Just like so far, legacy config space handlers have to filter out requests that address regions >= 256.

Quoted 5 lines

Quoted 3 lines
For example, when accessing to offset 257 of conventional pci device, the access is routed to offset 1 due to the masking.
Such overwrapping isn't correct.
No, it isn't routed like that. The mask used via mmio is 0xfff.

Ah sorry, you're right.
So pci_default_{read, write}_config() need to check it.

Signature · 1 line

yamahata

Michael S. Tsirkin

2011-07-20 16:44 UTC #8 lore raw

In reply to Jan Kiszka

Quoted 19 lines · Jan Kiszka

Quoted 18 lines · Isaku Yamahata
Quoted 16 lines · Jan Kiszka
Quoted 11 lines · Jan Kiszka
Quoted 1 line · Isaku Yamahata
Hi. This clean up looks good basically.
Oops, forgot to cc you. Sorry.
Quoted 3 lines
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.
Mmh, I see. Looks like we need to split accesses at this boundary and executed them separately.
Nope, no such issue: we already automatically split up accesses that span the legacy/extended boundary. Just like so far, legacy config space handlers have to filter out requests that address regions >= 256.

Quoted 7 lines

Quoted 3 lines
For example, when accessing to offset 257 of conventional pci device, the access is routed to offset 1 due to the masking.
Such overwrapping isn't correct.
No, it isn't routed like that. The mask used via mmio is 0xfff.
Jan

I thought about this some more, I'd like to see how devices are going to benefit. Any examples?
If not, is it easier to simply make this logic part of dev assignment?

Quoted 3 lines

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Jan Kiszka

2011-07-20 17:11 UTC #9 lore raw

Quoted 24 lines · Michael S. Tsirkin

Quoted 23 lines · Jan Kiszka
Quoted 21 lines · Isaku Yamahata
Quoted 10 lines · Jan Kiszka
Quoted 9 lines · Jan Kiszka
Quoted 1 line · Isaku Yamahata
Hi. This clean up looks good basically.
Oops, forgot to cc you. Sorry.
Quoted 3 lines
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.
Quoted 6 lines
Quoted 2 lines
Mmh, I see. Looks like we need to split accesses at this boundary and executed them separately.
Nope, no such issue: we already automatically split up accesses that span the legacy/extended boundary. Just like so far, legacy config space handlers have to filter out requests that address regions >= 256.
For example, when accessing to offset 257 of conventional pci device, the access is routed to offset 1 due to the masking.
Such overwrapping isn't correct.

Quoted 6 lines

Quoted 3 lines
No, it isn't routed like that. The mask used via mmio is 0xfff.
Jan
I thought about this some more, I'd like to see how devices are going to benefit. Any examples?

No in-tree device currently gets beyond the "write default, check range" pattern.

Quoted 2 lines

If not, is it easier to simply make this logic
part of dev assignment?

That's a question of clean interfaces. Not checking at the core means exposing invalid accesses to the callbacks.

The logic is required anyway, so better put it in a central place. E.g. if the Xen people push their device assignment as well, we will suddenly have more than one user.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Michael S. Tsirkin

2011-07-20 20:16 UTC #10 lore raw

Quoted 32 lines · Jan Kiszka

Quoted 24 lines · Michael S. Tsirkin
Quoted 23 lines · Jan Kiszka
Quoted 21 lines · Isaku Yamahata
Quoted 10 lines · Jan Kiszka
Quoted 9 lines · Jan Kiszka
Quoted 1 line · Isaku Yamahata
Hi. This clean up looks good basically.
Oops, forgot to cc you. Sorry.
Quoted 3 lines
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.
Quoted 6 lines
Quoted 2 lines
Mmh, I see. Looks like we need to split accesses at this boundary and executed them separately.
Nope, no such issue: we already automatically split up accesses that span the legacy/extended boundary. Just like so far, legacy config space handlers have to filter out requests that address regions >= 256.
For example, when accessing to offset 257 of conventional pci device, the access is routed to offset 1 due to the masking.
Such overwrapping isn't correct.
Quoted 6 lines
Quoted 3 lines
No, it isn't routed like that. The mask used via mmio is 0xfff.
Jan
I thought about this some more, I'd like to see how devices are going to benefit. Any examples?

Quoted 8 lines

No in-tree device currently gets beyond the "write default, check range" pattern.
Quoted 2 lines
If not, is it easier to simply make this logic
part of dev assignment?
That's a question of clean interfaces. Not checking at the core means exposing invalid accesses to the callbacks.

That's the point: at least the express spec seems to explicitly allow any accesses within a dword, and disallow accesses crossing dwords. And there's no way to create such on the PCI bus either.

So it makes sense to handle cross-dword accesses in a special way (possibly disallow them altogether, need to check non-pc systems for this).

But non-aligned access within a dword is legal according to the spec.

Quoted 5 lines

The logic is required anyway, so better put it in a central place. E.g. if the Xen people push their device assignment as well, we will suddenly have more than one user.
Jan

That's the question: why is the logic required?

Signature · 1 line

MST

Michael S. Tsirkin

2011-07-20 14:21 UTC #11 lore raw

In reply to Jan Kiszka

Quoted 8 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>
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 <[email protected]>

Nice, this is possible now that everyone uses pci_host. Some comments below.

Quoted 16 lines

---

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(-)

hw/pci.c +0 -0

index b904a4e..6957ece 100644

hw/pci.c static void pci_update_irq_disabled(PCIDevice *d, int was_irq_disabled) · line 1108

Quoted 5 lines

hw/pci.c +2 -0

@@ -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)

Let's put these in pci_host.h/pci_host.c, and prefix in a way so we don't want devices to use this mistakenly. Also please add a comment here saying what this does. Maybe also note how, if some system device ever does need misaligned accesses, it will need some special hacks.

hw/pci.c static void pci_update_irq_disabled(PCIDevice *d, int was_irq_disabled) · line 1112

Quoted 4 lines

hw/pci.c +4 -0

+{
+    assert(len == 1 || len == 2 || len == 4);
+
+    addr &= addr_mask;

A 4 byte access at address 255 would wrap around to 0 with this, while previously we ignored high bits on write and returned 0 on read.

I'd say let's check config size. With some more comments this gets:

	if (addr >= pci_config_size(pci_dev)) {
		return;
	}

	if (likely(!(addr & (len - 1)))) {
	       pci_dev->config_write(pci_dev, addr, val, len);
	       return;
	}

	/* Handle unaligned access: split it to two accesses each
	 * half the length of the original one. */
	len = len / 2;
	pci_host_config_write(pci_dev, addr, addr_mask, val, len);
	pci_host_config_write(pci_dev, addr + len, config_size, val >> (len * 8), len);

I am kind of unhappy with adding recursion here but it does seem to be the least of evils, and it is rare (and can't happen on the pc).

hw/pci.c static void pci_update_irq_disabled(PCIDevice *d, int was_irq_disabled) · line 1114

Quoted 2 lines

hw/pci.c +2 -0

+
+    if (addr & (len - 1)) {

Let's add an unlikely here.

hw/pci.c static void pci_update_irq_disabled(PCIDevice *d, int was_irq_disabled) · line 1115

Quoted 1 line

hw/pci.c +1 -0
+        len >>= 1;

I know it's the same but I prefer len /= 2;

Quoted 139 lines

hw/pci.c +30 -4

+        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));

hw/pci.h +5 -0

index c220745..3d5b39c 100644
@@ -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

hw/pci_host.c +6 -8

index 728e2d4..db888fb 100644
@@ -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;
 }

hw/pcie_host.c +6 -6

index b749865..e10d7a3 100644
@@ -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,

Signature · 1 line

1.7.3.4

Jan Kiszka

2011-07-20 14:37 UTC #12 lore raw

Quoted 32 lines · Michael S. Tsirkin

Quoted 8 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
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 <[email protected]>
Nice, this is possible now that everyone uses pci_host. Some comments below.
Quoted 19 lines
---
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(-)
hw/pci.c +0 -0
index b904a4e..6957ece 100644
@@ -1104,12 +1104,48 @@ static void pci_update_irq_disabled(PCIDevice *d, int was_irq_disabled)
     }
 }
 

Quoted 5 lines

hw/pci.c static void pci_update_irq_disabled(PCIDevice *d, int was_irq_disabled) · line 1108
Quoted 2 lines
hw/pci.c +2 -0
+void pci_config_write(PCIDevice *pci_dev, uint32_t addr, uint32_t addr_mask,
+                      uint32_t val, uint32_t len)
Let's put these in pci_host.h/pci_host.c, and prefix in a way so we don't want devices to use this mistakenly.

I didn't find direct relations between pcie_host.c and pci_host.c in the current code, so went for pci.c - but no problem to change.

pci_host_config_read/write?

Quoted 3 lines

Also please add a comment here saying what this does. Maybe also note how, if some system device ever does need misaligned accesses, it will need some special hacks.

OK.

Quoted 9 lines

hw/pci.c static void pci_update_irq_disabled(PCIDevice *d, int was_irq_disabled) · line 1112
Quoted 5 lines
hw/pci.c +4 -0
+{
+    assert(len == 1 || len == 2 || len == 4);
+
+    addr &= addr_mask;
A 4 byte access at address 255 would wrap around to 0 with this, while previously we ignored high bits on write and returned 0 on read.

The question is rather what the spec or real hw demand from us. Do you have any insights on this? I didn't find hints which behavior is more compliant. It looks like such access patterns are deep into bug land anyway.

Quoted 19 lines

I'd say let's check config size. With some more comments this gets:

	if (addr >= pci_config_size(pci_dev)) {
		return;
	}

	if (likely(!(addr & (len - 1)))) {
	       pci_dev->config_write(pci_dev, addr, val, len);
	       return;
	}

	/* Handle unaligned access: split it to two accesses each
	 * half the length of the original one. */
	len = len / 2;
	pci_host_config_write(pci_dev, addr, addr_mask, val, len);
	pci_host_config_write(pci_dev, addr + len, config_size, val >> (len * 8), len);

Quoted 3 lines

I am kind of unhappy with adding recursion here but it does seem to be the least of evils, and it is rare (and can't happen on the pc).

+ recursion depth is naturally limited.

Quoted 9 lines

hw/pci.c static void pci_update_irq_disabled(PCIDevice *d, int was_irq_disabled) · line 1114
Quoted 3 lines
hw/pci.c +2 -0
+
+    if (addr & (len - 1)) {
Let's add an unlikely here.
hw/pci.c static void pci_update_irq_disabled(PCIDevice *d, int was_irq_disabled) · line 1115
Quoted 1 line
hw/pci.c +1 -0
+        len >>= 1;
I know it's the same but I prefer len /= 2;

OK for both.

Thanks,
Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Michael S. Tsirkin

2011-07-20 15:47 UTC #13 lore raw

Quoted 7 lines · Jan Kiszka

Quoted 3 lines
A 4 byte access at address 255 would wrap around to 0 with this, while previously we ignored high bits on write and returned 0 on read.
The question is rather what the spec or real hw demand from us. Do you have any insights on this? I didn't find hints which behavior is more compliant. It looks like such access patterns are deep into bug land anyway.

For the express side, we don't need to support accesses that cross a 4 byte boundary - but guests might be able to generate them. Just in case, I'd prefer keeping the current behaviour WRT that. Paul, do you know which guests can do this?

However, as far as I can tell accesses within a dword can have any bytes enabled. So our interface for passing that info to devices isn't all that good, and I'm not 100% sure splitting up to length aligned transactions is better.
Maybe passing in a byte_enable mask is a better idea.

For example, as far as I an tell sysfs let you write/read any length (it splits it up internally ATM) so
it seems somewhat useless to do that work in
userspace as well.
We will, however, have to see how does the code look in the end.

Signature · 1 line

MST