Re: [PATCH V7 09/11] vpci: add initial support for virtual PCI bus topology

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.07.2022 16:16, Oleksandr wrote:
> On 27.07.22 13:32, Jan Beulich wrote:
>> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>>
>>> Assign SBDF to the PCI devices being passed through with bus 0.
>>> The resulting topology is where PCIe devices reside on the bus 0 of the
>>> root complex itself (embedded endpoints).
>>> This implementation is limited to 32 devices which are allowed on
>>> a single PCI bus.
>>>
>>> Please note, that at the moment only function 0 of a multifunction
>>> device can be passed through.
>> I've not been able to spot where this restriction is being enforced -
>> can you please point me at the respective code?
> 
> Nor have I found the respective code.
> 
> Could you please suggest a place where to put such enforcement (I guess, 
> this should be present in the toolstack)?

Such check should be in the tool stack primarily to give a sensible
error message to the user. Yet the hypervisor needs to check itself
nevertheless. You know the code you're adding much better than I do,
so I guess I'm a little puzzled by you asking me to suggest a place.
(And for the tool stack I guess asking tool stack folks would get
you better mileage.)

>>> @@ -124,6 +191,7 @@ void vpci_deassign_device(struct pci_dev *pdev)
>>>       if ( !has_vpci(pdev->domain) )
>>>           return;
>>>   
>>> +    vpci_remove_virtual_device(pdev);
>>>       vpci_remove_device(pdev);
>>>   }
>> And other call sites of vpci_remove_device() do not have a need of
>> cleaning up guest_sbdf / vpci_dev_assigned_map?
> 
> I am not 100% sure, but it looks like they don't need. On the other 
> hand, even if they don't need that, doing the cleaning won't be an issue 
> at all,
> 
> there is a check before cleaning (which will be extended as I proposed 
> above), so ...
> 
> 
>> IOW I wonder if it
>> wouldn't be better to have vpci_remove_device() do this as well
>> (retaining - see my comment on the earlier patch) the simple aliasing
>> of vpci_deassign_device() to vpci_remove_device()).
> 
> 
>     ... maybe yes. Shall I do that change?

Well - yes please, afaic.

Jan