qemu pci: pci_add_capability enhancement to prevent damaging config space
RFC PATCH 1/1 qemu pci: pci_add_capability enhancement to prevent damaging config space
Normally the pci_add_capability is called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.
In the case of VFIO the capability may already exist and adding new capability into the beginning of the linked list may create a loop.
For example, the old code destroys the following config of PCIe Intel E1000E:
before adding PCI_CAP_ID_MSI (0x05):0x34: 0xC80xC8: 0x01 0xD00xD0: 0x05 0xE00xE0: 0x10 0x00
after:0x34: 0xD00xC8: 0x01 0xD00xD0: 0x05 0xC80xE0: 0x10 0x00
As result capabilities 0x01 and 0x05 point to each other.
The proposed patch does not change capability pointers when the same type capability is about to add.
--- hw/pci.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-)
hw/pci.c +6 -4
@@ -1794,10 +1794,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, } config = pdev->config + offset; - config[PCI_CAP_LIST_ID ] = cap_id; - config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; - pdev->config[PCI_CAPABILITY_LIST ] = offset; - pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; +if (config[PCI_CAP_LIST_ID ] != cap_id) { + config[PCI_CAP_LIST_ID ] = cap_id; + config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; + pdev->config[PCI_CAPABILITY_LIST ] = offset; + pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; + }memset (pdev->used + offset,0xFF , size);/* Make capability read-only by default */ memset (pdev->wmask + offset,0 , size);
Signature · 1 line
Alexey
Quoted 37 lines · Alexey Kardashevskiy
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.In the case of VFIO the capability may already exist and adding new capability into the beginning of the linked list may create a loop.
For example, the old code destroys the following config of PCIe Intel E1000E:
before adding
PCI_CAP_ID_MSI(0x05):0x34:0xC80xC8:0x010xD00xD0:0x050xE00xE0:0x100x00after:
0x34:0xD00xC8:0x010xD00xD0:0x050xC80xE0:0x100x00As result capabilities
0x01and0x05point to each other.The proposed patch does not change capability pointers when the same type capability is about to add.
Signed-off-by Alexey Kardashevskiy <[email protected]>---
hw/pci.c| 10 ++++++----1 files changed, 6 insertions(+), 4 deletions(-)hw/pci.c +0
-0
hw/pci.c int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, · line 1797
Quoted 8 lines
hw/pci.c +1
-4@@ -1794,10 +1794,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, } config = pdev->config + offset; - config[PCI_CAP_LIST_ID ] = cap_id; - config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; - pdev->config[PCI_CAPABILITY_LIST ] = offset; - pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; +if (config[PCI_CAP_LIST_ID ] != cap_id) {
This doesn't scale. Capabilities are a list of CAPs. You'll have to do a loop through all capabilities, check if the one you want to add is there already and if so either
* replace the existing one or
* drop out and not write the new one in.
I'm not sure which way would be more natural.
Quoted 12 lines
hw/pci.c +5
-0+ config[PCI_CAP_LIST_ID ] = cap_id; + config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; + pdev->config[PCI_CAPABILITY_LIST ] = offset; + pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; + }memset (pdev->used + offset,0xFF , size);/* Make capability read-only by default */ memset (pdev->wmask + offset,0 , size);Signature · 2 lines
-- Alexey
11.05.2012 20:52, Alexander Graf написал:
Quoted 53 lines · Alexey Kardashevskiy
Quoted 38 lines
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.In the case of VFIO the capability may already exist and adding new capability into the beginning of the linked list may create a loop.
For example, the old code destroys the following config of PCIe Intel E1000E:
before adding
PCI_CAP_ID_MSI(0x05):0x34:0xC80xC8:0x010xD00xD0:0x050xE00xE0:0x100x00after:
0x34:0xD00xC8:0x010xD00xD0:0x050xC80xE0:0x100x00As result capabilities
0x01and0x05point to each other.The proposed patch does not change capability pointers when the same type capability is about to add.
Signed-off-by Alexey Kardashevskiy <[email protected]>---
hw/pci.c| 10 ++++++----1 files changed, 6 insertions(+), 4 deletions(-)hw/pci.c +0
-0hw/pci.c int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, · line 1797
Quoted 8 lines
hw/pci.c +1
-4@@ -1794,10 +1794,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, } config = pdev->config + offset; - config[PCI_CAP_LIST_ID ] = cap_id; - config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; - pdev->config[PCI_CAPABILITY_LIST ] = offset; - pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; +if (config[PCI_CAP_LIST_ID ] != cap_id) {This doesn't scale. Capabilities are a list of CAPs. You'll have to do a loop through all capabilities, check if the one you want to add is there already and if so either
* replace the existing one or
* drop out and not write the new one in.
Quoted 1 line
I'm not sure which way would be more natural.
There is a third option - add another function, lets call it pci_fixup_capability() which would do whatever pci_add_capability() does but won't touch list pointers.
When vfio, pci_add_capability() is called from the code which knows exactly that the capability exists and where it is and it calls pci_add_capability() based on this knowledge so doing additional loops just for imaginery scalability is a bit weird, no?
Quoted 8 lines
Quoted 8 lines
hw/pci.c +5
-0+ config[PCI_CAP_LIST_ID ] = cap_id; + config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; + pdev->config[PCI_CAPABILITY_LIST ] = offset; + pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; + }memset (pdev->used + offset,0xFF , size);/* Make capability read-only by default */ memset (pdev->wmask + offset,0 , size);
Signature · 3 lines
With best regards Alexey Kardashevskiy
Quoted 53 lines · Alexey Kardashevskiy
11.05.2012 20:52, Alexander Graf написал:
Quoted 52 lines · Alexey Kardashevskiy
Quoted 38 lines
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.In the case of VFIO the capability may already exist and adding new capability into the beginning of the linked list may create a loop.
For example, the old code destroys the following config of PCIe Intel E1000E:
before adding
PCI_CAP_ID_MSI(0x05):0x34:0xC80xC8:0x010xD00xD0:0x050xE00xE0:0x100x00after:
0x34:0xD00xC8:0x010xD00xD0:0x050xC80xE0:0x100x00As result capabilities
0x01and0x05point to each other.The proposed patch does not change capability pointers when the same type capability is about to add.
Signed-off-by Alexey Kardashevskiy <[email protected]>---
hw/pci.c| 10 ++++++----1 files changed, 6 insertions(+), 4 deletions(-)hw/pci.c +0
-0hw/pci.c int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, · line 1797
Quoted 8 lines
hw/pci.c +1
-4@@ -1794,10 +1794,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, } config = pdev->config + offset; - config[PCI_CAP_LIST_ID ] = cap_id; - config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; - pdev->config[PCI_CAPABILITY_LIST ] = offset; - pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; +if (config[PCI_CAP_LIST_ID ] != cap_id) {This doesn't scale. Capabilities are a list of CAPs. You'll have to do a loop through all capabilities, check if the one you want to add is there already and if so either
* replace the existing one or
* drop out and not write the new one in.
* hw_error :)
Quoted 6 lines
Quoted 2 lines
I'm not sure which way would be more natural.
There is a third option - add another function, lets call it
pci_fixup_capability()which would do whateverpci_add_capability()does but won't touch list pointers.
What good is a function that breaks internal consistency?
Quoted 4 lines
When vfio,
pci_add_capability()is called from the code which knows exactly that the capability exists and where it is and it callspci_add_capability()based on this knowledge so doing additional loops just for imaginery scalability is a bit weird, no?
Not sure I understand your proposal. The more generic a framework is, the better, no? In this code path we don't care about speed. We only care about consistency and reliability.
Alex
Quoted 60 lines · Alexander Graf
Quoted 53 lines · Alexey Kardashevskiy
11.05.2012 20:52, Alexander Graf написал:
Quoted 52 lines · Alexey Kardashevskiy
Quoted 38 lines
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.In the case of VFIO the capability may already exist and adding new capability into the beginning of the linked list may create a loop.
For example, the old code destroys the following config of PCIe Intel E1000E:
before adding
PCI_CAP_ID_MSI(0x05):0x34:0xC80xC8:0x010xD00xD0:0x050xE00xE0:0x100x00after:
0x34:0xD00xC8:0x010xD00xD0:0x050xC80xE0:0x100x00As result capabilities
0x01and0x05point to each other.The proposed patch does not change capability pointers when the same type capability is about to add.
Signed-off-by Alexey Kardashevskiy <[email protected]>---
hw/pci.c| 10 ++++++----1 files changed, 6 insertions(+), 4 deletions(-)hw/pci.c +0
-0hw/pci.c int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, · line 1797
Quoted 8 lines
hw/pci.c +1
-4@@ -1794,10 +1794,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, } config = pdev->config + offset; - config[PCI_CAP_LIST_ID ] = cap_id; - config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; - pdev->config[PCI_CAPABILITY_LIST ] = offset; - pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; +if (config[PCI_CAP_LIST_ID ] != cap_id) {This doesn't scale. Capabilities are a list of CAPs. You'll have to do a loop through all capabilities, check if the one you want to add is there already and if so either
* replace the existing one or
* drop out and not write the new one in.*
hw_error:)Quoted 1 line
Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
I'm not sure which way would be more natural.
There is a third option - add another function, lets call it
pci_fixup_capability()which would do whateverpci_add_capability()does but won't touch list pointers.What good is a function that breaks internal consistency?
It is broken already by having PCIDevice.used field. Normally pci_add_capability() would go through the whole list and add a capability if it does not exist. Emulated devices which care about having a capability at some fixed offset would have initialized their config space before calling this capabilities API (as VFIO does).
If we really want to support emulated devices which want some capabilities be at fixed offset and others at random offsets (strange, but ok), I do not see how it is bad to restore this consistency by special function (pci_fixup_capability()) to avoid its rewriting at different location as a guest driver may care about its offset.
Quoted 10 lines
Quoted 4 lines
When vfio,
pci_add_capability()is called from the code which knows exactly that the capability exists and where it is and it callspci_add_capability()based on this knowledge so doing additional loops just for imaginery scalability is a bit weird, no?Not sure I understand your proposal. The more generic a framework is, the better, no? In this code path we don't care about speed. We only care about consistency and reliability.
Alex
Signature · 1 line
Alexey
Alexander,
Is that any better? :)
@@ -1779,11 +1779,29 @@ static void pci_del_option_rom(PCIDevice *pdev)
* in pci config space */
int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
uint8_t offset, uint8_t size)
{
- uint8_t *config;
+ uint8_t *config, existing;
int i, overlapping_cap;+ existing = pci_find_capability(pdev, cap_id);
+ if (existing) {
+ if (offset && (existing != offset)) {
+ return -EEXIST;
+ }
+ for (i = existing; i < size; ++i) {
+ if (pdev->used[i]) {
+ return -EFAULT;
+ }
+ }
+ memset(pdev->used + offset, 0xFF, size);
+ /* Make capability read-only by default */
+ memset(pdev->wmask + offset, 0, size);
+ /* Check capability by default */
+ memset(pdev->cmask + offset, 0xFF, size);
+ return existing;
+ }
+
if (!offset) {
offset = pci_find_space(pdev, size);
if (!offset) {
return -ENOSPC;Quoted 94 lines · Alexey Kardashevskiy
Quoted 60 lines · Alexander Graf
Quoted 53 lines · Alexey Kardashevskiy
11.05.2012 20:52, Alexander Graf написал:
Quoted 52 lines · Alexey Kardashevskiy
Quoted 38 lines
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.In the case of VFIO the capability may already exist and adding new capability into the beginning of the linked list may create a loop.
For example, the old code destroys the following config of PCIe Intel E1000E:
before adding
PCI_CAP_ID_MSI(0x05):0x34:0xC80xC8:0x010xD00xD0:0x050xE00xE0:0x100x00after:
0x34:0xD00xC8:0x010xD00xD0:0x050xC80xE0:0x100x00As result capabilities
0x01and0x05point to each other.The proposed patch does not change capability pointers when the same type capability is about to add.
Signed-off-by Alexey Kardashevskiy <[email protected]>---
hw/pci.c| 10 ++++++----1 files changed, 6 insertions(+), 4 deletions(-)hw/pci.c +0
-0hw/pci.c int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, · line 1797
Quoted 8 lines
hw/pci.c +1
-4@@ -1794,10 +1794,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, } config = pdev->config + offset; - config[PCI_CAP_LIST_ID ] = cap_id; - config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; - pdev->config[PCI_CAPABILITY_LIST ] = offset; - pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; +if (config[PCI_CAP_LIST_ID ] != cap_id) {This doesn't scale. Capabilities are a list of CAPs. You'll have to do a loop through all capabilities, check if the one you want to add is there already and if so either
* replace the existing one or
* drop out and not write the new one in.*
hw_error:)Quoted 1 line
Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
I'm not sure which way would be more natural.
There is a third option - add another function, lets call it
pci_fixup_capability()which would do whateverpci_add_capability()does but won't touch list pointers.What good is a function that breaks internal consistency?
It is broken already by having PCIDevice.used field. Normally
pci_add_capability()would go through the whole list and add a capability if it does not exist. Emulated devices which care about having a capability at some fixed offset would have initialized their config space before calling this capabilities API (as VFIO does).If we really want to support emulated devices which want some capabilities be at fixed offset and others at random offsets (strange, but ok), I do not see how it is bad to restore this consistency by special function (
pci_fixup_capability()) to avoid its rewriting at different location as a guest driver may care about its offset.Quoted 10 lines
Quoted 4 lines
When vfio,
pci_add_capability()is called from the code which knows exactly that the capability exists and where it is and it callspci_add_capability()based on this knowledge so doing additional loops just for imaginery scalability is a bit weird, no?Not sure I understand your proposal. The more generic a framework is, the better, no? In this code path we don't care about speed. We only care about consistency and reliability.
Alex
Signature · 1 line
Alexey
Quoted 3 lines · Alexey Kardashevskiy
Alexander,
Is that any better? :)
Alex (Graf that is), ping ?
The original patch from Alexey was fine btw.
VFIO will always call things with the existing capability offset so there's no real risk of doing the wrong thing or break the list or anything.
IE. A small simple patch that addresses the problem :-)
The new patch is a bit more "robust" I believe, I don't think we need to go too far to fix a problem we don't have. But we need a fix for the real issue and the simple patch does it neatly from what I can understand.
Cheers,
Ben.
Quoted 135 lines
(quoted diff) +19
-1@@ -1779,11 +1779,29 @@ static void pci_del_option_rom(PCIDevice *pdev) * in pci config space */ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t offset, uint8_t size) { - uint8_t *config; + uint8_t *config, existing; int i, overlapping_cap; + existing = pci_find_capability(pdev, cap_id); + if (existing) { + if (offset && (existing != offset)) { + return -EEXIST; + } + for (i = existing; i < size; ++i) { + if (pdev->used[i]) { + return -EFAULT; + } + } + memset(pdev->used + offset, 0xFF, size); + /* Make capability read-only by default */ + memset(pdev->wmask + offset, 0, size); + /* Check capability by default */ + memset(pdev->cmask + offset, 0xFF, size); + return existing; + } + if (!offset) { offset = pci_find_space(pdev, size); if (!offset) { return -ENOSPC;Quoted 94 lines · Alexey Kardashevskiy
Quoted 60 lines · Alexander Graf
Quoted 53 lines · Alexey Kardashevskiy
11.05.2012 20:52, Alexander Graf написал:
Quoted 52 lines · Alexey Kardashevskiy
Quoted 38 lines
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.In the case of VFIO the capability may already exist and adding new capability into the beginning of the linked list may create a loop.
For example, the old code destroys the following config of PCIe Intel E1000E:
before adding
PCI_CAP_ID_MSI(0x05):0x34:0xC80xC8:0x010xD00xD0:0x050xE00xE0:0x100x00after:
0x34:0xD00xC8:0x010xD00xD0:0x050xC80xE0:0x100x00As result capabilities
0x01and0x05point to each other.The proposed patch does not change capability pointers when the same type capability is about to add.
Signed-off-by Alexey Kardashevskiy <[email protected]>---
hw/pci.c| 10 ++++++----1 files changed, 6 insertions(+), 4 deletions(-)hw/pci.c +0
-0hw/pci.c int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, · line 1797
Quoted 8 lines
hw/pci.c +1
-4@@ -1794,10 +1794,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, } config = pdev->config + offset; - config[PCI_CAP_LIST_ID ] = cap_id; - config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; - pdev->config[PCI_CAPABILITY_LIST ] = offset; - pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; +if (config[PCI_CAP_LIST_ID ] != cap_id) {This doesn't scale. Capabilities are a list of CAPs. You'll have to do a loop through all capabilities, check if the one you want to add is there already and if so either
* replace the existing one or
* drop out and not write the new one in.*
hw_error:)Quoted 1 line
Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
I'm not sure which way would be more natural.
There is a third option - add another function, lets call it
pci_fixup_capability()which would do whateverpci_add_capability()does but won't touch list pointers.What good is a function that breaks internal consistency?
It is broken already by having PCIDevice.used field. Normally
pci_add_capability()would go through the whole list and add a capability if it does not exist. Emulated devices which care about having a capability at some fixed offset would have initialized their config space before calling this capabilities API (as VFIO does).If we really want to support emulated devices which want some capabilities be at fixed offset and others at random offsets (strange, but ok), I do not see how it is bad to restore this consistency by special function (
pci_fixup_capability()) to avoid its rewriting at different location as a guest driver may care about its offset.Quoted 10 lines
Quoted 4 lines
When vfio,
pci_add_capability()is called from the code which knows exactly that the capability exists and where it is and it callspci_add_capability()based on this knowledge so doing additional loops just for imaginery scalability is a bit weird, no?Not sure I understand your proposal. The more generic a framework is, the better, no? In this code path we don't care about speed. We only care about consistency and reliability.
Alex
Quoted 24 lines · Benjamin Herrenschmidt
Quoted 3 lines · Alexey Kardashevskiy
Alexander,
Is that any better? :)
Alex (Graf that is), ping ?
The original patch from Alexey was fine btw.
VFIO will always call things with the existing capability offset so there's no real risk of doing the wrong thing or break the list or anything.
IE. A small simple patch that addresses the problem :-)
The new patch is a bit more "robust" I believe, I don't think we need to go too far to fix a problem we don't have. But we need a fix for the real issue and the simple patch does it neatly from what I can understand.
Cheers,
Ben.Quoted 1 line
Quoted 7 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1783
Quoted 7 lines
(quoted diff) +1
-1@@ -1779,11 +1779,29 @@ static void pci_del_option_rom(PCIDevice *pdev) * in pci config space */ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t offset, uint8_t size) { - uint8_t *config; + uint8_t *config, existing;
Existing is a pointer to the target dev's config space, right?
Quoted 8 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1791
Quoted 8 lines
(quoted diff) +6
-0int i, overlapping_cap; + existing = pci_find_capability(pdev, cap_id); + if (existing) { + if (offset && (existing != offset)) { + return -EEXIST; + } + for (i = existing; i < size; ++i) {
So how does this possibly make sense?
Quoted 5 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1796
Quoted 5 lines
(quoted diff) +5
-0+ if (pdev->used[i]) { + return -EFAULT; + } + } + memset(pdev->used + offset, 0xFF, size);
Why?
Quoted 2 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1798
Quoted 2 lines
(quoted diff) +2
-0+ /* Make capability read-only by default */ + memset(pdev->wmask + offset, 0, size);
Why?
Quoted 2 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1800
Quoted 2 lines
(quoted diff) +2
-0+ /* Check capability by default */ + memset(pdev->cmask + offset, 0xFF, size);
I don't understand this part either.
Alex
Quoted 112 lines
Quoted 110 lines
(quoted diff) +3
-0+ return existing; + } + if (!offset) { offset = pci_find_space(pdev, size); if (!offset) { return -ENOSPC;Quoted 94 lines · Alexey Kardashevskiy
Quoted 60 lines · Alexander Graf
Quoted 53 lines · Alexey Kardashevskiy
11.05.2012 20:52, Alexander Graf написал:
Quoted 52 lines · Alexey Kardashevskiy
Quoted 38 lines
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.In the case of VFIO the capability may already exist and adding new capability into the beginning of the linked list may create a loop.
For example, the old code destroys the following config of PCIe Intel E1000E:
before adding
PCI_CAP_ID_MSI(0x05):0x34:0xC80xC8:0x010xD00xD0:0x050xE00xE0:0x100x00after:
0x34:0xD00xC8:0x010xD00xD0:0x050xC80xE0:0x100x00As result capabilities
0x01and0x05point to each other.The proposed patch does not change capability pointers when the same type capability is about to add.
Signed-off-by Alexey Kardashevskiy <[email protected]>---
hw/pci.c| 10 ++++++----1 files changed, 6 insertions(+), 4 deletions(-)hw/pci.c +0
-0hw/pci.c int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, · line 1797
Quoted 8 lines
hw/pci.c +1
-4@@ -1794,10 +1794,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, } config = pdev->config + offset; - config[PCI_CAP_LIST_ID ] = cap_id; - config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; - pdev->config[PCI_CAPABILITY_LIST ] = offset; - pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; +if (config[PCI_CAP_LIST_ID ] != cap_id) {This doesn't scale. Capabilities are a list of CAPs. You'll have to do a loop through all capabilities, check if the one you want to add is there already and if so either
* replace the existing one or
* drop out and not write the new one in.*
hw_error:)Quoted 1 line
Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
I'm not sure which way would be more natural.
There is a third option - add another function, lets call it
pci_fixup_capability()which would do whateverpci_add_capability()does but won't touch list pointers.What good is a function that breaks internal consistency?
It is broken already by having PCIDevice.used field. Normally
pci_add_capability()would go through the whole list and add a capability if it does not exist. Emulated devices which care about having a capability at some fixed offset would have initialized their config space before calling this capabilities API (as VFIO does).If we really want to support emulated devices which want some capabilities be at fixed offset and others at random offsets (strange, but ok), I do not see how it is bad to restore this consistency by special function (
pci_fixup_capability()) to avoid its rewriting at different location as a guest driver may care about its offset.Quoted 10 lines
Quoted 4 lines
When vfio,
pci_add_capability()is called from the code which knows exactly that the capability exists and where it is and it callspci_add_capability()based on this knowledge so doing additional loops just for imaginery scalability is a bit weird, no?Not sure I understand your proposal. The more generic a framework is, the better, no? In this code path we don't care about speed. We only care about consistency and reliability.
Alex
Quoted 28 lines · Alexander Graf
Quoted 24 lines · Benjamin Herrenschmidt
Quoted 3 lines · Alexey Kardashevskiy
Alexander,
Is that any better? :)
Alex (Graf that is), ping ?
The original patch from Alexey was fine btw.
VFIO will always call things with the existing capability offset so there's no real risk of doing the wrong thing or break the list or anything.
IE. A small simple patch that addresses the problem :-)
The new patch is a bit more "robust" I believe, I don't think we need to go too far to fix a problem we don't have. But we need a fix for the real issue and the simple patch does it neatly from what I can understand.
Cheers,
Ben.Quoted 1 line
Quoted 9 lines
Quoted 7 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1783
Quoted 7 lines
(quoted diff) +1
-1@@ -1779,11 +1779,29 @@ static void pci_del_option_rom(PCIDevice *pdev) * in pci config space */ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t offset, uint8_t size) { - uint8_t *config; + uint8_t *config, existing;Existing is a pointer to the target dev's config space, right?
Yes.
Quoted 10 lines
Quoted 8 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1791
Quoted 8 lines
(quoted diff) +6
-0int i, overlapping_cap; + existing = pci_find_capability(pdev, cap_id); + if (existing) { + if (offset && (existing != offset)) { + return -EEXIST; + } + for (i = existing; i < size; ++i) {So how does this possibly make sense?
Although I do not expect VFIO to add capabilities (does not make sense), I still want to double check that this space has not been tried to use by someone else.
Quoted 6 lines
Quoted 5 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1796
Quoted 5 lines
(quoted diff) +5
-0+ if (pdev->used[i]) { + return -EFAULT; + } + } + memset(pdev->used + offset, 0xFF, size);Why?
Because I am marking the space this capability takes as used.
Quoted 3 lines
Quoted 2 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1798
Quoted 2 lines
(quoted diff) +2
-0+ /* Make capability read-only by default */ + memset(pdev->wmask + offset, 0, size);Why?
Because the pci_add_capability() does it for a new capability by default.
Quoted 4 lines
Quoted 2 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1800
Quoted 2 lines
(quoted diff) +2
-0+ /* Check capability by default */ + memset(pdev->cmask + offset, 0xFF, size);I don't understand this part either.
The pci_add_capability() does it for a new capability by default.
Quoted 115 lines
Alex
Quoted 112 lines
Quoted 110 lines
(quoted diff) +3
-0+ return existing; + } + if (!offset) { offset = pci_find_space(pdev, size); if (!offset) { return -ENOSPC;Quoted 94 lines · Alexey Kardashevskiy
Quoted 60 lines · Alexander Graf
Quoted 53 lines · Alexey Kardashevskiy
11.05.2012 20:52, Alexander Graf написал:
Quoted 52 lines · Alexey Kardashevskiy
Quoted 38 lines
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.In the case of VFIO the capability may already exist and adding new capability into the beginning of the linked list may create a loop.
For example, the old code destroys the following config of PCIe Intel E1000E:
before adding
PCI_CAP_ID_MSI(0x05):0x34:0xC80xC8:0x010xD00xD0:0x050xE00xE0:0x100x00after:
0x34:0xD00xC8:0x010xD00xD0:0x050xC80xE0:0x100x00As result capabilities
0x01and0x05point to each other.The proposed patch does not change capability pointers when the same type capability is about to add.
Signed-off-by Alexey Kardashevskiy <[email protected]>---
hw/pci.c| 10 ++++++----1 files changed, 6 insertions(+), 4 deletions(-)hw/pci.c +0
-0hw/pci.c int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, · line 1797
Quoted 8 lines
hw/pci.c +1
-4@@ -1794,10 +1794,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, } config = pdev->config + offset; - config[PCI_CAP_LIST_ID ] = cap_id; - config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; - pdev->config[PCI_CAPABILITY_LIST ] = offset; - pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; +if (config[PCI_CAP_LIST_ID ] != cap_id) {This doesn't scale. Capabilities are a list of CAPs. You'll have to do a loop through all capabilities, check if the one you want to add is there already and if so either
* replace the existing one or
* drop out and not write the new one in.*
hw_error:)Quoted 1 line
Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
I'm not sure which way would be more natural.
There is a third option - add another function, lets call it
pci_fixup_capability()which would do whateverpci_add_capability()does but won't touch list pointers.What good is a function that breaks internal consistency?
It is broken already by having PCIDevice.used field. Normally
pci_add_capability()would go through the whole list and add a capability if it does not exist. Emulated devices which care about having a capability at some fixed offset would have initialized their config space before calling this capabilities API (as VFIO does).If we really want to support emulated devices which want some capabilities be at fixed offset and others at random offsets (strange, but ok), I do not see how it is bad to restore this consistency by special function (
pci_fixup_capability()) to avoid its rewriting at different location as a guest driver may care about its offset.Quoted 10 lines
Quoted 4 lines
When vfio,
pci_add_capability()is called from the code which knows exactly that the capability exists and where it is and it callspci_add_capability()based on this knowledge so doing additional loops just for imaginery scalability is a bit weird, no?Not sure I understand your proposal. The more generic a framework is, the better, no? In this code path we don't care about speed. We only care about consistency and reliability.
Alex
Signature · 1 line
Alexey
Quoted 50 lines · Alexey Kardashevskiy
Quoted 28 lines · Alexander Graf
Quoted 24 lines · Benjamin Herrenschmidt
Quoted 3 lines · Alexey Kardashevskiy
Alexander,
Is that any better? :)
Alex (Graf that is), ping ?
The original patch from Alexey was fine btw.
VFIO will always call things with the existing capability offset so there's no real risk of doing the wrong thing or break the list or anything.
IE. A small simple patch that addresses the problem :-)
The new patch is a bit more "robust" I believe, I don't think we need to go too far to fix a problem we don't have. But we need a fix for the real issue and the simple patch does it neatly from what I can understand.
Cheers,
Ben.Quoted 1 line
Quoted 9 lines
Quoted 7 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1783
Quoted 7 lines
(quoted diff) +1
-1@@ -1779,11 +1779,29 @@ static void pci_del_option_rom(PCIDevice *pdev) * in pci config space */ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t offset, uint8_t size) { - uint8_t *config; + uint8_t *config, existing;Existing is a pointer to the target dev's config space, right?
Yes.
Quoted 9 lines
Quoted 8 lines
Quoted 8 lines
(quoted diff) +6
-0int i, overlapping_cap; + existing = pci_find_capability(pdev, cap_id); + if (existing) { + if (offset && (existing != offset)) { + return -EEXIST; + } + for (i = existing; i < size; ++i) {
Quoted 4 lines
Quoted 1 line
So how does this possibly make sense?
Although I do not expect VFIO to add capabilities (does not make sense), I still want to double check that this space has not been tried to use by someone else.
i is an int. existing is a uint8_t*.
Quoted 9 lines
Quoted 7 lines
Quoted 6 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1796
Quoted 6 lines
(quoted diff) +5
-0+ if (pdev->used[i]) { + return -EFAULT; + } + } + memset(pdev->used + offset, 0xFF, size);Why?
Because I am marking the space this capability takes as used.
But if it already existed (at the same offset), it should be set used already, no? Unless size > existing size, in which case you might overwrite data in the next chunk, no?
Quoted 6 lines
Quoted 4 lines
Quoted 3 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1798
Quoted 3 lines
(quoted diff) +2
-0+ /* Make capability read-only by default */ + memset(pdev->wmask + offset, 0, size);Why?
Because the
pci_add_capability()does it for a new capability by default.
Hrm. So you're copying code? Can't you merge the overwrite and write cases?
Alex
Quoted 130 lines
Quoted 6 lines
Quoted 4 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1800
Quoted 4 lines
(quoted diff) +2
-0+ /* Check capability by default */ + memset(pdev->cmask + offset, 0xFF, size);I don't understand this part either.
The
pci_add_capability()does it for a new capability by default.Quoted 115 lines
Alex
Quoted 112 lines
Quoted 110 lines
(quoted diff) +3
-0+ return existing; + } + if (!offset) { offset = pci_find_space(pdev, size); if (!offset) { return -ENOSPC;Quoted 94 lines · Alexey Kardashevskiy
Quoted 60 lines · Alexander Graf
Quoted 53 lines · Alexey Kardashevskiy
11.05.2012 20:52, Alexander Graf написал:
Quoted 52 lines · Alexey Kardashevskiy
Quoted 38 lines
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.In the case of VFIO the capability may already exist and adding new capability into the beginning of the linked list may create a loop.
For example, the old code destroys the following config of PCIe Intel E1000E:
before adding
PCI_CAP_ID_MSI(0x05):0x34:0xC80xC8:0x010xD00xD0:0x050xE00xE0:0x100x00after:
0x34:0xD00xC8:0x010xD00xD0:0x050xC80xE0:0x100x00As result capabilities
0x01and0x05point to each other.The proposed patch does not change capability pointers when the same type capability is about to add.
Signed-off-by Alexey Kardashevskiy <[email protected]>---
hw/pci.c| 10 ++++++----1 files changed, 6 insertions(+), 4 deletions(-)hw/pci.c +0
-0hw/pci.c int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, · line 1797
Quoted 8 lines
hw/pci.c +1
-4@@ -1794,10 +1794,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, } config = pdev->config + offset; - config[PCI_CAP_LIST_ID ] = cap_id; - config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; - pdev->config[PCI_CAPABILITY_LIST ] = offset; - pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; +if (config[PCI_CAP_LIST_ID ] != cap_id) {This doesn't scale. Capabilities are a list of CAPs. You'll have to do a loop through all capabilities, check if the one you want to add is there already and if so either
* replace the existing one or
* drop out and not write the new one in.*
hw_error:)Quoted 1 line
Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
I'm not sure which way would be more natural.
There is a third option - add another function, lets call it
pci_fixup_capability()which would do whateverpci_add_capability()does but won't touch list pointers.What good is a function that breaks internal consistency?
It is broken already by having PCIDevice.used field. Normally
pci_add_capability()would go through the whole list and add a capability if it does not exist. Emulated devices which care about having a capability at some fixed offset would have initialized their config space before calling this capabilities API (as VFIO does).If we really want to support emulated devices which want some capabilities be at fixed offset and others at random offsets (strange, but ok), I do not see how it is bad to restore this consistency by special function (
pci_fixup_capability()) to avoid its rewriting at different location as a guest driver may care about its offset.Quoted 10 lines
Quoted 4 lines
When vfio,
pci_add_capability()is called from the code which knows exactly that the capability exists and where it is and it callspci_add_capability()based on this knowledge so doing additional loops just for imaginery scalability is a bit weird, no?Not sure I understand your proposal. The more generic a framework is, the better, no? In this code path we don't care about speed. We only care about consistency and reliability.
Alex
Signature · 1 line
Alexey
Quoted 54 lines · Alexander Graf
Quoted 50 lines · Alexey Kardashevskiy
Quoted 28 lines · Alexander Graf
Quoted 24 lines · Benjamin Herrenschmidt
Quoted 3 lines · Alexey Kardashevskiy
Alexander,
Is that any better? :)
Alex (Graf that is), ping ?
The original patch from Alexey was fine btw.
VFIO will always call things with the existing capability offset so there's no real risk of doing the wrong thing or break the list or anything.
IE. A small simple patch that addresses the problem :-)
The new patch is a bit more "robust" I believe, I don't think we need to go too far to fix a problem we don't have. But we need a fix for the real issue and the simple patch does it neatly from what I can understand.
Cheers,
Ben.Quoted 1 line
Quoted 9 lines
Quoted 7 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1783
Quoted 7 lines
(quoted diff) +1
-1@@ -1779,11 +1779,29 @@ static void pci_del_option_rom(PCIDevice *pdev) * in pci config space */ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t offset, uint8_t size) { - uint8_t *config; + uint8_t *config, existing;Existing is a pointer to the target dev's config space, right?
Yes.
Quoted 9 lines
Quoted 8 lines
Quoted 8 lines
(quoted diff) +6
-0int i, overlapping_cap; + existing = pci_find_capability(pdev, cap_id); + if (existing) { + if (offset && (existing != offset)) { + return -EEXIST; + } + for (i = existing; i < size; ++i) {
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
So how does this possibly make sense?
Although I do not expect VFIO to add capabilities (does not make sense), I still want to double check that this space has not been tried to use by someone else.
i is an int. existing is a uint8_t*.
It was there before me. This function already does a loop and this is how it was coded at the first place.
Quoted 10 lines
Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1796
Quoted 5 lines
(quoted diff) +5
-0+ if (pdev->used[i]) { + return -EFAULT; + } + } + memset(pdev->used + offset, 0xFF, size);Why?
Because I am marking the space this capability takes as used.
But if it already existed (at the same offset), it should be set used already, no? Unless size > existing size, in which case you might overwrite data in the next chunk, no?
No, it does not exist for VFIO - VFIO read the config space from the host kernel first and then calls msi_init or msix_init or whatever_else_init depending on what it got from the host kernel. And these xxx_init() functions eventually call pci_add_capability().
Sure we can either implement own msi_init/msix_init (and may be others in the future) for VFIO (which would do all the same as other QEMU devices except touching the capabilities) OR hack msi_init/msix_init not to touch capabilities if they exist.
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Quoted 2 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1798
Quoted 2 lines
(quoted diff) +2
-0+ /* Make capability read-only by default */ + memset(pdev->wmask + offset, 0, size);Why?
Because the
pci_add_capability()does it for a new capability by default.Hrm. So you're copying code? Can't you merge the overwrite and write cases?
I am trying to make it as a single chunk which is as small as possible.
If it helps, below is the same patch with extended context to see what is going on in that function.
hw/pci.c | 20 +++++++++++++++++++-
1 files changed, 19 insertions(+), 1 deletions(-)
hw/pci.c +19 -1
@@ -1772,75 +1772,93 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) ptr =memory_region_get_ram_ptr (&pdev->rom);load_image (path, ptr);g_free (path);if (is_default_rom) {/* Only the default rom images will be patched (if needed). */ pci_patch_ids (pdev, ptr, size); }qemu_put_ram_ptr (ptr);pci_register_bar (pdev,PCI_ROM_SLOT ,0 , &pdev->rom);return 0 ; }static void pci_del_option_rom (PCIDevice *pdev) {if (!pdev->has_rom)return ;vmstate_unregister_ram (&pdev->rom, &pdev->qdev);memory_region_destroy (&pdev->rom); pdev->has_rom = false; }/* * if !offset * Reserve space and add capability to the linked list in pci config space * * if offset = 0, * Find and reserve space and add capability to the linked list * in pci config space */ int pci_add_capability (PCIDevice *pdev,uint8_t cap_id,uint8_t offset,uint8_t size) { -uint8_t *config; +uint8_t *config, existing;int i, overlapping_cap; + existing =pci_find_capability (pdev, cap_id); +if (existing) { +if (offset && (existing != offset)) { +return -EEXIST ; + } +for (i = existing; i < size; ++i) { +if (pdev->used[i]) { +return -EFAULT ; + } + } +memset (pdev->used + offset,0xFF , size); +/* Make capability read-only by default */ +memset (pdev->wmask + offset,0 , size); +/* Check capability by default */ +memset (pdev->cmask + offset,0xFF , size); +return existing; + } +if (!offset) { offset =pci_find_space (pdev, size);if (!offset) {return -ENOSPC ; } }else {/* Verify that capabilities don't overlap. Note: device assignment * depends on this check to verify that the device is not broken. * Should never trigger for emulated devices, but it's helpful * for debugging these. */ for (i = offset; i < offset + size; i++) { overlapping_cap =pci_find_capability_at_offset (pdev, i);if (overlapping_cap) {fprintf (stderr,"ERROR: %04x:%02x:%02x.%x " "Attempt to add PCI capability %x at offset " "%x overlaps existing capability %x at offset %x\n" ,pci_find_domain (pdev->bus),pci_bus_num (pdev->bus),PCI_SLOT (pdev->devfn),PCI_FUNC (pdev->devfn), cap_id, offset, overlapping_cap, i);return -EINVAL ; } } } config = pdev->config + offset; config[PCI_CAP_LIST_ID ] = cap_id; config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; pdev->config[PCI_CAPABILITY_LIST ] = offset; pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ;memset (pdev->used + offset,0xFF , size);/* Make capability read-only by default */ memset (pdev->wmask + offset,0 , size);/* Check capability by default */ memset (pdev->cmask + offset,0xFF , size);return offset; }
Quoted 128 lines
Quoted 128 lines
Quoted 4 lines
Quoted 2 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1800
Quoted 2 lines
(quoted diff) +2
-0+ /* Check capability by default */ + memset(pdev->cmask + offset, 0xFF, size);I don't understand this part either.
The
pci_add_capability()does it for a new capability by default.Quoted 115 lines
Alex
Quoted 112 lines
Quoted 110 lines
(quoted diff) +3
-0+ return existing; + } + if (!offset) { offset = pci_find_space(pdev, size); if (!offset) { return -ENOSPC;Quoted 94 lines · Alexey Kardashevskiy
Quoted 60 lines · Alexander Graf
Quoted 53 lines · Alexey Kardashevskiy
11.05.2012 20:52, Alexander Graf =0?8A0;:
Quoted 52 lines · Alexey Kardashevskiy
Quoted 38 lines
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.In the case of VFIO the capability may already exist and adding new capability into the beginning of the linked list may create a loop.
For example, the old code destroys the following config of PCIe Intel E1000E:
before adding
PCI_CAP_ID_MSI(0x05):0x34:0xC80xC8:0x010xD00xD0:0x050xE00xE0:0x100x00after:
0x34:0xD00xC8:0x010xD00xD0:0x050xC80xE0:0x100x00As result capabilities
0x01and0x05point to each other.The proposed patch does not change capability pointers when the same type capability is about to add.
Signed-off-by Alexey Kardashevskiy <[email protected]>---
hw/pci.c| 10 ++++++----1 files changed, 6 insertions(+), 4 deletions(-)hw/pci.c +0
-0hw/pci.c int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, · line 1797
Quoted 8 lines
hw/pci.c +1
-4@@ -1794,10 +1794,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, } config = pdev->config + offset; - config[PCI_CAP_LIST_ID ] = cap_id; - config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; - pdev->config[PCI_CAPABILITY_LIST ] = offset; - pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; +if (config[PCI_CAP_LIST_ID ] != cap_id) {This doesn't scale. Capabilities are a list of CAPs. You'll have to do a loop through all capabilities, check if the one you want to add is there already and if so either
* replace the existing one or
* drop out and not write the new one in.*
hw_error:)Quoted 1 line
Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
I'm not sure which way would be more natural.
There is a third option - add another function, lets call it
pci_fixup_capability()which would do whateverpci_add_capability()does but won't touch list pointers.What good is a function that breaks internal consistency?
It is broken already by having PCIDevice.used field. Normally
pci_add_capability()would go through the whole list and add a capability if it does not exist. Emulated devices which care about having a capability at some fixed offset would have initialized their config space before calling this capabilities API (as VFIO does).If we really want to support emulated devices which want some capabilities be at fixed offset and others at random offsets (strange, but ok), I do not see how it is bad to restore this consistency by special function (
pci_fixup_capability()) to avoid its rewriting at different location as a guest driver may care about its offset.Quoted 10 lines
Quoted 4 lines
When vfio,
pci_add_capability()is called from the code which knows exactly that the capability exists and where it is and it callspci_add_capability()based on this knowledge so doing additional loops just for imaginery scalability is a bit weird, no?Not sure I understand your proposal. The more generic a framework is, the better, no? In this code path we don't care about speed. We only care about consistency and reliability.
Alex
Signature · 1 line
Alexey
Signature · 1 line
Alexey
Quoted 57 lines · Alexey Kardashevskiy
Quoted 56 lines · Alexander Graf
Quoted 52 lines · Alexey Kardashevskiy
Quoted 28 lines · Alexander Graf
Quoted 24 lines · Benjamin Herrenschmidt
Quoted 3 lines · Alexey Kardashevskiy
Alexander,
Is that any better? :)
Alex (Graf that is), ping ?
The original patch from Alexey was fine btw.
VFIO will always call things with the existing capability offset so there's no real risk of doing the wrong thing or break the list or anything.
IE. A small simple patch that addresses the problem :-)
The new patch is a bit more "robust" I believe, I don't think we need to go too far to fix a problem we don't have. But we need a fix for the real issue and the simple patch does it neatly from what I can understand.
Cheers,
Ben.Quoted 1 line
Quoted 9 lines
Quoted 7 lines
hw/pci.c static void pci_del_option_rom(PCIDevice *pdev) · line 1783
Quoted 7 lines
hw/pci.c +1
-1@@ -1779,11 +1779,29 @@ static void pci_del_option_rom(PCIDevice *pdev)* in pci config space */ int pci_add_capability (PCIDevice *pdev,uint8_t cap_id,uint8_t offset,uint8_t size) { -uint8_t *config; +uint8_t *config, existing;Existing is a pointer to the target dev's config space, right?
Yes.
Quoted 10 lines
Quoted 8 lines
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1816
Quoted 8 lines
hw/pci.c +6
-0int i, overlapping_cap; + existing =pci_find_capability (pdev, cap_id); +if (existing) { +if (offset && (existing != offset)) { +return -EEXIST ; + } +for (i = existing; i < size; ++i) {So how does this possibly make sense?
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Although I do not expect VFIO to add capabilities (does not make sense), I still want to double check that this space has not been tried to use by someone else.
i is an int. existing is a uint8_t*.
It was there before me. This function already does a loop and this is how it was coded at the first place.
Ugh. Existing is a uint8_t, not a pointer. Gotta love C syntax...
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
hw/pci.c +5
-0+if (pdev->used[i]) { +return -EFAULT ; + } + } +memset (pdev->used + offset,0xFF , size);
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Why?
Because I am marking the space this capability takes as used.
But if it already existed (at the same offset), it should be set used already, no? Unless size > existing size, in which case you might overwrite data in the next chunk, no?
No, it does not exist for VFIO - VFIO read the config space from the host kernel first and then calls
msi_initormsix_initorwhatever_else_initdepending on what it got from the host kernel. And thesexxx_init()functions eventually callpci_add_capability().
So why would the function that populates the config space initially not set the used flag correctly?
Quoted 2 lines
Sure we can either implement own msi_init/msix_init (and may be others in the future) for VFIO (which would do all the same as other QEMU devices except touching the capabilities) OR hack msi_init/msix_init not to touch capabilities if they exist.
No, calling the internal functions sounds fine to me. It's the step before that irritates me. VFIO shouldn't differ too much from an emulated device wrt its config space population really.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
hw/pci.c +2
-0+/* Make capability read-only by default */ +memset (pdev->wmask + offset,0 , size);
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Why?
Because the
pci_add_capability()does it for a new capability by default.Hrm. So you're copying code? Can't you merge the overwrite and write cases?
I am trying to make it as a single chunk which is as small as possible.
No, you're needlessly duplicating code which is a bad idea :). Please reuse as much of the existing function as possible, unless it really doesn't make sense.
Quoted 57 lines
If it helps, below is the same patch with extended context to see what is going on in that function.
hw/pci.c| 20 +++++++++++++++++++-1 files changed, 19 insertions(+), 1 deletions(-)hw/pci.c +1
-1@@ -1772,75 +1772,93 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) ptr =memory_region_get_ram_ptr (&pdev->rom);load_image (path, ptr);g_free (path);if (is_default_rom) {/* Only the default rom images will be patched (if needed). */ pci_patch_ids (pdev, ptr, size); }qemu_put_ram_ptr (ptr);pci_register_bar (pdev,PCI_ROM_SLOT ,0 , &pdev->rom);return 0 ; }static void pci_del_option_rom (PCIDevice *pdev) {if (!pdev->has_rom)return ;vmstate_unregister_ram (&pdev->rom, &pdev->qdev);memory_region_destroy (&pdev->rom); pdev->has_rom = false; }/* * if !offset * Reserve space and add capability to the linked list in pci config space * * if offset = 0, * Find and reserve space and add capability to the linked list * in pci config space */ int pci_add_capability (PCIDevice *pdev,uint8_t cap_id,uint8_t offset,uint8_t size) { -uint8_t *config; +uint8_t *config, existing;int i, overlapping_cap;
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1820
Quoted 10 lines
hw/pci.c +10
-0+ existing =pci_find_capability (pdev, cap_id); +if (existing) { +if (offset && (existing != offset)) { +return -EEXIST ; + } +for (i = existing; i < size; ++i) { +if (pdev->used[i]) { +return -EFAULT ; + } + }
}
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1829
Quoted 9 lines
hw/pci.c +8
-0+memset (pdev->used + offset,0xFF , size); +/* Make capability read-only by default */ +memset (pdev->wmask + offset,0 , size); +/* Check capability by default */ +memset (pdev->cmask + offset,0xFF , size); +return existing; + } +if (!offset) {
&& !existing maybe?
Quoted 13 lines
hw/pci.c +0
-0offset =pci_find_space (pdev, size);if (!offset) {return -ENOSPC ; } }else {/* Verify that capabilities don't overlap. Note: device assignment * depends on this check to verify that the device is not broken. * Should never trigger for emulated devices, but it's helpful * for debugging these. */ for (i = offset; i < offset + size; i++) { overlapping_cap =pci_find_capability_at_offset (pdev, i);if (overlapping_cap) { fprintf(stderr, "ERROR: %04x:%02x:%02x.%x "
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1852
Quoted 10 lines
hw/pci.c +0
-0"Attempt to add PCI capability %x at offset " "%x overlaps existing capability %x at offset %x\n" ,pci_find_domain (pdev->bus),pci_bus_num (pdev->bus),PCI_SLOT (pdev->devfn),PCI_FUNC (pdev->devfn), cap_id, offset, overlapping_cap, i);return -EINVAL ; } } }
If (!existing) {
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1857
Quoted 5 lines
hw/pci.c +0
-0config = pdev->config + offset; config[PCI_CAP_LIST_ID ] = cap_id; config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; pdev->config[PCI_CAPABILITY_LIST ] = offset; pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ;
}
which poses the question why the above wouldn't apply for the existing case. It would work just as well to leave that in, no?
Alex
Quoted 143 lines
hw/pci.c +0
-0memset (pdev->used + offset,0xFF , size);/* Make capability read-only by default */ memset (pdev->wmask + offset,0 , size);/* Check capability by default */ memset (pdev->cmask + offset,0xFF , size);return offset; }Quoted 132 lines
Quoted 132 lines
Quoted 8 lines
Quoted 6 lines
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1825
Quoted 6 lines
hw/pci.c +2
-0+/* Check capability by default */ +memset (pdev->cmask + offset,0xFF , size);I don't understand this part either.
The
pci_add_capability()does it for a new capability by default.Quoted 115 lines
Alex
Quoted 112 lines
Quoted 110 lines
hw/pci.c +3
-0+return existing; + } +if (!offset) { offset =pci_find_space (pdev, size);if (!offset) {return -ENOSPC ;Quoted 94 lines · Alexey Kardashevskiy
Quoted 60 lines · Alexander Graf
Quoted 53 lines · Alexey Kardashevskiy
11.05.2012 20:52, Alexander Graf =0?8A0;:
Quoted 52 lines · Alexey Kardashevskiy
Quoted 38 lines
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.In the case of VFIO the capability may already exist and adding new capability into the beginning of the linked list may create a loop.
For example, the old code destroys the following config of PCIe Intel E1000E:
before adding
PCI_CAP_ID_MSI(0x05):0x34:0xC80xC8:0x010xD00xD0:0x050xE00xE0:0x100x00after:
0x34:0xD00xC8:0x010xD00xD0:0x050xC80xE0:0x100x00As result capabilities
0x01and0x05point to each other.The proposed patch does not change capability pointers when the same type capability is about to add.
Signed-off-by Alexey Kardashevskiy <[email protected]>---
hw/pci.c| 10 ++++++----1 files changed, 6 insertions(+), 4 deletions(-)hw/pci.c +0
-0hw/pci.c int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, · line 1797
Quoted 8 lines
hw/pci.c +1
-4@@ -1794,10 +1794,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, } config = pdev->config + offset; - config[PCI_CAP_LIST_ID ] = cap_id; - config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; - pdev->config[PCI_CAPABILITY_LIST ] = offset; - pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; +if (config[PCI_CAP_LIST_ID ] != cap_id) {This doesn't scale. Capabilities are a list of CAPs. You'll have to do a loop through all capabilities, check if the one you want to add is there already and if so either
* replace the existing one or
* drop out and not write the new one in.*
hw_error:)Quoted 1 line
Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
I'm not sure which way would be more natural.
There is a third option - add another function, lets call it
pci_fixup_capability()which would do whateverpci_add_capability()does but won't touch list pointers.What good is a function that breaks internal consistency?
It is broken already by having PCIDevice.used field. Normally
pci_add_capability()would go through the whole list and add a capability if it does not exist. Emulated devices which care about having a capability at some fixed offset would have initialized their config space before calling this capabilities API (as VFIO does).If we really want to support emulated devices which want some capabilities be at fixed offset and others at random offsets (strange, but ok), I do not see how it is bad to restore this consistency by special function (
pci_fixup_capability()) to avoid its rewriting at different location as a guest driver may care about its offset.Quoted 10 lines
Quoted 4 lines
When vfio,
pci_add_capability()is called from the code which knows exactly that the capability exists and where it is and it callspci_add_capability()based on this knowledge so doing additional loops just for imaginery scalability is a bit weird, no?Not sure I understand your proposal. The more generic a framework is, the better, no? In this code path we don't care about speed. We only care about consistency and reliability.
Alex
Signature · 1 line
AlexeySignature · 1 line
Alexey
Quoted 64 lines · Alexander Graf
Quoted 60 lines · Alexey Kardashevskiy
Quoted 59 lines · Alexander Graf
Quoted 54 lines · Alexey Kardashevskiy
Quoted 28 lines · Alexander Graf
Quoted 24 lines · Benjamin Herrenschmidt
Quoted 3 lines · Alexey Kardashevskiy
Alexander,
Is that any better? :)
Alex (Graf that is), ping ?
The original patch from Alexey was fine btw.
VFIO will always call things with the existing capability offset so there's no real risk of doing the wrong thing or break the list or anything.
IE. A small simple patch that addresses the problem :-)
The new patch is a bit more "robust" I believe, I don't think we need to go too far to fix a problem we don't have. But we need a fix for the real issue and the simple patch does it neatly from what I can understand.
Cheers,
Ben.Quoted 1 line
Quoted 9 lines
Quoted 7 lines
hw/pci.c static void pci_del_option_rom(PCIDevice *pdev) · line 1783
Quoted 7 lines
hw/pci.c +1
-1@@ -1779,11 +1779,29 @@ static void pci_del_option_rom(PCIDevice *pdev)* in pci config space */ int pci_add_capability (PCIDevice *pdev,uint8_t cap_id,uint8_t offset,uint8_t size) { -uint8_t *config; +uint8_t *config, existing;Existing is a pointer to the target dev's config space, right?
Yes.
Quoted 10 lines
Quoted 8 lines
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1816
Quoted 8 lines
hw/pci.c +6
-0int i, overlapping_cap; + existing =pci_find_capability (pdev, cap_id); +if (existing) { +if (offset && (existing != offset)) { +return -EEXIST ; + } +for (i = existing; i < size; ++i) {So how does this possibly make sense?
Although I do not expect VFIO to add capabilities (does not make sense), I still want to double check that this space has not been tried to use by someone else.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
i is an int. existing is a uint8_t*.
It was there before me. This function already does a loop and this is how it was coded at the first place.
Ugh. Existing is a
uint8_t, not a pointer. Gotta love C syntax...
Well it is still does not make much sense to have "int i" rather than "uint8_t i" :)
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 5 lines
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1821
Quoted 5 lines
hw/pci.c +5
-0+if (pdev->used[i]) { +return -EFAULT ; + } + } +memset (pdev->used + offset,0xFF , size);Why?
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Because I am marking the space this capability takes as used.
But if it already existed (at the same offset), it should be set used already, no? Unless size > existing size, in which case you might overwrite data in the next chunk, no?
No, it does not exist for VFIO - VFIO read the config space from the host kernel first and then calls
msi_initormsix_initorwhatever_else_initdepending on what it got from the host kernel. And thesexxx_init()functions eventually callpci_add_capability().So why would the function that populates the config space initially not set the used flag correctly?
This is internal kitchen of PCIDevice which I do not want to touch from anywhere but pci.c. And there is no "fixup_capability" or something.
Quoted 2 lines
Quoted 1 line
Sure we can either implement own msi_init/msix_init (and may be others in the future) for VFIO (which would do all the same as other QEMU devices except touching the capabilities) OR hack msi_init/msix_init not to touch capabilities if they exist.
No, calling the internal functions sounds fine to me. It's the step before that irritates me. VFIO shouldn't differ too much from an emulated device wrt its config space population really.
The last thing we want for a VFIO device is changing its capabilities list.
Quoted 11 lines
Quoted 9 lines
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Quoted 2 lines
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1823
Quoted 2 lines
hw/pci.c +2
-0+/* Make capability read-only by default */ +memset (pdev->wmask + offset,0 , size);Why?
Because the
pci_add_capability()does it for a new capability by default.Hrm. So you're copying code? Can't you merge the overwrite and write cases?
I am trying to make it as a single chunk which is as small as possible.
No, you're needlessly duplicating code which is a bad idea :). Please reuse as much of the existing function as possible, unless it really doesn't make sense.
I actually duplicated 4 (four) lines and did it just once. This is too little to be called "duplicating" :) And I get very special case visually separated and easy to remove if we find a better solution later.
But - no problemo, I'll rework it.
[no further comments]
Quoted 243 lines
Quoted 55 lines
If it helps, below is the same patch with extended context to see what is going on in that function.
hw/pci.c| 20 +++++++++++++++++++-1 files changed, 19 insertions(+), 1 deletions(-)hw/pci.c +1
-1@@ -1772,75 +1772,93 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) ptr =memory_region_get_ram_ptr (&pdev->rom);load_image (path, ptr);g_free (path);if (is_default_rom) {/* Only the default rom images will be patched (if needed). */ pci_patch_ids (pdev, ptr, size); }qemu_put_ram_ptr (ptr);pci_register_bar (pdev,PCI_ROM_SLOT ,0 , &pdev->rom);return 0 ; }static void pci_del_option_rom (PCIDevice *pdev) {if (!pdev->has_rom)return ;vmstate_unregister_ram (&pdev->rom, &pdev->qdev);memory_region_destroy (&pdev->rom); pdev->has_rom = false; }/* * if !offset * Reserve space and add capability to the linked list in pci config space * * if offset = 0, * Find and reserve space and add capability to the linked list * in pci config space */ int pci_add_capability (PCIDevice *pdev,uint8_t cap_id,uint8_t offset,uint8_t size) { -uint8_t *config; +uint8_t *config, existing;int i, overlapping_cap;hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1820
Quoted 10 lines
hw/pci.c +10
-0+ existing =pci_find_capability (pdev, cap_id); +if (existing) { +if (offset && (existing != offset)) { +return -EEXIST ; + } +for (i = existing; i < size; ++i) { +if (pdev->used[i]) { +return -EFAULT ; + } + }}
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1829
Quoted 9 lines
hw/pci.c +8
-0+memset (pdev->used + offset,0xFF , size); +/* Make capability read-only by default */ +memset (pdev->wmask + offset,0 , size); +/* Check capability by default */ +memset (pdev->cmask + offset,0xFF , size); +return existing; + } +if (!offset) {&& !existing maybe?
Quoted 13 lines
hw/pci.c +0
-0offset =pci_find_space (pdev, size);if (!offset) {return -ENOSPC ; } }else {/* Verify that capabilities don't overlap. Note: device assignment * depends on this check to verify that the device is not broken. * Should never trigger for emulated devices, but it's helpful * for debugging these. */ for (i = offset; i < offset + size; i++) { overlapping_cap =pci_find_capability_at_offset (pdev, i);if (overlapping_cap) { fprintf(stderr, "ERROR: %04x:%02x:%02x.%x "hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1852
Quoted 10 lines
hw/pci.c +0
-0"Attempt to add PCI capability %x at offset " "%x overlaps existing capability %x at offset %x\n" ,pci_find_domain (pdev->bus),pci_bus_num (pdev->bus),PCI_SLOT (pdev->devfn),PCI_FUNC (pdev->devfn), cap_id, offset, overlapping_cap, i);return -EINVAL ; } } }If (!existing) {
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1857
Quoted 5 lines
hw/pci.c +0
-0config = pdev->config + offset; config[PCI_CAP_LIST_ID ] = cap_id; config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; pdev->config[PCI_CAPABILITY_LIST ] = offset; pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ;}
which poses the question why the above wouldn't apply for the existing case. It would work just as well to leave that in, no?
Alex
Quoted 125 lines
hw/pci.c +0
-0memset (pdev->used + offset,0xFF , size);/* Make capability read-only by default */ memset (pdev->wmask + offset,0 , size);/* Check capability by default */ memset (pdev->cmask + offset,0xFF , size);return offset; }Quoted 118 lines
Quoted 118 lines
Quoted 8 lines
Quoted 6 lines
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1825
Quoted 6 lines
hw/pci.c +2
-0+/* Check capability by default */ +memset (pdev->cmask + offset,0xFF , size);I don't understand this part either.
The
pci_add_capability()does it for a new capability by default.Quoted 105 lines
Alex
Quoted 102 lines
Quoted 102 lines
hw/pci.c +3
-0+return existing; + } +if (!offset) { offset =pci_find_space (pdev, size);if (!offset) {return -ENOSPC ;Quoted 88 lines · Alexey Kardashevskiy
Quoted 60 lines · Alexander Graf
Quoted 53 lines · Alexey Kardashevskiy
11.05.2012 20:52, Alexander Graf =0?8A0;:
Quoted 52 lines · Alexey Kardashevskiy
Quoted 38 lines
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.In the case of VFIO the capability may already exist and adding new capability into the beginning of the linked list may create a loop.
For example, the old code destroys the following config of PCIe Intel E1000E:
before adding
PCI_CAP_ID_MSI(0x05):0x34:0xC80xC8:0x010xD00xD0:0x050xE00xE0:0x100x00after:
0x34:0xD00xC8:0x010xD00xD0:0x050xC80xE0:0x100x00As result capabilities
0x01and0x05point to each other.The proposed patch does not change capability pointers when the same type capability is about to add.
Signed-off-by Alexey Kardashevskiy <[email protected]>---
hw/pci.c| 10 ++++++----1 files changed, 6 insertions(+), 4 deletions(-)hw/pci.c +0
-0hw/pci.c int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, · line 1797
Quoted 8 lines
hw/pci.c +1
-4@@ -1794,10 +1794,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, } config = pdev->config + offset; - config[PCI_CAP_LIST_ID ] = cap_id; - config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; - pdev->config[PCI_CAPABILITY_LIST ] = offset; - pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; +if (config[PCI_CAP_LIST_ID ] != cap_id) {This doesn't scale. Capabilities are a list of CAPs. You'll have to do a loop through all capabilities, check if the one you want to add is there already and if so either
* replace the existing one or
* drop out and not write the new one in.*
hw_error:)Quoted 1 line
Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
I'm not sure which way would be more natural.
There is a third option - add another function, lets call it
pci_fixup_capability()which would do whateverpci_add_capability()does but won't touch list pointers.What good is a function that breaks internal consistency?
It is broken already by having PCIDevice.used field. Normally
pci_add_capability()would go through the whole list and add a capability if it does not exist. Emulated devices which care about having a capability at some fixed offset would have initialized their config space before calling this capabilities API (as VFIO does).If we really want to support emulated devices which want some capabilities be at fixed offset and others at random offsets (strange, but ok), I do not see how it is bad to restore this consistency by special function (
pci_fixup_capability()) to avoid its rewriting at different location as a guest driver may care about its offset.Quoted 6 lines
Quoted 4 lines
When vfio,
pci_add_capability()is called from the code which knows exactly that the capability exists and where it is and it callspci_add_capability()based on this knowledge so doing additional loops just for imaginery scalability is a bit weird, no?Not sure I understand your proposal. The more generic a framework is, the better, no? In this code path we don't care about speed. We only care about consistency and reliability.
Signature · 1 line
Alexey
Quoted 88 lines · Alexey Kardashevskiy
Quoted 64 lines · Alexander Graf
Quoted 60 lines · Alexey Kardashevskiy
Quoted 59 lines · Alexander Graf
Quoted 54 lines · Alexey Kardashevskiy
Quoted 28 lines · Alexander Graf
Quoted 24 lines · Benjamin Herrenschmidt
Quoted 3 lines · Alexey Kardashevskiy
Alexander,
Is that any better? :)
Alex (Graf that is), ping ?
The original patch from Alexey was fine btw.
VFIO will always call things with the existing capability offset so there's no real risk of doing the wrong thing or break the list or anything.
IE. A small simple patch that addresses the problem :-)
The new patch is a bit more "robust" I believe, I don't think we need to go too far to fix a problem we don't have. But we need a fix for the real issue and the simple patch does it neatly from what I can understand.
Cheers,
Ben.Quoted 1 line
Quoted 9 lines
Quoted 7 lines
hw/pci.c static void pci_del_option_rom(PCIDevice *pdev) · line 1783
Quoted 7 lines
hw/pci.c +1
-1@@ -1779,11 +1779,29 @@ static void pci_del_option_rom(PCIDevice *pdev)* in pci config space */ int pci_add_capability (PCIDevice *pdev,uint8_t cap_id,uint8_t offset,uint8_t size) { -uint8_t *config; +uint8_t *config, existing;Existing is a pointer to the target dev's config space, right?
Yes.
Quoted 10 lines
Quoted 8 lines
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1816
Quoted 8 lines
hw/pci.c +6
-0int i, overlapping_cap; + existing =pci_find_capability (pdev, cap_id); +if (existing) { +if (offset && (existing != offset)) { +return -EEXIST ; + } +for (i = existing; i < size; ++i) {So how does this possibly make sense?
Although I do not expect VFIO to add capabilities (does not make sense), I still want to double check that this space has not been tried to use by someone else.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
i is an int. existing is a uint8_t*.
It was there before me. This function already does a loop and this is how it was coded at the first place.
Ugh. Existing is a
uint8_t, not a pointer. Gotta love C syntax...Well it is still does not make much sense to have "int i" rather than "
uint8_ti" :)Quoted 12 lines
Quoted 12 lines
Quoted 10 lines
Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1821
Quoted 5 lines
hw/pci.c +5
-0+if (pdev->used[i]) { +return -EFAULT ; + } + } +memset (pdev->used + offset,0xFF , size);Why?
Because I am marking the space this capability takes as used.
But if it already existed (at the same offset), it should be set used already, no? Unless size > existing size, in which case you might overwrite data in the next chunk, no?
Quoted 6 lines
Quoted 2 lines
Quoted 1 line
No, it does not exist for VFIO - VFIO read the config space from the host kernel first and then calls
msi_initormsix_initorwhatever_else_initdepending on what it got from the host kernel. And thesexxx_init()functions eventually callpci_add_capability().So why would the function that populates the config space initially not set the used flag correctly?
This is internal kitchen of PCIDevice which I do not want to touch from anywhere but
pci.c. And there is no "fixup_capability" or something.
Hrm. Maybe we should have one? :) Or instead of populating the config space with the exact data from the host device, loop through the host device capabilities and populate them using this function as we go. That should maintain the offsets, but ensure that all internal flags are set, no?
Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
Sure we can either implement own msi_init/msix_init (and may be others in the future) for VFIO (which would do all the same as other QEMU devices except touching the capabilities) OR hack msi_init/msix_init not to touch capabilities if they exist.
No, calling the internal functions sounds fine to me. It's the step before that irritates me. VFIO shouldn't differ too much from an emulated device wrt its config space population really.
The last thing we want for a VFIO device is changing its capabilities list.
Well - we want it to look the same. The population should go through the same methods as emulated devices have to go through, no? :)
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
Quoted 7 lines
Quoted 5 lines
Quoted 4 lines
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1823
Quoted 4 lines
hw/pci.c +2
-0+/* Make capability read-only by default */ +memset (pdev->wmask + offset,0 , size);Why?
Because the
pci_add_capability()does it for a new capability by default.Hrm. So you're copying code? Can't you merge the overwrite and write cases?
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
I am trying to make it as a single chunk which is as small as possible.
No, you're needlessly duplicating code which is a bad idea :). Please reuse as much of the existing function as possible, unless it really doesn't make sense.
I actually duplicated 4 (four) lines and did it just once. This is too little to be called "duplicating" :) And I get very special case visually separated and easy to remove if we find a better solution later.
Yeah, but the special case really shouldn't be all that special - that's why I was irritated.
Quoted 1 line
But - no problemo, I'll rework it.
Thanks!
Quoted 1 line
[no further comments]
What about mine below? ;)
Alex
Quoted 252 lines
Quoted 246 lines
Quoted 58 lines
If it helps, below is the same patch with extended context to see what is going on in that function.
hw/pci.c| 20 +++++++++++++++++++-1 files changed, 19 insertions(+), 1 deletions(-)hw/pci.c +1
-1@@ -1772,75 +1772,93 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) ptr =memory_region_get_ram_ptr (&pdev->rom);load_image (path, ptr);g_free (path);if (is_default_rom) {/* Only the default rom images will be patched (if needed). */ pci_patch_ids (pdev, ptr, size); }qemu_put_ram_ptr (ptr);pci_register_bar (pdev,PCI_ROM_SLOT ,0 , &pdev->rom);return 0 ; }static void pci_del_option_rom (PCIDevice *pdev) {if (!pdev->has_rom)return ;vmstate_unregister_ram (&pdev->rom, &pdev->qdev);memory_region_destroy (&pdev->rom); pdev->has_rom = false; }/* * if !offset * Reserve space and add capability to the linked list in pci config space * * if offset = 0, * Find and reserve space and add capability to the linked list * in pci config space */ int pci_add_capability (PCIDevice *pdev,uint8_t cap_id,uint8_t offset,uint8_t size) { -uint8_t *config; +uint8_t *config, existing;int i, overlapping_cap;hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1820
Quoted 10 lines
hw/pci.c +10
-0+ existing =pci_find_capability (pdev, cap_id); +if (existing) { +if (offset && (existing != offset)) { +return -EEXIST ; + } +for (i = existing; i < size; ++i) { +if (pdev->used[i]) { +return -EFAULT ; + } + }}
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1829
Quoted 9 lines
hw/pci.c +8
-0+memset (pdev->used + offset,0xFF , size); +/* Make capability read-only by default */ +memset (pdev->wmask + offset,0 , size); +/* Check capability by default */ +memset (pdev->cmask + offset,0xFF , size); +return existing; + } +if (!offset) {&& !existing maybe?
Quoted 13 lines
hw/pci.c +0
-0offset =pci_find_space (pdev, size);if (!offset) {return -ENOSPC ; } }else {/* Verify that capabilities don't overlap. Note: device assignment * depends on this check to verify that the device is not broken. * Should never trigger for emulated devices, but it's helpful * for debugging these. */ for (i = offset; i < offset + size; i++) { overlapping_cap =pci_find_capability_at_offset (pdev, i);if (overlapping_cap) { fprintf(stderr, "ERROR: %04x:%02x:%02x.%x "hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1852
Quoted 10 lines
hw/pci.c +0
-0"Attempt to add PCI capability %x at offset " "%x overlaps existing capability %x at offset %x\n" ,pci_find_domain (pdev->bus),pci_bus_num (pdev->bus),PCI_SLOT (pdev->devfn),PCI_FUNC (pdev->devfn), cap_id, offset, overlapping_cap, i);return -EINVAL ; } } }If (!existing) {
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1857
Quoted 5 lines
hw/pci.c +0
-0config = pdev->config + offset; config[PCI_CAP_LIST_ID ] = cap_id; config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; pdev->config[PCI_CAPABILITY_LIST ] = offset; pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ;}
which poses the question why the above wouldn't apply for the existing case. It would work just as well to leave that in, no?
Alex
Quoted 125 lines
hw/pci.c +0
-0memset (pdev->used + offset,0xFF , size);/* Make capability read-only by default */ memset (pdev->wmask + offset,0 , size);/* Check capability by default */ memset (pdev->cmask + offset,0xFF , size);return offset; }Quoted 118 lines
Quoted 118 lines
Quoted 8 lines
Quoted 6 lines
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1825
Quoted 6 lines
hw/pci.c +2
-0+/* Check capability by default */ +memset (pdev->cmask + offset,0xFF , size);I don't understand this part either.
The
pci_add_capability()does it for a new capability by default.Quoted 105 lines
Alex
Quoted 102 lines
Quoted 102 lines
hw/pci.c +3
-0+return existing; + } +if (!offset) { offset =pci_find_space (pdev, size);if (!offset) {return -ENOSPC ;Quoted 88 lines · Alexey Kardashevskiy
Quoted 60 lines · Alexander Graf
Quoted 53 lines · Alexey Kardashevskiy
11.05.2012 20:52, Alexander Graf =0?8A0;:
Quoted 52 lines · Alexey Kardashevskiy
Quoted 46 lines
Normally the pci_add_capability is called on devices to add new
capability. This is ok for emulated devices which capabilities list
is being built by QEMU.In the case of VFIO the capability may already exist and adding new
capability into the beginning of the linked list may create a loop.For example, the old code destroys the following config
of PCIe Intel E1000E:before adding PCI_CAP_ID_MSI (0x05):
0x34: 0xC8
0xC8: 0x01 0xD0
0xD0: 0x05 0xE0
0xE0: 0x10 0x00after:
0x34: 0xD0
0xC8: 0x01 0xD0
0xD0: 0x05 0xC8
0xE0: 0x10 0x00As result capabilities 0x01 and 0x05 point to each other.
The proposed patch does not change capability pointers when
the same type capability is about to add.Signed-off-by: Alexey Kardashevskiy <[email protected]>
---
hw/pci.c | 10 ++++++----
1 files changed, 6 insertions(+), 4 deletions(-)diff --git a/hw/pci.c b/hw/pci.c
index aa0c0b8..1f7c924 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1794,10 +1794,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
}config = pdev->config + offset;
- config[PCI_CAP_LIST_ID] = cap_id;
- config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
- pdev->config[PCI_CAPABILITY_LIST] = offset;
- pdev->config[PCI_STATUS] |= PCI_STATUS_CAP_LIST;
+ if (config[PCI_CAP_LIST_ID] != cap_id) {This doesn't scale. Capabilities are a list of CAPs. You'll have to do a loop through all capabilities, check if the one you want to add is there already and if so either
* replace the existing one or
* drop out and not write the new one in.*
hw_error:)Quoted 1 line
Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
I'm not sure which way would be more natural.
There is a third option - add another function, lets call it
pci_fixup_capability()which would do whateverpci_add_capability()does but won't touch list pointers.What good is a function that breaks internal consistency?
It is broken already by having PCIDevice.used field. Normally
pci_add_capability()would go through the whole list and add a capability if it does not exist. Emulated devices which care about having a capability at some fixed offset would have initialized their config space before calling this capabilities API (as VFIO does).If we really want to support emulated devices which want some capabilities be at fixed offset and others at random offsets (strange, but ok), I do not see how it is bad to restore this consistency by special function (
pci_fixup_capability()) to avoid its rewriting at different location as a guest driver may care about its offset.Quoted 6 lines
Quoted 4 lines
When vfio,
pci_add_capability()is called from the code which knows exactly that the capability exists and where it is and it callspci_add_capability()based on this knowledge so doing additional loops just for imaginery scalability is a bit weird, no?Not sure I understand your proposal. The more generic a framework is, the better, no? In this code path we don't care about speed. We only care about consistency and reliability.
Signature · 1 line
Alexey
Quoted 4 lines · Alexander Graf
Quoted 1 line · Alexey Kardashevskiy
This is internal kitchen of PCIDevice which I do not want to touch
from anywhere but
pci.c. And
Quoted 8 lines
Quoted 1 line
there is no "
fixup_capability" or something.Hrm. Maybe we should have one? :) Or instead of populating the config space with the exact data from the host device,
loop through the host device capabilities and populate them using this function as we go.
That should maintain the offsets, but ensure that all internal flags
are set, no?
That actually sounds reasonable, though it might be more efficient to have something like pci_parse_config() or similar called once with a pre-cooked config space.
Internally inside pci.c it can do that same loop you mention. The advantage is that it can also do whatever else we might need in the future. If for some reason, something wants to cache a cap pointer it can be done there, whatever else that is normally initialized as fields to generate the config space can be initialized by reading the config space and then initializing the fields etc... from that one function.
Cheers,
Ben.
Yet another try :)
Normally the pci_add_capability is called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.
In the case of VFIO the capability may already exist and adding new capability into the beginning of the linked list may create a loop as the existing code ignores capabilities which point to the capability being added.
For example, the old code destroys the following config of PCIe Intel E1000E:
before adding PCI_CAP_ID_MSI (0x05) at offset 0xD0:0x34: 0xC80xC8: 0x01 0xD00xD0: 0x05 0xE00xE0: 0x10 0x00
after:0x34: 0xD00xC8: 0x01 0xD00xD0: 0x05 0xC80xE0: 0x10 0x00
As result capabilities at 0xC8 and 0xD0 point to each other.
--- hw/pci.c | 25 ++++++++++++++++++++----- 1 files changed, 20 insertions(+), 5 deletions(-)
hw/pci.c +20 -5
@@ -1791,61 +1791,76 @@ static void pci_del_option_rom(PCIDevice *pdev)return ;vmstate_unregister_ram (&pdev->rom, &pdev->qdev);memory_region_destroy (&pdev->rom); pdev->has_rom = false; }/* * if !offset * Reserve space and add capability to the linked list in pci config space * * if offset = 0, * Find and reserve space and add capability to the linked list -* in pci config space */ +* in pci config space +* Also does not change the capability list pointers +* if a capability already exists (actual for device with pre-cooked config +* space such as VFIO) +*/ int pci_add_capability (PCIDevice *pdev,uint8_t cap_id,uint8_t offset,uint8_t size) {uint8_t *config;int i, overlapping_cap; +uint8_t existing_offset; + + existing_offset =pci_find_capability (pdev, cap_id); +if (existing_offset) { +if (offset && (existing_offset != offset)) { +return -EEXIST ; + } + offset = existing_offset; + }if (!offset) { offset =pci_find_space (pdev, size);if (!offset) {return -ENOSPC ; } }else {/* Verify that capabilities don't overlap. Note: device assignment * depends on this check to verify that the device is not broken. * Should never trigger for emulated devices, but it's helpful * for debugging these. */ for (i = offset; i < offset + size; i++) { overlapping_cap =pci_find_capability_at_offset (pdev, i);if (overlapping_cap) {fprintf (stderr,"ERROR: %04x:%02x:%02x.%x " "Attempt to add PCI capability %x at offset " "%x overlaps existing capability %x at offset %x\n" ,pci_find_domain (pdev->bus),pci_bus_num (pdev->bus),PCI_SLOT (pdev->devfn),PCI_FUNC (pdev->devfn), cap_id, offset, overlapping_cap, i);return -EINVAL ; } } } - config = pdev->config + offset; - config[PCI_CAP_LIST_ID ] = cap_id; - config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; - pdev->config[PCI_CAPABILITY_LIST ] = offset; +if (!existing_offset) { + config = pdev->config + offset; + config[PCI_CAP_LIST_ID ] = cap_id; + config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; + pdev->config[PCI_CAPABILITY_LIST ] = offset; + } pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ;memset (pdev->used + offset,0xFF , size);/* Make capability read-only by default */ memset (pdev->wmask + offset,0 , size);/* Check capability by default */ memset (pdev->cmask + offset,0xFF , size);return offset; }/* Unlink capability from the pci config space. */ void pci_del_capability (PCIDevice *pdev,uint8_t cap_id,uint8_t size) {uint8_t prev, offset =pci_find_capability_list (pdev, cap_id, &prev);
Signature · 1 line
1.7.7.3
Quoted 354 lines · Alexander Graf
Quoted 57 lines · Alexey Kardashevskiy
Quoted 56 lines · Alexander Graf
Quoted 52 lines · Alexey Kardashevskiy
Quoted 28 lines · Alexander Graf
Quoted 24 lines · Benjamin Herrenschmidt
Quoted 3 lines · Alexey Kardashevskiy
Alexander,
Is that any better? :)
Alex (Graf that is), ping ?
The original patch from Alexey was fine btw.
VFIO will always call things with the existing capability offset so there's no real risk of doing the wrong thing or break the list or anything.
IE. A small simple patch that addresses the problem :-)
The new patch is a bit more "robust" I believe, I don't think we need to go too far to fix a problem we don't have. But we need a fix for the real issue and the simple patch does it neatly from what I can understand.
Cheers,
Ben.Quoted 1 line
Quoted 9 lines
Quoted 7 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1783
Quoted 7 lines
(quoted diff) +1
-1@@ -1779,11 +1779,29 @@ static void pci_del_option_rom(PCIDevice *pdev) * in pci config space */ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t offset, uint8_t size) { - uint8_t *config; + uint8_t *config, existing;Existing is a pointer to the target dev's config space, right?
Yes.
Quoted 10 lines
Quoted 8 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1791
Quoted 8 lines
(quoted diff) +6
-0int i, overlapping_cap; + existing = pci_find_capability(pdev, cap_id); + if (existing) { + if (offset && (existing != offset)) { + return -EEXIST; + } + for (i = existing; i < size; ++i) {So how does this possibly make sense?
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Although I do not expect VFIO to add capabilities (does not make sense), I still want to double check that this space has not been tried to use by someone else.
i is an int. existing is a uint8_t*.
It was there before me. This function already does a loop and this is how it was coded at the first place.
Ugh. Existing is a
uint8_t, not a pointer. Gotta love C syntax...Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
(quoted diff) +5
-0+ if (pdev->used[i]) { + return -EFAULT; + } + } + memset(pdev->used + offset, 0xFF, size);Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Why?
Because I am marking the space this capability takes as used.
But if it already existed (at the same offset), it should be set used already, no? Unless size > existing size, in which case you might overwrite data in the next chunk, no?
No, it does not exist for VFIO - VFIO read the config space from the host kernel first and then calls
msi_initormsix_initorwhatever_else_initdepending on what it got from the host kernel. And thesexxx_init()functions eventually callpci_add_capability().So why would the function that populates the config space initially not set the used flag correctly?
Quoted 2 lines
Sure we can either implement own msi_init/msix_init (and may be others in the future) for VFIO (which would do all the same as other QEMU devices except touching the capabilities) OR hack msi_init/msix_init not to touch capabilities if they exist.
No, calling the internal functions sounds fine to me. It's the step before that irritates me. VFIO shouldn't differ too much from an emulated device wrt its config space population really.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
(quoted diff) +2
-0+ /* Make capability read-only by default */ + memset(pdev->wmask + offset, 0, size);Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Why?
Because the
pci_add_capability()does it for a new capability by default.Hrm. So you're copying code? Can't you merge the overwrite and write cases?
I am trying to make it as a single chunk which is as small as possible.
No, you're needlessly duplicating code which is a bad idea :). Please reuse as much of the existing function as possible, unless it really doesn't make sense.
Quoted 57 lines
If it helps, below is the same patch with extended context to see what is going on in that function.
hw/pci.c| 20 +++++++++++++++++++-1 files changed, 19 insertions(+), 1 deletions(-)hw/pci.c +1
-1@@ -1772,75 +1772,93 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) ptr =memory_region_get_ram_ptr (&pdev->rom);load_image (path, ptr);g_free (path);if (is_default_rom) {/* Only the default rom images will be patched (if needed). */ pci_patch_ids (pdev, ptr, size); }qemu_put_ram_ptr (ptr);pci_register_bar (pdev,PCI_ROM_SLOT ,0 , &pdev->rom);return 0 ; }static void pci_del_option_rom (PCIDevice *pdev) {if (!pdev->has_rom)return ;vmstate_unregister_ram (&pdev->rom, &pdev->qdev);memory_region_destroy (&pdev->rom); pdev->has_rom = false; }/* * if !offset * Reserve space and add capability to the linked list in pci config space * * if offset = 0, * Find and reserve space and add capability to the linked list * in pci config space */ int pci_add_capability (PCIDevice *pdev,uint8_t cap_id,uint8_t offset,uint8_t size) { -uint8_t *config; +uint8_t *config, existing;int i, overlapping_cap;hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1820
Quoted 10 lines
hw/pci.c +10
-0+ existing =pci_find_capability (pdev, cap_id); +if (existing) { +if (offset && (existing != offset)) { +return -EEXIST ; + } +for (i = existing; i < size; ++i) { +if (pdev->used[i]) { +return -EFAULT ; + } + }}
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1829
Quoted 9 lines
hw/pci.c +8
-0+memset (pdev->used + offset,0xFF , size); +/* Make capability read-only by default */ +memset (pdev->wmask + offset,0 , size); +/* Check capability by default */ +memset (pdev->cmask + offset,0xFF , size); +return existing; + } +if (!offset) {&& !existing maybe?
Quoted 13 lines
hw/pci.c +0
-0offset =pci_find_space (pdev, size);if (!offset) {return -ENOSPC ; } }else {/* Verify that capabilities don't overlap. Note: device assignment * depends on this check to verify that the device is not broken. * Should never trigger for emulated devices, but it's helpful * for debugging these. */ for (i = offset; i < offset + size; i++) { overlapping_cap =pci_find_capability_at_offset (pdev, i);if (overlapping_cap) { fprintf(stderr, "ERROR: %04x:%02x:%02x.%x "hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1852
Quoted 10 lines
hw/pci.c +0
-0"Attempt to add PCI capability %x at offset " "%x overlaps existing capability %x at offset %x\n" ,pci_find_domain (pdev->bus),pci_bus_num (pdev->bus),PCI_SLOT (pdev->devfn),PCI_FUNC (pdev->devfn), cap_id, offset, overlapping_cap, i);return -EINVAL ; } } }If (!existing) {
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1857
Quoted 5 lines
hw/pci.c +0
-0config = pdev->config + offset; config[PCI_CAP_LIST_ID ] = cap_id; config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; pdev->config[PCI_CAPABILITY_LIST ] = offset; pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ;}
which poses the question why the above wouldn't apply for the existing case. It would work just as well to leave that in, no?
Alex
Quoted 125 lines
hw/pci.c +0
-0memset (pdev->used + offset,0xFF , size);/* Make capability read-only by default */ memset (pdev->wmask + offset,0 , size);/* Check capability by default */ memset (pdev->cmask + offset,0xFF , size);return offset; }Quoted 118 lines
Quoted 118 lines
Quoted 8 lines
Quoted 6 lines
Review static void pci_del_option_rom(PCIDevice *pdev) · line 1800
Quoted 6 lines
(quoted diff) +2
-0+ /* Check capability by default */ + memset(pdev->cmask + offset, 0xFF, size);I don't understand this part either.
The
pci_add_capability()does it for a new capability by default.Quoted 105 lines
Alex
Quoted 102 lines
Quoted 102 lines
(quoted diff) +3
-0+ return existing; + } + if (!offset) { offset = pci_find_space(pdev, size); if (!offset) { return -ENOSPC;Quoted 88 lines · Alexey Kardashevskiy
Quoted 60 lines · Alexander Graf
Quoted 53 lines · Alexey Kardashevskiy
11.05.2012 20:52, Alexander Graf =0?8A0;:
Quoted 52 lines · Alexey Kardashevskiy
Quoted 38 lines
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.In the case of VFIO the capability may already exist and adding new capability into the beginning of the linked list may create a loop.
For example, the old code destroys the following config of PCIe Intel E1000E:
before adding
PCI_CAP_ID_MSI(0x05):0x34:0xC80xC8:0x010xD00xD0:0x050xE00xE0:0x100x00after:
0x34:0xD00xC8:0x010xD00xD0:0x050xC80xE0:0x100x00As result capabilities
0x01and0x05point to each other.The proposed patch does not change capability pointers when the same type capability is about to add.
Signed-off-by Alexey Kardashevskiy <[email protected]>---
hw/pci.c| 10 ++++++----1 files changed, 6 insertions(+), 4 deletions(-)hw/pci.c +0
-0hw/pci.c int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, · line 1797
Quoted 8 lines
hw/pci.c +1
-4@@ -1794,10 +1794,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, } config = pdev->config + offset; - config[PCI_CAP_LIST_ID ] = cap_id; - config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; - pdev->config[PCI_CAPABILITY_LIST ] = offset; - pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; +if (config[PCI_CAP_LIST_ID ] != cap_id) {This doesn't scale. Capabilities are a list of CAPs. You'll have to do a loop through all capabilities, check if the one you want to add is there already and if so either
* replace the existing one or
* drop out and not write the new one in.*
hw_error:)Quoted 1 line
Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
I'm not sure which way would be more natural.
There is a third option - add another function, lets call it
pci_fixup_capability()which would do whateverpci_add_capability()does but won't touch list pointers.What good is a function that breaks internal consistency?
It is broken already by having PCIDevice.used field. Normally
pci_add_capability()would go through the whole list and add a capability if it does not exist. Emulated devices which care about having a capability at some fixed offset would have initialized their config space before calling this capabilities API (as VFIO does).If we really want to support emulated devices which want some capabilities be at fixed offset and others at random offsets (strange, but ok), I do not see how it is bad to restore this consistency by special function (
pci_fixup_capability()) to avoid its rewriting at different location as a guest driver may care about its offset.Quoted 6 lines
Quoted 4 lines
When vfio,
pci_add_capability()is called from the code which knows exactly that the capability exists and where it is and it callspci_add_capability()based on this knowledge so doing additional loops just for imaginery scalability is a bit weird, no?Not sure I understand your proposal. The more generic a framework is, the better, no? In this code path we don't care about speed. We only care about consistency and reliability.
Signature · 1 line
Alexey
Quoted 7 lines · Alexey Kardashevskiy
Yet another try :)
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.In the case of VFIO the capability may already exist and adding new
Why does it exit? VFIO should build the virtual capability list from scratch (just like classic device assignment does), recreating the layout of the physical device (except for masked out caps). In that case, this conflict should become impossible, no?
But if pci_*add*_capability should actually be used like this (I doubt this), some renaming would be required. "Add" sound like "append" to me, not "update".
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
08.06.2012 20:56, Jan Kiszka написал:
Quoted 7 lines · Alexey Kardashevskiy
Quoted 6 lines
Yet another try :)
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.
Quoted 6 lines
Quoted 1 line
In the case of VFIO the capability may already exist and adding new
Why does it exit? VFIO should build the virtual capability list from scratch (just like classic device assignment does), recreating the layout of the physical device (except for masked out caps). In that case, this conflict should become impossible, no?
Normally capabilities in emulated devices are created by calling msi_init or msix_init - just when emulated device wants to advertise it to the guest.
In the case of VFIO, there is a lot of capabilities which QEMU does not know and does not want to know about. They are read from the host kernel as is. And we definitely want to pass these capabilities to the guest as is, i.e. on the same position and the same number of them. Just for some we call pci_add_capability (indirectly!) if we want QEMU to support them somehow.
If we invent some function which "readds" all the capabilities we got from the host to keep internal QEMU's PCIDevice data in sync, then we'll need to change every piece of code which adds capabilities. I noticed, this is very common approach here to change a lot for a very small thing or rare case but I'd like to avoid this :)
Quoted 2 lines
But if pci_*add*_capability should actually be used like this (I doubt this),
MSI/MSIX use it. To enable MSI/MSIX on VFIO PCIDevice, we call msi_init/msix_init and they call pci_add_capability.
Quoted 2 lines
some renaming would be required. "Add" sound like "append" to me, not "update".
It is "add" for all the cases but VFIO. VFIO is the very special case and I do not see another one doing the same soon.
Signature · 3 lines
With best regards Alexey Kardashevskiy -- icq: 52150396
Quoted 26 lines · Alexey Kardashevskiy
08.06.2012 20:56, Jan Kiszka написал:
Quoted 7 lines · Alexey Kardashevskiy
Quoted 6 lines
Yet another try :)
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.Quoted 6 lines
Quoted 1 line
In the case of VFIO the capability may already exist and adding new
Why does it exit? VFIO should build the virtual capability list from scratch (just like classic device assignment does), recreating the layout of the physical device (except for masked out caps). In that case, this conflict should become impossible, no?
Normally capabilities in emulated devices are created by calling
msi_initormsix_init- just when emulated device wants to advertise it to the guest.In the case of VFIO, there is a lot of capabilities which QEMU does not know and does not want to know about. They are read from the host kernel as is. And we definitely want to pass these capabilities to the guest as is, i.e. on the same position and the same number of them. Just for some we call
pci_add_capability(indirectly!) if we want QEMU to support them somehow.
Quoted 3 lines
If we invent some function which "readds" all the capabilities we got from the host to keep internal QEMU's PCIDevice data in sync, then we'll need to change every piece of code which adds capabilities.
I can't follow. What is different in VFIO from device-assignment.c, assigned_device_pci_cap_init (except that it already uses msi[x]_init, something we need to fix in device-assignment.c)?
Quoted 9 lines
I noticed,
this is very common approach here to change a lot for a very small thing or rare case but I'd like to avoid this :)Quoted 2 lines
But if pci_*add*_capability should actually be used like this (I doubt this),
MSI/MSIX use it. To enable MSI/MSIX on VFIO PCIDevice, we call msi_init/msix_init and they call
pci_add_capability.
You can't blame msi_init/msix_init for the fact that VFIO creates a capability list with an existing MSI/MSI-X entry beforehand.
Quoted 6 lines
Quoted 3 lines
some renaming would be required. "Add" sound like "append" to me, not "update".
It is "add" for all the cases but VFIO. VFIO is the very special case and I do not see another one doing the same soon.
PCI device assignment may have some special requirements. Then it is either required to generalize common services properly or keep the specialty local. So far, this proposal does not fall in any of those two categories.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
08.06.2012 21:30, Jan Kiszka пишет:
Quoted 27 lines · Alexey Kardashevskiy
Quoted 26 lines
08.06.2012 20:56, Jan Kiszka написал:
Quoted 7 lines · Alexey Kardashevskiy
Quoted 6 lines
Yet another try :)
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.Quoted 6 lines
Quoted 1 line
In the case of VFIO the capability may already exist and adding new
Why does it exit? VFIO should build the virtual capability list from scratch (just like classic device assignment does), recreating the layout of the physical device (except for masked out caps). In that case, this conflict should become impossible, no?
Normally capabilities in emulated devices are created by calling
msi_initormsix_init- just when emulated device wants to advertise it to the guest.In the case of VFIO, there is a lot of capabilities which QEMU does not know and does not want to know about. They are read from the host kernel as is. And we definitely want to pass these capabilities to the guest as is, i.e. on the same position and the same number of them. Just for some we call
pci_add_capability(indirectly!) if we want QEMU to support them somehow.
Quoted 7 lines
Quoted 3 lines
If we invent some function which "readds" all the capabilities we got from the host to keep internal QEMU's PCIDevice data in sync, then we'll need to change every piece of code which adds capabilities.
I can't follow. What is different in VFIO from
device-assignment.c,assigned_device_pci_cap_init(except that it already uses msi[x]_init, something we need to fix indevice-assignment.c)?
What are device-assignment.c and assigned_device_pci_cap_init? Cannot find them in QEMU tree.
Ah, anyway. The main difference is QEMU does not emulate VFIO devices, it just a proxy to the host system. Or I do not understand the question.
Quoted 4 lines
Quoted 4 lines
I noticed,
this is very common approach here to change a lot for a very small thing or rare case but I'd like to avoid this :)
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
But if pci_*add*_capability should actually be used like this (I doubt this),
MSI/MSIX use it. To enable MSI/MSIX on VFIO PCIDevice, we call msi_init/msix_init and they call
pci_add_capability.You can't blame msi_init/msix_init for the fact that VFIO creates a capability list with an existing MSI/MSI-X entry beforehand.
VFIO does not create any capability. It gets them all from the host kernel and passes to the guest as is. VFIO only needs MSIX to be enabled in VFIO.
Quoted 10 lines
Quoted 5 lines
Quoted 2 lines
some renaming would be required. "Add" sound like "append" to me, not "update".
It is "add" for all the cases but VFIO. VFIO is the very special case and I do not see another one doing the same soon.
PCI device assignment may have some special requirements. Then it is either required to generalize common services properly or keep the specialty local. So far, this proposal does not fall in any of those two categories.
It is a common patch. It does not know about VFIO and lets pci_add_capability handle one more situation when the capability already exists.
The only "common" solution I see here is
1) to add pci_fixup_capabilities() which would mark all the bytes of existing capabilities as "used", we will call it once we fetched the config space from the host kernel
2) to fix pci_add_capabilities not to fail and simply return (0?) if we add a capability which already exists.
Will it be ok?
Signature · 3 lines
With best regards Alexey Kardashevskiy -- icq: 52150396
Quoted 32 lines · Alexey Kardashevskiy
08.06.2012 21:30, Jan Kiszka пишет:
Quoted 31 lines · Alexey Kardashevskiy
Quoted 29 lines
08.06.2012 20:56, Jan Kiszka написал:
Quoted 7 lines · Alexey Kardashevskiy
Quoted 6 lines
Yet another try :)
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.Quoted 6 lines
Quoted 1 line
In the case of VFIO the capability may already exist and adding new
Why does it exit? VFIO should build the virtual capability list from scratch (just like classic device assignment does), recreating the layout of the physical device (except for masked out caps). In that case, this conflict should become impossible, no?
Normally capabilities in emulated devices are created by calling
msi_initormsix_init- just when emulated device wants to advertise it to the guest.In the case of VFIO, there is a lot of capabilities which QEMU does not know and does not want to know about. They are read from the host kernel as is. And we definitely want to pass these capabilities to the guest as is, i.e. on the same position and the same number of them. Just for some we call
pci_add_capability(indirectly!) if we want QEMU to support them somehow.If we invent some function which "readds" all the capabilities we got from the host to keep internal QEMU's PCIDevice data in sync, then we'll need to change every piece of code which adds capabilities.
Quoted 6 lines
Quoted 3 lines
I can't follow. What is different in VFIO from
device-assignment.c,assigned_device_pci_cap_init(except that it already uses msi[x]_init, something we need to fix indevice-assignment.c)?What are
device-assignment.candassigned_device_pci_cap_init? Cannot find them in QEMU tree.
"Old-style" KVM device assignment is not yet upstream. You can find it in qemu-kvm, hopefully in upstream soon as well.
Quoted 14 lines
Ah, anyway. The main difference is QEMU does not emulate VFIO devices, it just a proxy to the host system. Or I do not understand the question.
Quoted 10 lines
Quoted 9 lines
I noticed,
this is very common approach here to change a lot for a very small thing or rare case but I'd like to avoid this :)Quoted 2 lines
But if pci_*add*_capability should actually be used like this (I doubt this),
MSI/MSIX use it. To enable MSI/MSIX on VFIO PCIDevice, we call msi_init/msix_init and they call
pci_add_capability.
Quoted 6 lines
Quoted 2 lines
You can't blame msi_init/msix_init for the fact that VFIO creates a capability list with an existing MSI/MSI-X entry beforehand.
VFIO does not create any capability. It gets them all from the host kernel and passes to the guest as is. VFIO only needs MSIX to be enabled in VFIO.
Just like any device in QEMU, also VFIO need to set up a virtual config space when it registers with the PCI core layer. Even if the virtual one is modeled after the real one, it is still _created_ by the VFIO userspace part. And this creation process is obviously a bit messed up so far. Fix this, but not by adding workarounds in the MSI or PCI layer. Rather add all capabilities you want to expose to the guest via pci_add_capability or, indirectly, via msi[x]_init at the right position. Do not just copy the real config space over, that breaks the core layer as we see.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 64 lines · Jan Kiszka
Quoted 32 lines · Alexey Kardashevskiy
08.06.2012 21:30, Jan Kiszka пишет:
Quoted 31 lines · Alexey Kardashevskiy
Quoted 29 lines
08.06.2012 20:56, Jan Kiszka написал:
Quoted 7 lines · Alexey Kardashevskiy
Quoted 6 lines
Yet another try :)
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.Quoted 6 lines
Quoted 1 line
In the case of VFIO the capability may already exist and adding new
Why does it exit? VFIO should build the virtual capability list from scratch (just like classic device assignment does), recreating the layout of the physical device (except for masked out caps). In that case, this conflict should become impossible, no?
Normally capabilities in emulated devices are created by calling
msi_initormsix_init- just when emulated device wants to advertise it to the guest.In the case of VFIO, there is a lot of capabilities which QEMU does not know and does not want to know about. They are read from the host kernel as is. And we definitely want to pass these capabilities to the guest as is, i.e. on the same position and the same number of them. Just for some we call
pci_add_capability(indirectly!) if we want QEMU to support them somehow.If we invent some function which "readds" all the capabilities we got from the host to keep internal QEMU's PCIDevice data in sync, then we'll need to change every piece of code which adds capabilities.
Quoted 6 lines
Quoted 3 lines
I can't follow. What is different in VFIO from
device-assignment.c,assigned_device_pci_cap_init(except that it already uses msi[x]_init, something we need to fix indevice-assignment.c)?What are
device-assignment.candassigned_device_pci_cap_init? Cannot find them in QEMU tree."Old-style" KVM device assignment is not yet upstream. You can find it in qemu-kvm, hopefully in upstream soon as well.
Quoted 14 lines
Ah, anyway. The main difference is QEMU does not emulate VFIO devices, it just a proxy to the host system. Or I do not understand the question.
Quoted 10 lines
Quoted 9 lines
I noticed,
this is very common approach here to change a lot for a very small thing or rare case but I'd like to avoid this :)Quoted 2 lines
But if pci_*add*_capability should actually be used like this (I doubt this),
MSI/MSIX use it. To enable MSI/MSIX on VFIO PCIDevice, we call msi_init/msix_init and they call
pci_add_capability.Quoted 6 lines
Quoted 2 lines
You can't blame msi_init/msix_init for the fact that VFIO creates a capability list with an existing MSI/MSI-X entry beforehand.
VFIO does not create any capability. It gets them all from the host kernel and passes to the guest as is. VFIO only needs MSIX to be enabled in VFIO.
Quoted 9 lines
Just like any device in QEMU, also VFIO need to set up a virtual config space when it registers with the PCI core layer. Even if the virtual one is modeled after the real one, it is still _created_ by the VFIO userspace part. And this creation process is obviously a bit messed up so far. Fix this, but not by adding workarounds in the MSI or PCI layer. Rather add all capabilities you want to expose to the guest via
pci_add_capabilityor, indirectly, via msi[x]_init at the right position. Do not just copy the real config space over, that breaks the core layer as we see.
The difference between VFIO and kvm device assignment is that VFIO emulates a lot of config space for us, so most things are passed through. MSI and MSIX are unique that we actually do want the qemu support for helping us to manage them. So we're basically not telling qemu about anything other than these, and for the most part, that works since qemu never handles access to the other capabilities. However, I think you're probably right, VFIO should just walk the capabilities list, registering each with qemu. It's a little "unnecessary" overhead from the VFIO perspective, but it makes the VFIO device less unique. I'll work on adding this. Thanks,
Alex
Quoted 3 lines · Alex Williamson
The difference between VFIO and kvm device assignment is that VFIO emulates a lot of config space for us, so most things are passed through.
That's not different from current device assignment, is it? I think the major difference is that VFIO filters and potentially post-processes the direct writes in kernel space.
Quoted 8 lines
MSI and MSIX are unique that we actually do want the qemu
support for helping us to manage them. So we're basically not telling qemu about anything other than these, and for the most part, that works since qemu never handles access to the other capabilities. However, I think you're probably right, VFIO should just walk the capabilities list, registering each with qemu. It's a little "unnecessary" overhead from the VFIO perspective, but it makes the VFIO device less unique. I'll work on adding this. Thanks,
Great, thanks!
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 8 lines · Jan Kiszka
Quoted 3 lines · Alex Williamson
The difference between VFIO and kvm device assignment is that VFIO emulates a lot of config space for us, so most things are passed through.
That's not different from current device assignment, is it? I think the major difference is that VFIO filters and potentially post-processes the direct writes in kernel space.
Right, and having the filtering/virtualization in the kernel means that qemu only handles a very small subset of PCI config space. That's made us lax in even telling qemu about the areas that it'll never see accesses too. For current device assignment, since we doing the emulation in qemu, it's a little more beneficial to register everything. Thanks,
Alex
Quoted 57 lines · Alexey Kardashevskiy
Quoted 56 lines · Alexander Graf
Quoted 52 lines · Alexey Kardashevskiy
Quoted 28 lines · Alexander Graf
Quoted 24 lines · Benjamin Herrenschmidt
Quoted 3 lines · Alexey Kardashevskiy
Alexander,
Is that any better? :)
Alex (Graf that is), ping ?
The original patch from Alexey was fine btw.
VFIO will always call things with the existing capability offset so there's no real risk of doing the wrong thing or break the list or anything.
IE. A small simple patch that addresses the problem :-)
The new patch is a bit more "robust" I believe, I don't think we need to go too far to fix a problem we don't have. But we need a fix for the real issue and the simple patch does it neatly from what I can understand.
Cheers,
Ben.Quoted 1 line
Quoted 9 lines
Quoted 7 lines
hw/pci.c static void pci_del_option_rom(PCIDevice *pdev) · line 1783
Quoted 7 lines
hw/pci.c +1
-1@@ -1779,11 +1779,29 @@ static void pci_del_option_rom(PCIDevice *pdev)* in pci config space */ int pci_add_capability (PCIDevice *pdev,uint8_t cap_id,uint8_t offset,uint8_t size) { -uint8_t *config; +uint8_t *config, existing;Existing is a pointer to the target dev's config space, right?
Yes.
Quoted 10 lines
Quoted 8 lines
hw/pci.c static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom) · line 1816
Quoted 8 lines
hw/pci.c +6
-0int i, overlapping_cap; + existing =pci_find_capability (pdev, cap_id); +if (existing) { +if (offset && (existing != offset)) { +return -EEXIST ; + } +for (i = existing; i < size; ++i) {So how does this possibly make sense?
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Although I do not expect VFIO to add capabilities (does not make sense), I still want to double check that this space has not been tried to use by someone else.
i is an int. existing is a uint8_t*.
It was there before me. This function already does a loop and this is how it was coded at the first place.
Also, while at it, please add some comments at least for the code you add that explain why you do the things you do :).
Alex
Quoted 6 lines · Alexey Kardashevskiy
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.In the case of VFIO the capability may already exist and adding new capability into the beginning of the linked list may create a loop.
Hi,
I don't quite understand how we get a loop, if 'offset' is supplied to 'pci_add_capability' and there is an overlap we get -EINVAL. Otherwise, we are adding the capability in a new empty space. So, I see how we could get the capability in the list twice, but not how there is a loop. what am I missing?
Thanks,
-Jason
Quoted 52 lines
For example, the old code destroys the following config of PCIe Intel E1000E:
before adding
PCI_CAP_ID_MSI(0x05):0x34:0xC80xC8:0x010xD00xD0:0x050xE00xE0:0x100x00after:
0x34:0xD00xC8:0x010xD00xD0:0x050xC80xE0:0x100x00As result capabilities
0x01and0x05point to each other.The proposed patch does not change capability pointers when the same type capability is about to add.
Signed-off-by Alexey Kardashevskiy <[email protected]>--- hw/pci.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-)hw/pci.c +6
-4@@ -1794,10 +1794,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, } config = pdev->config + offset; - config[PCI_CAP_LIST_ID ] = cap_id; - config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; - pdev->config[PCI_CAPABILITY_LIST ] = offset; - pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; +if (config[PCI_CAP_LIST_ID ] != cap_id) { + config[PCI_CAP_LIST_ID ] = cap_id; + config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; + pdev->config[PCI_CAPABILITY_LIST ] = offset; + pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; + }memset (pdev->used + offset,0xFF , size);/* Make capability read-only by default */ memset (pdev->wmask + offset,0 , size);Signature · 2 lines
-- Alexey
12.05.2012 5:20, Jason Baron написал:
Quoted 8 lines · Alexey Kardashevskiy
Quoted 6 lines
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.In the case of VFIO the capability may already exist and adding new capability into the beginning of the linked list may create a loop.
Quoted 7 lines
Hi,
I don't quite understand how we get a loop, if 'offset' is supplied to '
pci_add_capability' and there is an overlap we get-EINVAL. Otherwise, we are adding the capability in a new empty space. So, I see how we could get the capability in the list twice, but not how there is a loop. what am I missing?
This happens only with VFIO.
The capability already exists in the config space as it is fetched from the host kernel _before_ msi_init is called. Furthermore, msi_init() is called when VFIO sees this capability in the config space.
We probably want to re-add all capabilities, do not know...
Quoted 51 lines
Thanks,
-Jason
Quoted 47 lines
For example, the old code destroys the following config of PCIe Intel E1000E:
before adding
PCI_CAP_ID_MSI(0x05):0x34:0xC80xC8:0x010xD00xD0:0x050xE00xE0:0x100x00after:
0x34:0xD00xC8:0x010xD00xD0:0x050xC80xE0:0x100x00As result capabilities
0x01and0x05point to each other.The proposed patch does not change capability pointers when the same type capability is about to add.
Signed-off-by Alexey Kardashevskiy <[email protected]>--- hw/pci.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-)hw/pci.c +6
-4@@ -1794,10 +1794,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, } config = pdev->config + offset; - config[PCI_CAP_LIST_ID ] = cap_id; - config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; - pdev->config[PCI_CAPABILITY_LIST ] = offset; - pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; +if (config[PCI_CAP_LIST_ID ] != cap_id) { + config[PCI_CAP_LIST_ID ] = cap_id; + config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; + pdev->config[PCI_CAPABILITY_LIST ] = offset; + pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; + }memset (pdev->used + offset,0xFF , size);/* Make capability read-only by default */ memset (pdev->wmask + offset,0 , size);
Signature · 3 lines
With best regards Alexey Kardashevskiy -- icq: 52150396
Quoted 18 lines · Alexey Kardashevskiy
12.05.2012 5:20, Jason Baron написал:
Quoted 8 lines · Alexey Kardashevskiy
Quoted 6 lines
Normally the
pci_add_capabilityis called on devices to add new capability. This is ok for emulated devices which capabilities list is being built by QEMU.In the case of VFIO the capability may already exist and adding new capability into the beginning of the linked list may create a loop.
Quoted 7 lines
Hi,
I don't quite understand how we get a loop, if 'offset' is supplied to '
pci_add_capability' and there is an overlap we get-EINVAL. Otherwise, we are adding the capability in a new empty space. So, I see how we could get the capability in the list twice, but not how there is a loop. what am I missing?
Quoted 7 lines
This happens only with VFIO.
The capability already exists in the config space as it is fetched from the host kernel _before_
msi_initis called. Furthermore,msi_init()is called when VFIO sees this capability in the config space.We probably want to re-add all capabilities, do not know...
Yep, I've had a msi[1] and msix[2] patches in my vfio tree for a long time, we really want to support this generically for all capabilities though. We either need to detect or allow the caller to specify that the config space is already programmed. Note that even if we don't create a loop, particularly finicky drivers may balk at just changing the order of the capabilities list. Thanks,
Alex
[1]https://github.com/awilliam/qemu-vfio/commit/a9f04351610ab69e22d90a76dc85be3269000a9f [2]https://github.com/awilliam/qemu-vfio/commit/b4de3d0436b0260fbc6fcd40787c1c92ffca2980
Quoted 50 lines
Quoted 47 lines
Quoted 47 lines
For example, the old code destroys the following config of PCIe Intel E1000E:
before adding
PCI_CAP_ID_MSI(0x05):0x34:0xC80xC8:0x010xD00xD0:0x050xE00xE0:0x100x00after:
0x34:0xD00xC8:0x010xD00xD0:0x050xC80xE0:0x100x00As result capabilities
0x01and0x05point to each other.The proposed patch does not change capability pointers when the same type capability is about to add.
Signed-off-by Alexey Kardashevskiy <[email protected]>--- hw/pci.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-)hw/pci.c +6
-4@@ -1794,10 +1794,12 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, } config = pdev->config + offset; - config[PCI_CAP_LIST_ID ] = cap_id; - config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; - pdev->config[PCI_CAPABILITY_LIST ] = offset; - pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; +if (config[PCI_CAP_LIST_ID ] != cap_id) { + config[PCI_CAP_LIST_ID ] = cap_id; + config[PCI_CAP_LIST_NEXT ] = pdev->config[PCI_CAPABILITY_LIST ]; + pdev->config[PCI_CAPABILITY_LIST ] = offset; + pdev->config[PCI_STATUS ] |=PCI_STATUS_CAP_LIST ; + }memset (pdev->used + offset,0xFF , size);/* Make capability read-only by default */ memset (pdev->wmask + offset,0 , size);