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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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

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