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....@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;
>>
