Re: [PATCH for-4.17 v2 4/5] pci: do not disable memory decoding for devices

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.10.2022 16:44, Roger Pau Monne wrote:
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -121,7 +121,9 @@ static void modify_decoding(const struct pci_dev *pdev, 
> uint16_t cmd,
>          }
>  
>          if ( !rom_only &&
> -             (bar->type != VPCI_BAR_ROM || header->rom_enabled) )
> +             (bar->type != VPCI_BAR_ROM || header->rom_enabled) &&
> +             pci_check_bar(pdev, _mfn(PFN_DOWN(bar->addr)),
> +                           _mfn(PFN_DOWN(bar->addr + bar->size - 1))) )
>              bar->enabled = map;
>      }

What about the ROM handling immediately above from here?

> @@ -234,9 +236,25 @@ static int modify_bars(const struct pci_dev *pdev, 
> uint16_t cmd, bool rom_only)
>  
>          if ( !MAPPABLE_BAR(bar) ||
>               (rom_only ? bar->type != VPCI_BAR_ROM
> -                       : (bar->type == VPCI_BAR_ROM && 
> !header->rom_enabled)) )
> +                       : (bar->type == VPCI_BAR_ROM && 
> !header->rom_enabled)) ||
> +             /* Skip BARs already in the requested state. */
> +             bar->enabled == !!(cmd & PCI_COMMAND_MEMORY) )
>              continue;
>  
> +        /*
> +         * Only do BAR position checks for the hardware domain, for guests 
> it's
> +         * assumed that the hardware domain has changed the position of any
> +         * problematic BARs.
> +         */
> +        if ( is_hardware_domain(pdev->domain) &&
> +             !pci_check_bar(pdev, _mfn(start), _mfn(end)) )
> +        {
> +            printk(XENLOG_G_WARNING
> +                   "%pp: not mapping BAR [%lx, %lx] invalid position\n",
> +                   &pdev->sbdf, start, end);
> +            continue;
> +        }

I'm not convinced of it being appropriate to skip the check for DomU.
I'd rather consider this a "fixme", as (perhaps somewhere else) we
should return an error if a misconfigured device was passed. We cannot
blindly leave the security of the system to tool stack + Dom0 kernel
imo.

And then, if this is Dom0-only, I think it wants to be XENLOG_WARNING,
i.e. without the G infix.

Jan