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

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2024/8/21 05:42, Stewart Hildebrand wrote:
> On 8/20/24 03:01, Jan Beulich wrote:
>> 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.
> 
> I agree on removing the condition for the reason you mentioned. I'd
> like to remove the other instances of the condition in this file as
> well, but that is the subject of a separate patch in the works [3].
> 
> [3] 
> https://lore.kernel.org/xen-devel/20231109182716.367119-9-stewart.hildebrand@xxxxxxx/
Thanks Stewart and Jan, I will remove this check from my patch in next version.

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

-- 
Best regards,
Jiqian Chen.