Re: [Qemu-devel] [RFC PATCH] qemu pci: pci_add_capability enhancement to prevent damaging config space

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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;






On 14/05/12 13:49, Alexey Kardashevskiy wrote:
> On 12/05/12 00:13, Alexander Graf wrote:
>>
>> On 11.05.2012, at 14:47, Alexey Kardashevskiy wrote:
>>
>>> 11.05.2012 20:52, Alexander Graf написал:
>>>>
>>>> On 11.05.2012, at 08:45, Alexey Kardashevskiy wrote:
>>>>
>>>>> 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 0x00
>>>>>
>>>>> after:
>>>>> 0x34: 0xD0
>>>>> 0xC8: 0x01 0xD0
>>>>> 0xD0: 0x05 0xC8
>>>>> 0xE0: 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.
>>>>>
>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>> ---
>>>>> 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 :)
>>
>>>>
>>>> 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.
>>
>> 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.
> 
> 
> 
>>> 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?
>>
>> 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
>>
> 
> 


-- 
Alexey