On 22.05.2012, at 08:11, Alexey Kardashevskiy <aik@xxxxxxxxx> wrote:
> On 22/05/12 15:52, Alexander Graf wrote:
>>
>>
>> On 22.05.2012, at 05:44, Alexey Kardashevskiy <aik@xxxxxxxxx> wrote:
>>
>>> On 22/05/12 13:21, Alexander Graf wrote:
>>>>
>>>>
>>>> On 22.05.2012, at 04:02, Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
