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@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;
+ *c += size;
+ }
break;
}
ASSERT(rc < size);
@@ -213,28 +224,22 @@ bool vpci_process_pending(struct vcpu *v)
return true;
}
- if ( rc )
+ if ( rc && !is_hardware_domain(v->domain) )
{
- spin_lock(&pdev->vpci->lock);
- /* Disable memory decoding unconditionally on failure. */
- modify_decoding(pdev, v->vpci.cmd & ~PCI_COMMAND_MEMORY,
- false);
- spin_unlock(&pdev->vpci->lock);
-
- /* Clean all the rangesets */
- for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
- if ( !rangeset_is_empty(header->bars[i].mem) )
- rangeset_purge(header->bars[i].mem);
-
- v->vpci.pdev = NULL;
-
read_unlock(&v->domain->pci_lock);
- if ( !is_hardware_domain(v->domain) )
- domain_crash(v->domain);
+ domain_crash(v->domain);
return false;
}
+ ASSERT(!rc);
+ /*
+ * Purge rangeset to deal with the hardware domain having triggered an
+ * error. It shouldn't be possible, as map_range() will always shallow
+ * errors for hardware domain owned devices, and
+ * rangeset_consume_ranges() itself doesn't generate any errors.
+ */
+ rangeset_purge(bar->mem);
}
v->vpci.pdev = NULL;
--
2.49.0
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@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.
> break;
> }
> ASSERT(rc < size);
> @@ -213,28 +224,22 @@ bool vpci_process_pending(struct vcpu *v)
> return true;
> }
>
> - if ( rc )
> + if ( rc && !is_hardware_domain(v->domain) )
> {
> - spin_lock(&pdev->vpci->lock);
> - /* Disable memory decoding unconditionally on failure. */
> - modify_decoding(pdev, v->vpci.cmd & ~PCI_COMMAND_MEMORY,
> - false);
This path could be taken for either map or unmap. Do we still want to disable
memory decoding in case of unmap?
> - spin_unlock(&pdev->vpci->lock);
> -
> - /* Clean all the rangesets */
> - for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
> - if ( !rangeset_is_empty(header->bars[i].mem) )
> - rangeset_purge(header->bars[i].mem);
> -
> - v->vpci.pdev = NULL;
> -
> read_unlock(&v->domain->pci_lock);
>
> - if ( !is_hardware_domain(v->domain) )
> - domain_crash(v->domain);
> + domain_crash(v->domain);
>
> return false;
> }
> + ASSERT(!rc);
> + /*
> + * Purge rangeset to deal with the hardware domain having triggered an
> + * error. It shouldn't be possible, as map_range() will always shallow
> + * errors for hardware domain owned devices, and
> + * rangeset_consume_ranges() itself doesn't generate any errors.
> + */
> + rangeset_purge(bar->mem);
Reiterating what was mentioned above: if map_range returned 0 without
incrementing *c, we won't make it back here.
> }
> v->vpci.pdev = NULL;
>
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@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.
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@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.
>
> > break;
> > }
> > ASSERT(rc < size);
> > @@ -213,28 +224,22 @@ bool vpci_process_pending(struct vcpu *v)
> > return true;
> > }
> >
> > - if ( rc )
> > + if ( rc && !is_hardware_domain(v->domain) )
> > {
> > - spin_lock(&pdev->vpci->lock);
> > - /* Disable memory decoding unconditionally on failure. */
> > - modify_decoding(pdev, v->vpci.cmd & ~PCI_COMMAND_MEMORY,
> > - false);
>
> This path could be taken for either map or unmap. Do we still want to disable
> memory decoding in case of unmap?
Does it make an effective difference? For the hardware domain we
should never get here, and for domUs the domain will be destroyed, so
disabling memory decoding is not helpful?
Thanks, Roger.
On 8/18/25 03:42, Roger Pau Monné wrote:
> 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@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
<snip>
>>> @@ -213,28 +224,22 @@ bool vpci_process_pending(struct vcpu *v)
>>> return true;
>>> }
>>>
>>> - if ( rc )
>>> + if ( rc && !is_hardware_domain(v->domain) )
>>> {
>>> - spin_lock(&pdev->vpci->lock);
>>> - /* Disable memory decoding unconditionally on failure. */
>>> - modify_decoding(pdev, v->vpci.cmd & ~PCI_COMMAND_MEMORY,
>>> - false);
>>
>> This path could be taken for either map or unmap. Do we still want to disable
>> memory decoding in case of unmap?
>
> Does it make an effective difference? For the hardware domain we
> should never get here, and for domUs the domain will be destroyed, so
> disabling memory decoding is not helpful?
Since the domU will be destroyed, the PCI device will get assigned back to hwdom
or put into quarantine. In case of quarantine, it shouldn't matter. In case of
assignment back to hwdom, I think by keeping memory decoding enabled it could
potentially trigger additional p2m operations.
In any case, I don't have a strong opinion on disabling memory decoding vs not,
but I think the commit message ought to mention something about it.
On 8/15/25 14:53, 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@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.
Sorry, I sent the previous email too soon, I see now that it shouldn't be
possible for size to be zero.
>
>> + *c += size;
>
> This line is now only executed for hwdom, but ...
>
>> + }
>
> ... it should go outside of the hwdom check because domUs still need it.
>
>> break;
>> }
>> ASSERT(rc < size);
>> @@ -213,28 +224,22 @@ bool vpci_process_pending(struct vcpu *v)
>> return true;
>> }
>>
>> - if ( rc )
>> + if ( rc && !is_hardware_domain(v->domain) )
>> {
>> - spin_lock(&pdev->vpci->lock);
>> - /* Disable memory decoding unconditionally on failure. */
>> - modify_decoding(pdev, v->vpci.cmd & ~PCI_COMMAND_MEMORY,
>> - false);
>
> This path could be taken for either map or unmap. Do we still want to disable
> memory decoding in case of unmap?
>
>> - spin_unlock(&pdev->vpci->lock);
>> -
>> - /* Clean all the rangesets */
>> - for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
>> - if ( !rangeset_is_empty(header->bars[i].mem) )
>> - rangeset_purge(header->bars[i].mem);
>> -
>> - v->vpci.pdev = NULL;
>> -
>> read_unlock(&v->domain->pci_lock);
>>
>> - if ( !is_hardware_domain(v->domain) )
>> - domain_crash(v->domain);
>> + domain_crash(v->domain);
>>
>> return false;
>> }
>> + ASSERT(!rc);
>> + /*
>> + * Purge rangeset to deal with the hardware domain having triggered an
>> + * error. It shouldn't be possible, as map_range() will always shallow
>> + * errors for hardware domain owned devices, and
>> + * rangeset_consume_ranges() itself doesn't generate any errors.
>> + */
>> + rangeset_purge(bar->mem);
>
> Reiterating what was mentioned above: if map_range returned 0 without
> incrementing *c, we won't make it back here.
>
>> }
>> v->vpci.pdev = NULL;
>>
© 2016 - 2025 Red Hat, Inc.