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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> +             * 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 )
>          {
>              /*