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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.03.2025 15:50, Mykyta Poturai wrote:
> On 21.03.25 15:41, Jan Beulich wrote:
>> On 21.03.2025 11:56, Mykyta Poturai wrote:
>>> On 17.03.25 17:07, Jan Beulich wrote:
>>>> On 14.03.2025 14:34, Mykyta Poturai wrote:
>>>>> --- a/xen/drivers/pci/physdev.c
>>>>> +++ b/xen/drivers/pci/physdev.c
>>>>> @@ -19,7 +19,7 @@ ret_t pci_physdev_op(int cmd, 
>>>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>            struct pci_dev_info pdev_info;
>>>>>            nodeid_t node = NUMA_NO_NODE;
>>>>>    
>>>>> -        if ( !is_pci_passthrough_enabled() )
>>>>> +        if ( !is_pci_passthrough_enabled(true) )
>>>>>                return -EOPNOTSUPP;
>>>>
>>>> Seeing the function's parameter name, how do you know it's Dom0 calling
>>>> here?
>>>
>>> Is this a functional or naming concern? If it is about naming then can
>>> it also be solved by renaming the parameter?
>>
>> The renaming suggested above would resolve this, yes. Whether "for_pci_hwdom"
>> or anything alike is a good parameter name is a different question.
>>
>>> Regarding functional issues, I have assumed that only hwdom can make
>>> physdev operations, but after checking it, this assumption seems to be
>>> correct on x86 but wrong on Arm.
>>> I expected there would be a check in do_arm_physdev_op() or somewhere
>>> near it, similar to how it is done in x86, but there are none. I'm not
>>> sure if it is intentional or by mistake, I think it needs some
>>> clarification by Arm folks.
>>
>> Hmm, looking at x86'es do_physdev_op() I fear I can't see such a check there
>> either. And indeed there are certain PHYSDEVOP_* which DomU-s may also make
>> use of.
> 
> It is one level above in hvm_physdev_op()
> 
>      case PHYSDEVOP_setup_gsi:
>      case PHYSDEVOP_pci_mmcfg_reserved:
>      case PHYSDEVOP_pci_device_add:
>      case PHYSDEVOP_pci_device_remove:
>      case PHYSDEVOP_pci_device_reset:
>      case PHYSDEVOP_dbgp_op:
>          if ( !is_hardware_domain(currd) )
>              return -ENOSYS;
>          break;

But that's for just HVM guests, and only a functional restriction, not a
security one.

Jan