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

[Alexander Graf]

On 22.05.2012, at 08:11, Alexey Kardashevskiy <address@hidden> wrote:

> On 22/05/12 15:52, Alexander Graf wrote:
>> 
>> 
>> On 22.05.2012, at 05:44, Alexey Kardashevskiy <address@hidden> wrote:
>> 
>>> On 22/05/12 13:21, Alexander Graf wrote:
>>>> 
>>>> 
>>>> On 22.05.2012, at 04:02, Benjamin Herrenschmidt <address@hidden> wrote:
>>>> 
>>>>> On Fri, 2012-05-18 at 15:12 +1000, Alexey Kardashevskiy wrote:
>>>>>> 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.
>>>>> 
>>>>>> 
>>>>>> @@ -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.
>>> 
>>>>>>   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) {
>>>> 
>>>> 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.

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