Re: [Qemu-devel] [PATCH] pci: Error on PCI capability collisions

[Jan Kiszka]

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 <address@hidden>
>>>>>>>>
>>>>>>>> Nothing good can happen when we overlap capabilities
>>>>>>>>
>>>>>>>> [ Jan: rebased over qemu, minor formatting ]
>>>>>>>>
>>>>>>>> Signed-off-by: Jan Kiszka <address@hidden>
>>>>>>>
>>>>>>> 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