| Message ID | ca4c2d0f-883b-379f-27bd-d86a436870d2@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Jean-Philippe Brucker <jean-philippe.brucker@arm.com> 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; ^~~~
On 03/08/17 11:25, Punit Agrawal wrote: > Jean-Philippe Brucker <jean-philippe.brucker@arm.com> 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
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);