Re: [PATCH v4 00/11] PCI devices passthrough on Arm, part 3

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.11.2021 09:34, Oleksandr Andrushchenko wrote:
> On 22.11.21 10:22, Jan Beulich wrote:
>> On 19.11.2021 15:23, Roger Pau Monné wrote:
>>> On Fri, Nov 19, 2021 at 02:56:12PM +0100, Jan Beulich wrote:
>>>> On 05.11.2021 07:56, Oleksandr Andrushchenko wrote:
>>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>>>>
>>>>> Hi, all!
>>>>>
>>>>> This patch series is focusing on vPCI and adds support for non-identity
>>>>> PCI BAR mappings which is required while passing through a PCI device to
>>>>> a guest. The highlights are:
>>>>>
>>>>> - Add relevant vpci register handlers when assigning PCI device to a 
>>>>> domain
>>>>>    and remove those when de-assigning. This allows having different
>>>>>    handlers for different domains, e.g. hwdom and other guests.
>>>>>
>>>>> - Emulate guest BAR register values based on physical BAR values.
>>>>>    This allows creating a guest view of the registers and emulates
>>>>>    size and properties probe as it is done during PCI device enumeration 
>>>>> by
>>>>>    the guest.
>>>>>
>>>>> - Instead of handling a single range set, that contains all the memory
>>>>>    regions of all the BARs and ROM, have them per BAR.
>>>>>
>>>>> - Take into account guest's BAR view and program its p2m accordingly:
>>>>>    gfn is guest's view of the BAR and mfn is the physical BAR value as set
>>>>>    up by the host bridge in the hardware domain.
>>>>>    This way hardware doamin sees physical BAR values and guest sees
>>>>>    emulated ones.
>>>>>
>>>>> The series also adds support for virtual PCI bus topology for guests:
>>>>>   - We emulate a single host bridge for the guest, so segment is always 0.
>>>>>   - The implementation is limited to 32 devices which are allowed on
>>>>>     a single PCI bus.
>>>>>   - The virtual bus number is set to 0, so virtual devices are seen
>>>>>     as embedded endpoints behind the root complex.
>>>>>
>>>>> The series was also tested on:
>>>>>   - x86 PVH Dom0 and doesn't break it.
>>>>>   - x86 HVM with PCI passthrough to DomU and doesn't break it.
>>>>>
>>>>> Thank you,
>>>>> Oleksandr
>>>>>
>>>>> Oleksandr Andrushchenko (11):
>>>>>    vpci: fix function attributes for vpci_process_pending
>>>>>    vpci: cancel pending map/unmap on vpci removal
>>>>>    vpci: make vpci registers removal a dedicated function
>>>>>    vpci: add hooks for PCI device assign/de-assign
>>>>>    vpci/header: implement guest BAR register handlers
>>>>>    vpci/header: handle p2m range sets per BAR
>>>>>    vpci/header: program p2m with guest BAR view
>>>>>    vpci/header: emulate PCI_COMMAND register for guests
>>>>>    vpci/header: reset the command register when adding devices
>>>>>    vpci: add initial support for virtual PCI bus topology
>>>>>    xen/arm: translate virtual PCI bus topology for guests
>>>> If I'm not mistaken by the end of this series a guest can access a
>>>> device handed to it. I couldn't find anything dealing with the
>>>> uses of vpci_{read,write}_hw() and vpci_hw_{read,write}*() to cover
>>>> config registers not covered by registered handlers. IMO this should
>>>> happen before patch 5: Before any handlers get registered the view a
>>>> guest would have would be all ones no matter which register it
>>>> accesses. Handler registration would then "punch holes" into this
>>>> "curtain", as opposed to Dom0, where handler registration hides
>>>> previously visible raw hardware registers.
>>> FWIW, I've also raised the same concern in a different thread:
>>>
>>> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/YYD7VmDGKJRkid4a@Air-de-Roger/__;!!GF_29dbcQIUBPA!n37Yig9pAAyho7fB9kC-q0T-gjC_utpzjtQxNv8udMX0dXK54PWcHwBqtmzHXSc5lTkzKu4XfQ$
>>>  [lore[.]kernel[.]org]
>>>
>>> It seems like this is future work, but unless such a model is
>>> implemented vPCI cannot be used for guest passthrough.
>>>
>>> I'm fine with doing it in a separate series, but needs to be kept in
>>> mind.
>> Not just this - it also needs to be recorded in this cover letter and
>> imo also in a comment in the sources somewhere. Or else the question
>> will (validly) be raised again and again.
> I am fine adding such a comment, but am not sure where to put it.
> What would be your best bet if you were to look for this information?
> I think we can put that in xen/drivers/vpci/vpci.c at the top, right
> after the license in the same comment block.

I would put this e.g. next to the first call to vpci_read_hw() from
vpci_read(), making the wording general enough to express that this
applies to all such calls, including the write counterpart ones.

Jan