On 03/08/17 11:25, Punit Agrawal wrote:
> Jean-Philippe Brucker <jean-philippe.brucker@xxxxxxx> writes:
>
>> On 01/08/17 17:04, Punit Agrawal wrote:
>> [...]
>>> I think I've figured out what's going wrong. pci_for_each_cap is defined
>>> as -
>>>
>>> #define pci_for_each_cap(pos, cap, hdr) \
>>> for ((pos) = (hdr)->capabilities & ~3; \
>>> (cap) = (void *)(hdr) + (pos), (pos) != 0; \
>>> (pos) = ((struct pci_cap_hdr *)(cap))->next & ~3)
>>>
>>> Here cap is being assigned before the pos != 0 check. So in the last
>>> iteration through the loop, cap will point to the start of the PCI
>>> header - which is then used to set "last->next = pos".
>>
>> Ah right, sorry about that. How about moving the pos check in front? Does
>> the following work for you?
>>
>> diff --git a/include/kvm/pci.h b/include/kvm/pci.h
>> index c5fc8254..37a65956 100644
>> --- a/include/kvm/pci.h
>> +++ b/include/kvm/pci.h
>> @@ -142,9 +142,9 @@ struct pci_device_header {
>> enum irq_type irq_type;
>> };
>>
>> -#define pci_for_each_cap(pos, cap, hdr) \
>> - for ((pos) = (hdr)->capabilities & ~3; \
>> - (cap) = (void *)(hdr) + (pos), (pos) != 0; \
>> +#define pci_for_each_cap(pos, cap, hdr) \
>> + for ((pos) = (hdr)->capabilities & ~3; \
>> + (pos) != 0 && ({ (cap) = (void *)(hdr) + (pos); 1; }); \
>> (pos) = ((struct pci_cap_hdr *)(cap))->next & ~3)
>>
>> int pci__init(struct kvm *kvm);
>
> The compiler throws a warning with the above change and fails to compile
> (as all warnings are treated as error by kvmtool build) -
>
> vfio/pci.c: In function ‘vfio_pci_setup_device’:
> vfio/pci.c:511:14: error: ‘last’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> last->next = pos;
> ~~~~~~~~~~~^~~~~
> vfio/pci.c:507:23: note: ‘last’ was declared here
> struct pci_cap_hdr *last;
> ^~~~
Ok fair enough. In practice this wouldn't happen, as there is at least
one capability in the chain when we get there (the compiler might get
confused by the "& ~3" masking, which doesn't matter for our virtual
values). But the specialization you proposed to get the last capability
should be fine, I'll go with that in the next version.
Thanks,
Jean
