Re: [PATCH v5 2/9] vpci: make BAR mapping more resilient for the hardware domain

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.07.2026 23:02, Stewart Hildebrand wrote:
> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> 
> The logic in map_range() will bubble up failures to the upper layer, which
> will result in any remaining regions being skip, and for the non-hardware
> domain case the owner domain of the device would be destroyed.  However for
> the hardware domain the intent is to continue execution, hoping the
> failure to modify the p2m could be worked around by the hardware domain.
> 
> To accomplish that in a better way, ignore failures and skip the range in
> that case, possibly continuing to map further ranges.
> 
> Since the error path in vpci_process_pending() should only be used by domUs
> now, and it will unconditionally end up calling domain_crash(), simplify
> it: there's no need to cleanup if the domain will be destroyed.

Aren't we increasing the number of places adjustments will need making if
that (not really nice) crashing of the domain was sorted at some point?

> Memory decoding may be left enabled in case of mapping error for devices
> assigned to domUs.

I'm similarly concerned of this. I also can't spot why this would be for
DomU-s only. The original code did it for all domains.

> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -70,17 +70,26 @@ static int cf_check map_range(
>  
>          rc = map->map ? map_mmio_regions(map->d, _gfn(s), size, 
> _mfn(map_mfn))
>                        : unmap_mmio_regions(map->d, _gfn(s), size, 
> _mfn(map_mfn));
> -        if ( rc == 0 )
> -        {
> -            *c += size;
> -            break;
> -        }
>          if ( rc < 0 )
>          {
>              printk(XENLOG_G_WARNING
>                     "Failed to %smap [%lx %lx] -> [%lx %lx] for %pd: %d\n",
>                     map->map ? "" : "un", s, e, map_mfn,
>                     map_mfn + size, map->d, rc);
> +            goto done;
> +        }
> +        if ( rc == 0 )
> +        {
> + done:
> +            if ( is_hardware_domain(map->d) )
> +                /*
> +                 * Ignore failures for the hardware domain and skip the 
> range.
> +                 * Do it as a best effort workaround to attempt to get the
> +                 * hardware domain to boot.
> +                 */
> +                rc = 0;

Why would this not move ahead of the "goto done"? For the if() here it's
a no-op. Then (really: independently) ...

> +            *c += size;
>              break;
>          }

... the remaining piece could be in "if ( rc <= 0 )".

Jan