[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v9 7/8] xen/arm: enable dom0 to use PCI devices with pci-passthrough=no
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/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 am afraid I don't understand your concern. It still checks if PCI >> passthrough is enabled. With just the change that ARM needs some extra >> logic for Dom0 PCI to work properly. > > Conceptually there's no such thing as "pass through" for Dom0. Hence the > name of the function itself isn't correctly reflecting what it's checking > for. iommu_enabled is a prereq for pass-through to be enabled, but it > doesn't imply that's necessarily the case. Okay, now I think I get it. Yes from that point of view it seems kind of wrong. Maybe use a separate function then, something like "hwdom_has_vpci"? >> Maybe change the parameter name to >> something like "for_pci_hwdom"? > > That may help below, yes. But not here. > >>>>> @@ -85,7 +94,7 @@ static int __init pci_init(void) >>>> * Enable PCI passthrough when has been enabled explicitly >>>> * (pci-passthrough=on). >>>> */ >>>> - if ( !pci_passthrough_enabled ) >>>> + if ( !is_pci_passthrough_enabled(true) ) >>> >>> There's no Dom0 in sight anywhere here, is there? How can you pass true >>> as argument for the "dom0" parameter? >> >> This should be read as "is pci passthrough enabled for Dom0?" and if it >> is we definitely need to do a PCI init. >> >> I've also done some investigations on possible ways to remove the >> Dom0/other domains distinction, but I'm afraid this is the most >> reasonable way to make PCI functional on Dom0 without explicitly >> enabling PCI passthrough. >> >> SMMU is configured to trigger a fault on all transactions by default and >> we can't statically map PCI devices to Dom0, so the only other way is to >> put PCI in full passthrough mode, which I think is not safe enough. >> And we also can't drop this patch as it was directly requested by Julien >> here [1]. >> >>>> --- 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. > > Jan 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; -- Mykyta
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |