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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Aug 15, 2025 at 02:53:35PM -0400, Stewart Hildebrand wrote:
> On 8/14/25 12:03, Roger Pau Monne wrote:
> > 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, hopping 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.
> > 
> > No functional change for domUs intended.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> >  xen/drivers/vpci/header.c | 51 +++++++++++++++++++++------------------
> >  1 file changed, 28 insertions(+), 23 deletions(-)
> > 
> > diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> > index b9756b364300..1035dcca242d 100644
> > --- a/xen/drivers/vpci/header.c
> > +++ b/xen/drivers/vpci/header.c
> > @@ -64,7 +64,8 @@ static int cf_check map_range(
> >              printk(XENLOG_G_WARNING
> >                     "%pd denied access to MMIO range [%#lx, %#lx]\n",
> >                     map->d, map_mfn, m_end);
> > -            return -EPERM;
> > +            rc = -EPERM;
> > +            goto done;
> >          }
> >  
> >          rc = xsm_iomem_mapping(XSM_HOOK, map->d, map_mfn, m_end, map->map);
> > @@ -73,7 +74,7 @@ static int cf_check map_range(
> >              printk(XENLOG_G_WARNING
> >                     "%pd XSM denied access to MMIO range [%#lx, %#lx]: 
> > %d\n",
> >                     map->d, map_mfn, m_end, rc);
> > -            return rc;
> > +            goto done;
> >          }
> >  
> >          /*
> > @@ -87,17 +88,27 @@ 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;
> 
> If we return success and size is zero, we will potentially attempt to 
> map/unmap
> the same region again in an infinite loop. rangeset_consume_ranges would 
> invoke
> map_range again directly without returning to vpci_process_pending.
> 
> > +                *c += size;
> 
> This line is now only executed for hwdom, but ...
> 
> > +            }
> 
> ... it should go outside of the hwdom check because domUs still need it.

Indeed, this should be:

            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;

            *c += size;
            break;

Otherwise domU won't make progress.  It would be helpful to have some
domU testing in the CI loop, otherwise I have no way to test the domU
side when modifying vPCI.

Thanks, Roger.