| Message ID | 20230707014754.51333-3-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Kconfig for PCI passthrough on ARM | expand |
On 07.07.2023 03:47, Stewart Hildebrand wrote: > --- a/xen/arch/arm/include/asm/domain.h > +++ b/xen/arch/arm/include/asm/domain.h > @@ -298,8 +298,7 @@ static inline void arch_vcpu_block(struct vcpu *v) {} > > #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) > > -/* vPCI is not available on Arm */ > -#define has_vpci(d) ({ (void)(d); false; }) > +#define has_vpci(d) ({ IS_ENABLED(CONFIG_HAS_VPCI) && is_hardware_domain(d); }) While likely not much of a problem here, I think we should strive to write macros such that their arguments are evaluated exactly once in all cases (for side effects to occur exactly once). When that's not easily possible, so be it, but here it doesn't look problematic to swap both sides of the &&. Jan
Hi, On 07/07/2023 02:47, Stewart Hildebrand wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > VPCI is disabled on ARM. Make it depend on CONFIG_HAS_VPCI to test the PCI > passthrough support. Also make it depend on is_hardware_domain for now. The > is_hardware_domain check should be removed when vPCI is upstreamed. > > While here, remove the comment on the preceding line. > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > Signed-off-by: Rahul Singh <rahul.singh@arm.com> > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> > --- > There are two downstreams [1] [2] that have independently made a version this > change, each with different Signed-off-by's. I simply picked one at random for > the Author: field, and added both Signed-off-by lines. Please let me know if > there are any objections. > > v1->v2: > * add is_hardware_domain check. This will need to be removed after the vPCI > series [3] is merged. > > downstream->v1: > * change to IS_ENABLED(CONFIG_HAS_VPCI) instead of hardcoded to true > * remove the comment on the preceding line > > [1] https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commit/27be1729ce8128dbe37275ce7946b2fbd2e5a382 > [2] https://github.com/xen-troops/xen/commit/bf12185e6fb2e31db0d8e6ea9ccd8a02abadec17 > [3] https://lists.xenproject.org/archives/html/xen-devel/2023-06/msg00863.html > --- > xen/arch/arm/include/asm/domain.h | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h > index 99e798ffff68..1a13965a26b8 100644 > --- a/xen/arch/arm/include/asm/domain.h > +++ b/xen/arch/arm/include/asm/domain.h > @@ -298,8 +298,7 @@ static inline void arch_vcpu_block(struct vcpu *v) {} > > #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) > > -/* vPCI is not available on Arm */ > -#define has_vpci(d) ({ (void)(d); false; }) > +#define has_vpci(d) ({ IS_ENABLED(CONFIG_HAS_VPCI) && is_hardware_domain(d); }) So in v1, I asked whether we should use is_hardware_domain() or d->arch.pci. I see you went with the former, but wouldn't this mean that the vPCI is always enabled for dom0 when CONFIG_HAS_VPCI=y? Shouldn't this instead be conditional to pci_passthrough_enabled? So you could return d->arch.pci in has_vcpi(). The field would be set by domain_create() based on the flags passed by the caller. I would properly plumb to xen_domctl_createdomain and has a check in arch_sanitise_domain_config() to confirm the flag can be set. Cheers,
On 7/7/23 02:59, Jan Beulich wrote: > On 07.07.2023 03:47, Stewart Hildebrand wrote: >> --- a/xen/arch/arm/include/asm/domain.h >> +++ b/xen/arch/arm/include/asm/domain.h >> @@ -298,8 +298,7 @@ static inline void arch_vcpu_block(struct vcpu *v) {} >> >> #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) >> >> -/* vPCI is not available on Arm */ >> -#define has_vpci(d) ({ (void)(d); false; }) >> +#define has_vpci(d) ({ IS_ENABLED(CONFIG_HAS_VPCI) && is_hardware_domain(d); }) > > While likely not much of a problem here, I think we should strive to > write macros such that their arguments are evaluated exactly once in > all cases (for side effects to occur exactly once). When that's not > easily possible, so be it, but here it doesn't look problematic to > swap both sides of the &&. Thanks for pointing this out. Hmm... I'm considering turning it into a static inline function. This would also satisfy MISRA C:2012 Dir 4.9: "A function should be used in preference to a function-like macro where they are interchangeable" [1]. [1] https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/D_04_09.c
On 18.07.2023 19:01, Stewart Hildebrand wrote: > On 7/7/23 02:59, Jan Beulich wrote: >> On 07.07.2023 03:47, Stewart Hildebrand wrote: >>> --- a/xen/arch/arm/include/asm/domain.h >>> +++ b/xen/arch/arm/include/asm/domain.h >>> @@ -298,8 +298,7 @@ static inline void arch_vcpu_block(struct vcpu *v) {} >>> >>> #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) >>> >>> -/* vPCI is not available on Arm */ >>> -#define has_vpci(d) ({ (void)(d); false; }) >>> +#define has_vpci(d) ({ IS_ENABLED(CONFIG_HAS_VPCI) && is_hardware_domain(d); }) >> >> While likely not much of a problem here, I think we should strive to >> write macros such that their arguments are evaluated exactly once in >> all cases (for side effects to occur exactly once). When that's not >> easily possible, so be it, but here it doesn't look problematic to >> swap both sides of the &&. > > Thanks for pointing this out. Hmm... I'm considering turning it into a static inline function. This would also satisfy MISRA C:2012 Dir 4.9: "A function should be used in preference to a function-like macro where they are interchangeable" [1]. I don't think that'll work prior to us splitting type definitions into separate headers. You simply cannot deref d at this point (or in fact at any point within this header), as struct domain hasn't been defined yet. Jan > [1] https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/D_04_09.c
On 7/7/23 04:55, Julien Grall wrote: > Hi, > > On 07/07/2023 02:47, Stewart Hildebrand wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> VPCI is disabled on ARM. Make it depend on CONFIG_HAS_VPCI to test the PCI >> passthrough support. Also make it depend on is_hardware_domain for now. The >> is_hardware_domain check should be removed when vPCI is upstreamed. >> >> While here, remove the comment on the preceding line. >> >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> >> --- >> There are two downstreams [1] [2] that have independently made a version this >> change, each with different Signed-off-by's. I simply picked one at random for >> the Author: field, and added both Signed-off-by lines. Please let me know if >> there are any objections. >> >> v1->v2: >> * add is_hardware_domain check. This will need to be removed after the vPCI >> series [3] is merged. >> >> downstream->v1: >> * change to IS_ENABLED(CONFIG_HAS_VPCI) instead of hardcoded to true >> * remove the comment on the preceding line >> >> [1] https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commit/27be1729ce8128dbe37275ce7946b2fbd2e5a382 >> [2] https://github.com/xen-troops/xen/commit/bf12185e6fb2e31db0d8e6ea9ccd8a02abadec17 >> [3] https://lists.xenproject.org/archives/html/xen-devel/2023-06/msg00863.html >> --- >> xen/arch/arm/include/asm/domain.h | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h >> index 99e798ffff68..1a13965a26b8 100644 >> --- a/xen/arch/arm/include/asm/domain.h >> +++ b/xen/arch/arm/include/asm/domain.h >> @@ -298,8 +298,7 @@ static inline void arch_vcpu_block(struct vcpu *v) {} >> >> #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) >> >> -/* vPCI is not available on Arm */ >> -#define has_vpci(d) ({ (void)(d); false; }) >> +#define has_vpci(d) ({ IS_ENABLED(CONFIG_HAS_VPCI) && is_hardware_domain(d); }) > > So in v1, I asked whether we should use is_hardware_domain() or > d->arch.pci. I see you went with the former, but wouldn't this mean that > the vPCI is always enabled for dom0 when CONFIG_HAS_VPCI=y? Yes... > Shouldn't this instead be conditional to pci_passthrough_enabled? That could be a possibility, however, in v5 of the PCI ARM SMMU series [4], we propose removing the pci_passthrough_enabled flag (as a way to use PCI devices in dom0 without pci-passthrough=yes). If pci_passthrough_enabled is gone, the conditions under which vpci should be enabled for dom0 aren't entirely clear to me (other than CONFIG_HAS_VPCI=y). [4] https://lists.xenproject.org/archives/html/xen-devel/2023-10/msg00210.html > So you could return d->arch.pci in has_vcpi(). The field would be set by > domain_create() based on the flags passed by the caller. I would > properly plumb to xen_domctl_createdomain and has a check in > arch_sanitise_domain_config() to confirm the flag can be set. I'll plumb this for v3 of this series. > > Cheers, > > -- > Julien Grall
diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h index 99e798ffff68..1a13965a26b8 100644 --- a/xen/arch/arm/include/asm/domain.h +++ b/xen/arch/arm/include/asm/domain.h @@ -298,8 +298,7 @@ static inline void arch_vcpu_block(struct vcpu *v) {} #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) -/* vPCI is not available on Arm */ -#define has_vpci(d) ({ (void)(d); false; }) +#define has_vpci(d) ({ IS_ENABLED(CONFIG_HAS_VPCI) && is_hardware_domain(d); }) struct arch_vcpu_io { struct instr_details dabt_instr; /* when the instruction is decoded */