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....@citrix.com>
> > ---
> > 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.
