Re: [XEN PATCH v13 1/6] xen/pci: Add hypercall to support reset of pcidev

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.08.2024 08:00, Chen, Jiqian wrote:
> On 2024/8/19 17:04, Jan Beulich wrote:
>> On 16.08.2024 13:08, Jiqian Chen wrote:
>>> @@ -67,6 +68,57 @@ ret_t pci_physdev_op(int cmd, 
>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>          break;
>>>      }
>>>  
>>> +    case PHYSDEVOP_pci_device_reset:
>>> +    {
>>> +        struct pci_device_reset dev_reset;
>>> +        struct pci_dev *pdev;
>>> +        pci_sbdf_t sbdf;
>>> +
>>> +        ret = -EOPNOTSUPP;
>>> +        if ( !is_pci_passthrough_enabled() )
>>> +            break;
>>
>> It occurs to me (only now, sorry): Does this case really need to be an
>> error? I.e. do we really need to bother callers by having them find out
>> whether pass-through is supported in the underlying Xen?
> I am not sure, but for x86, passthrough is always true, it doesn't matter.
> For arm, this hypercall is also used for passthrough devices for now, so it 
> is better to keep the same behavior as other PHYSDEVOP_pci_device_* operation?

Despite seeing that I did ack the respective change[1] back at the time, I
(now) view this as grossly misnamed, at best. Imo it makes pretty little
sense for that predicate helper to return true when there are no IOMMUs in
use. Even more so that on an Arm/PCI system without IOMMUs one can use the
command line option and then execution will make it past this check.

I further question the related part of [2]: Why did the stub need moving?
I'm not even sure that part of the change fell under the Suggested-by:
there, but I also can't exclude it (I didn't bother trying to find where
the suggestion was made).

In any event - with [1] PHYSDEVOP_*pci* ended up inconsistent on x86,
even if right now only on the surface. Yet as soon as this predicate is
changed to take IOMMUs into account, the latent inconsistency would
become a real one.

An alternative to changing how the function behaves would be to rename it,
for name and purpose to actually match - is_pci_passthrough_permitted()
maybe?

Thoughts anyone, Arm / SMMU maintainers in particular?

Finally, as to the change here: On an Arm/PCI system where pass-through
isn't enabled, the hypervisor will still need to know about resets when
vPCI is in use for Dom0. IOW I'd like to refine my earlier comment into
suggesting that the conditional be dropped altogether.

Jan

[1] 15517ed61f55 xen/arm: Add cmdline boot option "pci-passthrough = <boolean>"
[2] dec9e02f3190 xen: avoid generation of stub <asm/pci.h> header