On 22.05.2012, at 08:11, Alexey Kardashevskiy <a...@ozlabs.ru> wrote:
> On 22/05/12 15:52, Alexander Graf wrote: >> >> >> On 22.05.2012, at 05:44, Alexey Kardashevskiy <a...@ozlabs.ru> wrote: >> >>> On 22/05/12 13:21, Alexander Graf wrote: >>>> >>>> >>>> On 22.05.2012, at 04:02, Benjamin Herrenschmidt <b...@kernel.crashing.org> >>>> 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