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

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

Posted by Roger Pau Monne 11 months, 1 week ago

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@citrix.com>
---
 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.
+             *
+             * 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.
+             */
+            continue;
+
         if ( tmp == pdev )
         {
             /*
-- 
2.40.0

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

Posted by Jan Beulich 11 months, 1 week ago

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@citrix.com>
> ---
>  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.

> +             * 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)? Plus, when considering hybrid guests, won't
we need to take into account BARs of externally handled devices as well,
to avoid overlaps?

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.

Jan

> +             */
> +            continue;
> +
>          if ( tmp == pdev )
>          {
>              /*

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

Posted by Roger Pau Monné 11 months, 1 week ago

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@citrix.com>
> > ---
> >  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?

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

Thanks, Roger.

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

Posted by Jan Beulich 11 months, 1 week ago

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@citrix.com>
>>> ---
>>>  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