Re: [PATCH] vpci/header: cope with devices not having vpci allocated

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.05.2023 15:27, Roger Pau Monné wrote:
> On Thu, May 25, 2023 at 11:05:52AM +0200, Jan Beulich wrote:
>> On 25.05.2023 10:30, Roger Pau Monne wrote:
>>> When traversing the list of pci devices assigned to a domain cope with
>>> some of them not having the vpci struct allocated. It's possible for
>>> the hardware domain to have read-only devices assigned that are not
>>> handled by vPCI, or for unprivileged domains to have some devices
>>> handled by an emulator different than vPCI.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>> ---
>>>  xen/drivers/vpci/header.c | 14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>>> index ec2e978a4e6b..3c1fcfb208cf 100644
>>> --- a/xen/drivers/vpci/header.c
>>> +++ b/xen/drivers/vpci/header.c
>>> @@ -289,6 +289,20 @@ static int modify_bars(const struct pci_dev *pdev, 
>>> uint16_t cmd, bool rom_only)
>>>       */
>>>      for_each_pdev ( pdev->domain, tmp )
>>>      {
>>> +        if ( !tmp->vpci )
>>> +            /*
>>> +             * For the hardware domain it's possible to have devices 
>>> assigned
>>> +             * to it that are not handled by vPCI, either because those are
>>> +             * read-only devices, or because vPCI setup has failed.
>>
>> So this really is a forward looking comment, becoming true only (aiui)
>> when my patch also makes it in.
> 
> The r/o part yes, device setup failing is already possible.
> 
> I think it's fine to have the r/o part added already.
> 
>>> +             * For unprivileged domains we should aim for passthrough 
>>> devices
>>> +             * to be capable of being handled by different emulators, and 
>>> hence
>>> +             * a domain could have some devices handled by vPCI and others 
>>> by
>>> +             * QEMU for example, and the later won't have pdev->vpci
>>> +             * allocated.
>>
>> This, otoh, I don't understand: Do we really intend to have pass-through
>> devices handled by qemu or alike, for PVH? Or are you thinking of hybrid
>> HVM (some vPCI, some qemu)?
> 
> I was thinking about hybrid.
> 
>> Plus, when considering hybrid guests, won't
>> we need to take into account BARs of externally handled devices as well,
>> to avoid overlaps?
> 
> On that scenario we would request non-overlapping BARs for things to
> work as expected, I think that's already the case for HVM if you mix
> QEMU and demu for the same domain.
> 
>> In any event, until the DomU side picture is more clear, I'd suggest we
>> avoid making statements pinning down expectations. That said, you're the
>> maintainer, so if you really want it like this, so be it.
> 
> OK, I don't have a strong opinion, so I'm fine with dropping the "For
> unprivileged domains ..." paragraph.
> 
> Would you like me to resend with that dropped?

Yes, please, because ...

> I assume you also want the commit message adjusted to not mention
> unprivileged domains?

... some adjustment will be wanted. Mentioning (vague) plans in the
description is fine with me, if you want to.

Jan