Re: [PATCH v5 1/9] vpci: move BAR mapping permissions checks

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.07.2026 23:02, Stewart Hildebrand wrote:
> There's no need to defer the permissions checks. Perform them right away
> in modify_bars().
> 
> As a result of the move, the permissions checks will now cover the whole
> BAR before any holes are removed.

Is this a good idea though? A really good toolstack would avoid granting
access to e.g. the MSI-X table.

> Carry over the domain_crash() in the error path for domUs.

I don't think this is warranted.

> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -58,24 +58,6 @@ static int cf_check map_range(
>           * offset of the current address from the BAR start.
>           */
>          unsigned long map_mfn = start_mfn + s - start_gfn;
> -        unsigned long m_end = map_mfn + size - 1;
> -
> -        if ( !iomem_access_permitted(map->d, map_mfn, m_end) )
> -        {
> -            printk(XENLOG_G_WARNING
> -                   "%pd denied access to MMIO range [%#lx, %#lx]\n",
> -                   map->d, map_mfn, m_end);
> -            return -EPERM;
> -        }
> -
> -        rc = xsm_iomem_mapping_vpci(XSM_HOOK, map->d, map_mfn, m_end, 
> map->map);
> -        if ( rc )
> -        {
> -            printk(XENLOG_G_WARNING
> -                   "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
> -                   map->d, map_mfn, m_end, rc);

I realize you literally only take what's here, but ...

> @@ -369,6 +351,28 @@ static int modify_bars(const struct pci_dev *pdev, 
> uint16_t cmd, bool rom_only)
>              return -EINVAL;
>          }
>  
> +        if ( !iomem_access_permitted(pdev->domain, start, end) )
> +        {
> +            printk(XENLOG_G_WARNING
> +                   "%pd denied access to MMIO range [%#lx, %#lx]\n",
> +                   pdev->domain, start, end);
> +            if ( !is_hardware_domain(pdev->domain) )
> +                domain_crash(pdev->domain);
> +            return -EPERM;
> +        }
> +
> +        rc = xsm_iomem_mapping_vpci(XSM_HOOK, pdev->domain, start, end,
> +                                    !!(cmd & PCI_COMMAND_MEMORY));
> +        if ( rc )
> +        {
> +            printk(XENLOG_G_WARNING
> +                   "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
> +                   pdev->domain, start, end, rc);

... is the logging of rc actually useful? Afaict it's only ever going to be
-EPERM.

Jan