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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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

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