On Wed, Aug 24, 2011 at 02:36:31PM +0200, Jan Kiszka wrote: > On 2011-08-24 14:34, Michael S. Tsirkin wrote: > > On Wed, Aug 24, 2011 at 02:29:36PM +0200, Jan Kiszka wrote: > >> On 2011-08-24 13:58, Michael S. Tsirkin wrote: > >>> On Wed, Aug 24, 2011 at 12:10:32PM +0200, Jan Kiszka wrote: > >>>> On 2011-08-24 12:04, Michael S. Tsirkin wrote: > >>>>> On Tue, Aug 23, 2011 at 07:28:08PM +0200, Jan Kiszka wrote: > >>>>>> From: Alex Williamson <alex.william...@redhat.com> > >>>>>> > >>>>>> Nothing good can happen when we overlap capabilities > >>>>>> > >>>>>> [ Jan: rebased over qemu, minor formatting ] > >>>>>> > >>>>>> Signed-off-by: Jan Kiszka <jan.kis...@siemens.com> > >>>>> > >>>>> This doesn't build for me: > >>>>> > >>>>> /scm/qemu/hw/pci.c: In function ‘pci_add_capability’: > >>>>> /scm/qemu/hw/pci.c:1970:45: error: ‘PCIDevice’ has no member named > >>>>> ‘config_map’ > >>>> > >>>> Yeah, sorry, forgot to refresh the commit before posting. > >>>> > >>>>> > >>>>> I think that what that includes is the capability including each given > >>>>> offset, right? It would be easy to write some code scanning the > >>>>> capability list to figure this value out. > >>>>> Something along the lines of (untested): > >>>>> > >>>>> static > >>>>> uint8_t pci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset) > >>>>> { > >>>>> uint8_t next, prev, found = 0; > >>>>> > >>>>> if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST)) > >>>>> return 0; > >>>>> > >>>>> for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]); > >>>>> prev = next + PCI_CAP_LIST_NEXT) > >>>>> if (next <= offset && next > found) > >>>>> found = next; > >>>>> > >>>>> return found; > >>>>> } > >>>> > >>>> Sounds useful, will enhance the patch. > >>>> > >>>> (Originally, I just wanted to reduce the qemu-kvm delta... :) ) > >>>> > >>>> Jan > >>> > >>> Also, let's add a comment documenting the > >>> reason for this check: device assignment > >>> depends on this check to verify that the device > >>> is not broken. > >> > >> Based on the previous discussion, I don't think this is accurate as it > >> will also validate emulated devices. > >> > >> Jan > > > > Something like the below is accurate, right? > > > > /* Device assignment depends on this check to verify that the device > > is not broken. Should never trigger for emulated devices, > > but it's helpful for debugging these. > > */ > > I've expressed this in the commit message. Unless there is another > reason to do v3, maybe you can merge the comment on commit. > > Jan
Sure, I can do that, no need with v3. You are fine with the way it's formulated? > -- > Siemens AG, Corporate Technology, CT T DE IT 1 > Corporate Competence Center Embedded Linux