On 2011-08-24 14:39, Michael S. Tsirkin wrote: > 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?
Yep. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux