Re: [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.04.2025 13:26, Mykyta Poturai wrote:
> On 28.04.25 12:01, Jan Beulich wrote:
>> On 28.04.2025 10:21, Mykyta Poturai wrote:
>>> On 17.03.25 17:07, Jan Beulich wrote:
>>>> On 14.03.2025 14:34, Mykyta Poturai wrote:
>>>>> --- a/xen/arch/arm/pci/pci.c
>>>>> +++ b/xen/arch/arm/pci/pci.c
>>>>> @@ -16,9 +16,18 @@
>>>>>    #include <xen/device_tree.h>
>>>>>    #include <xen/errno.h>
>>>>>    #include <xen/init.h>
>>>>> +#include <xen/iommu.h>
>>>>>    #include <xen/param.h>
>>>>>    #include <xen/pci.h>
>>>>>    
>>>>> +bool is_pci_passthrough_enabled(bool dom0)
>>>>> +{
>>>>> +    if ( dom0 )
>>>>> +        return pci_passthrough_enabled || iommu_enabled;
>>>>
>>>> As I think I said before - the function's name now no longer expresses
>>>> what it really checks. That (imo heavily) misleading at the use sites
>>>> of this function.
>>>
>>> I've spent some more time thinking about how to better deal with this.
>>> In the end, I think your earlier suggestion about introducing a new arch
>>> specific function is the best approach, but I want to agree on the
>>> naming before sending new patches. Would "arch_requires_pci_physdev" be
>>> an appropriate name in your opinion?
>>>
>>> At the call sites it will look like this:
>>>       case PHYSDEVOP_pci_device_remove: {
>>>           struct physdev_pci_device dev;
>>>
>>>           if ( !is_pci_passthrough_enabled() && 
>>> !arch_requires_pci_physdev())
>>>               return -EOPNOTSUPP;
>>
>> There are several questions that affect naming: Is it really "requires"? Is
>> it really all PCI-related physdevops? Is the ordering of naming elements in
>> line with what we use elsewhere (arch_ first is, but perhaps either pci or
>> physdevop wants to move earlier)?
> 
> I understand the issue with the ordering, will 
> "arch_pci_requires_physdev_ops" or "arch_physdev_pci_update_required" be 
> better? Regarding the specific ops, only add/remove are needed, but I am 
> not sure how to elegantly encode this in the name. Maybe you can suggest 
> something better if you have something specific in mind?

Simply arch_pci_device_physdevop()? This would also avoid the question if
it's "requires", "wants", or yet something else. (I'm not going to insist
that the verb be omitted, though. If it's included, I'd ask though that
it match in tense the "enabled" in the other predicate.) And it would cover
PHYSDEVOP_pci_device_reset as well, which sooner or later I expect you'll
discover you want/need Dom0 to issue, too.

Jan