| Message ID | 20230814042536.878720-14-Henry.Wang@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Split MMU code as the prepration of MPU work | expand |
On 14.08.2023 06:25, Henry Wang wrote: > From: Penny Zheng <Penny.Zheng@arm.com> > > SMMU subsystem is only supported in MMU system, so we make it dependent > on CONFIG_HAS_MMU. Nit: Stale "HAS" infix? Jan > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > Signed-off-by: Wei Chen <wei.chen@arm.com> > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > Acked-by: Jan Beulich <jbeulich@suse.com> > --- > v5: > - Add Acked-by tag from Jan. > v4: > - No change > v3: > - new patch > --- > xen/drivers/passthrough/Kconfig | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/xen/drivers/passthrough/Kconfig b/xen/drivers/passthrough/Kconfig > index 864fcf3b0c..ebb350bc37 100644 > --- a/xen/drivers/passthrough/Kconfig > +++ b/xen/drivers/passthrough/Kconfig > @@ -5,6 +5,7 @@ config HAS_PASSTHROUGH > if ARM > config ARM_SMMU > bool "ARM SMMUv1 and v2 driver" > + depends on MMU > default y > ---help--- > Support for implementations of the ARM System MMU architecture > @@ -15,7 +16,7 @@ config ARM_SMMU > > config ARM_SMMU_V3 > bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support" if EXPERT > - depends on ARM_64 && (!ACPI || BROKEN) > + depends on ARM_64 && (!ACPI || BROKEN) && MMU > ---help--- > Support for implementations of the ARM System MMU architecture > version 3. Driver is in experimental stage and should not be used in
Hi Jan, > On Aug 14, 2023, at 15:08, Jan Beulich <jbeulich@suse.com> wrote: > > On 14.08.2023 06:25, Henry Wang wrote: >> From: Penny Zheng <Penny.Zheng@arm.com> >> >> SMMU subsystem is only supported in MMU system, so we make it dependent >> on CONFIG_HAS_MMU. > > Nit: Stale "HAS" infix? Ah…Nice catch, sorry about that, will fix that in v6 if the series needs changes in other patches. Kind regards, Henry > > Jan > >> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >> Signed-off-by: Wei Chen <wei.chen@arm.com> >> Signed-off-by: Henry Wang <Henry.Wang@arm.com> >> Acked-by: Jan Beulich <jbeulich@suse.com> >> --- >> v5: >> - Add Acked-by tag from Jan. >> v4: >> - No change >> v3: >> - new patch >> --- >> xen/drivers/passthrough/Kconfig | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/xen/drivers/passthrough/Kconfig b/xen/drivers/passthrough/Kconfig >> index 864fcf3b0c..ebb350bc37 100644 >> --- a/xen/drivers/passthrough/Kconfig >> +++ b/xen/drivers/passthrough/Kconfig >> @@ -5,6 +5,7 @@ config HAS_PASSTHROUGH >> if ARM >> config ARM_SMMU >> bool "ARM SMMUv1 and v2 driver" >> + depends on MMU >> default y >> ---help--- >> Support for implementations of the ARM System MMU architecture >> @@ -15,7 +16,7 @@ config ARM_SMMU >> >> config ARM_SMMU_V3 >> bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support" if EXPERT >> - depends on ARM_64 && (!ACPI || BROKEN) >> + depends on ARM_64 && (!ACPI || BROKEN) && MMU >> ---help--- >> Support for implementations of the ARM System MMU architecture >> version 3. Driver is in experimental stage and should not be used in >
Hi, On 14/08/2023 05:25, Henry Wang wrote: > From: Penny Zheng <Penny.Zheng@arm.com> > > SMMU subsystem is only supported in MMU system, so we make it dependent > on CONFIG_HAS_MMU. "only supported" as in it doesn't work with Xen or the HW is not supporting it? Also, I am not entirely convinced that anything in passthrough would properly work with MPU. At least none of the IOMMU drivers are. So I would consider to completely disable HAS_PASSTHROUGH. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > Signed-off-by: Wei Chen <wei.chen@arm.com> > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > Acked-by: Jan Beulich <jbeulich@suse.com> > --- > v5: > - Add Acked-by tag from Jan. > v4: > - No change > v3: > - new patch > --- > xen/drivers/passthrough/Kconfig | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/xen/drivers/passthrough/Kconfig b/xen/drivers/passthrough/Kconfig > index 864fcf3b0c..ebb350bc37 100644 > --- a/xen/drivers/passthrough/Kconfig > +++ b/xen/drivers/passthrough/Kconfig > @@ -5,6 +5,7 @@ config HAS_PASSTHROUGH > if ARM > config ARM_SMMU > bool "ARM SMMUv1 and v2 driver" > + depends on MMU > default y > ---help--- > Support for implementations of the ARM System MMU architecture > @@ -15,7 +16,7 @@ config ARM_SMMU > > config ARM_SMMU_V3 > bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support" if EXPERT > - depends on ARM_64 && (!ACPI || BROKEN) > + depends on ARM_64 && (!ACPI || BROKEN) && MMU > ---help--- > Support for implementations of the ARM System MMU architecture > version 3. Driver is in experimental stage and should not be used in Cheers,
Hi Julien, > On Aug 22, 2023, at 05:34, Julien Grall <julien@xen.org> wrote: > > Hi, > > On 14/08/2023 05:25, Henry Wang wrote: >> From: Penny Zheng <Penny.Zheng@arm.com> >> SMMU subsystem is only supported in MMU system, so we make it dependent >> on CONFIG_HAS_MMU. > > "only supported" as in it doesn't work with Xen or the HW is not supporting it? I think currently there are no hardware combination of MPU + SMMU, but theoretically I think this is a valid combination since SMMU supports the linear mapping. So would below reword looks good to you: “Currently the hardware use case of connecting SMMU to MPU system is rarely seen, so we make CONFIG_ARM_SMMU and CONFIG_ARM_SMMU_V3 dependent on CONFIG_MMU." > > Also, I am not entirely convinced that anything in passthrough would properly work with MPU. At least none of the IOMMU drivers are. So I would consider to completely disable HAS_PASSTHROUGH. I agree, do you think adding below addition diff to this patch makes sense to you? If so I guess would also need to mention this in commit message. ``` diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index 8a7b79b4b5..fd29d14ed6 100644 --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -13,7 +13,7 @@ config ARM def_bool y select HAS_ALTERNATIVE select HAS_DEVICE_TREE - select HAS_PASSTHROUGH + select HAS_PASSTHROUGH if MMU select HAS_PDX select HAS_UBSAN select IOMMU_FORCE_PT_SHARE ``` Kind regards, Henry
On 22/08/2023 03:11, Henry Wang wrote: > Hi Julien, Hi, >> On Aug 22, 2023, at 05:34, Julien Grall <julien@xen.org> wrote: >> >> Hi, >> >> On 14/08/2023 05:25, Henry Wang wrote: >>> From: Penny Zheng <Penny.Zheng@arm.com> >>> SMMU subsystem is only supported in MMU system, so we make it dependent >>> on CONFIG_HAS_MMU. >> >> "only supported" as in it doesn't work with Xen or the HW is not supporting it? > > I think currently there are no hardware combination of MPU + SMMU, but > theoretically I think this is a valid combination since SMMU supports the linear > mapping. So would below reword looks good to you: > > “Currently the hardware use case of connecting SMMU to MPU system is rarely > seen, so we make CONFIG_ARM_SMMU and CONFIG_ARM_SMMU_V3 > dependent on CONFIG_MMU." I read this as there might be MPU system with SMMU in development. What you want to explain is why we can't let the developper to select the SMMU driver on an MPU system. From my understanding this is because the drivers are expecting to use the page-tables and the concept doesn't exist in the MPU system. So the drivers are not ready for the MPU. > >> >> Also, I am not entirely convinced that anything in passthrough would properly work with MPU. At least none of the IOMMU drivers are. So I would consider to completely disable HAS_PASSTHROUGH. > > I agree, do you think adding below addition diff to this patch makes sense to you? I think it should be a replacement because none of the IOMMU drivers works for the MPU. So I would rather prefer if we avoid adding "depends on" on all of them (even if there are only 3) for now. > If so I guess would also need to mention this in commit message. Did you confirm that Xen MPU still build without HAS_PASSTHROUGH? Cheers,
Hi Julien, > On Aug 22, 2023, at 16:18, Julien Grall <julien@xen.org> wrote: > > On 22/08/2023 03:11, Henry Wang wrote: >> Hi Julien, > > Hi, > >>> On Aug 22, 2023, at 05:34, Julien Grall <julien@xen.org> wrote: >>> >>> Hi, >>> >>> On 14/08/2023 05:25, Henry Wang wrote: >>>> From: Penny Zheng <Penny.Zheng@arm.com> >>>> SMMU subsystem is only supported in MMU system, so we make it dependent >>>> on CONFIG_HAS_MMU. >>> >>> "only supported" as in it doesn't work with Xen or the HW is not supporting it? >> I think currently there are no hardware combination of MPU + SMMU, but >> theoretically I think this is a valid combination since SMMU supports the linear >> mapping. So would below reword looks good to you: >> “Currently the hardware use case of connecting SMMU to MPU system is rarely >> seen, so we make CONFIG_ARM_SMMU and CONFIG_ARM_SMMU_V3 >> dependent on CONFIG_MMU." > > I read this as there might be MPU system with SMMU in development. What you want to explain is why we can't let the developper to select the SMMU driver on an MPU system. > > From my understanding this is because the drivers are expecting to use the page-tables and the concept doesn't exist in the MPU system. So the drivers are not ready for the MPU. I agree. > >>> >>> Also, I am not entirely convinced that anything in passthrough would properly work with MPU. At least none of the IOMMU drivers are. So I would consider to completely disable HAS_PASSTHROUGH. >> I agree, do you think adding below addition diff to this patch makes sense to you? > > I think it should be a replacement because none of the IOMMU drivers works for the MPU. So I would rather prefer if we avoid adding "depends on" on all of them (even if there are only 3) for now. I am a bit confused, I read your above explanation to the commit message as you agree with the idea that making SMMU driver not selectable by MPU system. If we replace this with “select HAS_PASSTHROUGH if MMU”, the SMMU driver will be selectable by MPU system. But... > >> If so I guess would also need to mention this in commit message. > > Did you confirm that Xen MPU still build without HAS_PASSTHROUGH? …this is a good catch, no MPU is not buildable without HAS_PASSTHROUGH, we will have: ``` In file included from ./include/xen/sched.h:12, from ./include/xen/iocap.h:10, from arch/arm/p2m.c:3: arch/arm/p2m.c: In function 'p2m_set_way_flush': ./include/xen/iommu.h:366:40: error: 'struct domain' has no member named 'iommu' 366 | #define dom_iommu(d) (&(d)->iommu) | ^~ ./include/xen/iommu.h:371:36: note: in expansion of macro 'dom_iommu' 371 | #define iommu_use_hap_pt(d) (dom_iommu(d)->hap_pt_share) | ^~~~~~~~~ arch/arm/p2m.c:617:10: note: in expansion of macro 'iommu_use_hap_pt' 617 | if ( iommu_use_hap_pt(current->domain) ) | ^~~~~~~~~~~~~~~~ In file included from ./include/xen/sched.h:12, from ./arch/arm/include/asm/grant_table.h:7, from ./include/xen/grant_table.h:29, from arch/arm/domain.c:4: arch/arm/domain.c: In function 'arch_domain_creation_finished': ./include/xen/iommu.h:366:40: error: 'struct domain' has no member named 'iommu' 366 | #define dom_iommu(d) (&(d)->iommu) | ^~ ./include/xen/iommu.h:371:36: note: in expansion of macro 'dom_iommu' 371 | #define iommu_use_hap_pt(d) (dom_iommu(d)->hap_pt_share) | ^~~~~~~~~ arch/arm/domain.c:880:11: note: in expansion of macro 'iommu_use_hap_pt' 880 | if ( !iommu_use_hap_pt(d) ) | ^~~~~~~~~~~~~~~~ CC arch/arm/shutdown.o CC lib/strlen.o CC arch/arm/smp.o CC arch/arm/smpboot.o CC common/xmalloc_tlsf.o CC lib/strncasecmp.o make[2]: *** [Rules.mk:247: arch/arm/p2m.o] Error 1 make[2]: *** Waiting for unfinished jobs.... ``` Kind regards, Henry > > Cheers, > > -- > Julien Grall
On 22/08/2023 09:48, Henry Wang wrote: > Hi Julien, > >> On Aug 22, 2023, at 16:18, Julien Grall <julien@xen.org> wrote: >> >> On 22/08/2023 03:11, Henry Wang wrote: >>> Hi Julien, >> >> Hi, >> >>>> On Aug 22, 2023, at 05:34, Julien Grall <julien@xen.org> wrote: >>>> >>>> Hi, >>>> >>>> On 14/08/2023 05:25, Henry Wang wrote: >>>>> From: Penny Zheng <Penny.Zheng@arm.com> >>>>> SMMU subsystem is only supported in MMU system, so we make it dependent >>>>> on CONFIG_HAS_MMU. >>>> >>>> "only supported" as in it doesn't work with Xen or the HW is not supporting it? >>> I think currently there are no hardware combination of MPU + SMMU, but >>> theoretically I think this is a valid combination since SMMU supports the linear >>> mapping. So would below reword looks good to you: >>> “Currently the hardware use case of connecting SMMU to MPU system is rarely >>> seen, so we make CONFIG_ARM_SMMU and CONFIG_ARM_SMMU_V3 >>> dependent on CONFIG_MMU." >> >> I read this as there might be MPU system with SMMU in development. What you want to explain is why we can't let the developper to select the SMMU driver on an MPU system. >> >> From my understanding this is because the drivers are expecting to use the page-tables and the concept doesn't exist in the MPU system. So the drivers are not ready for the MPU. > > I agree. > >> >>>> >>>> Also, I am not entirely convinced that anything in passthrough would properly work with MPU. At least none of the IOMMU drivers are. So I would consider to completely disable HAS_PASSTHROUGH. >>> I agree, do you think adding below addition diff to this patch makes sense to you? >> >> I think it should be a replacement because none of the IOMMU drivers works for the MPU. So I would rather prefer if we avoid adding "depends on" on all of them (even if there are only 3) for now. > > I am a bit confused, I read your above explanation to the commit message as you > agree with the idea that making SMMU driver not selectable by MPU system. No. I would rather prefer if HAS_PASSTHROUGH is completely disabled because I doubt the IOMMU infrastructure will work without any change for the MPU. So it sounds incorrect to enable HAS_PASSTHROUGH until one of you confirm it works. Cheers,
diff --git a/xen/drivers/passthrough/Kconfig b/xen/drivers/passthrough/Kconfig index 864fcf3b0c..ebb350bc37 100644 --- a/xen/drivers/passthrough/Kconfig +++ b/xen/drivers/passthrough/Kconfig @@ -5,6 +5,7 @@ config HAS_PASSTHROUGH if ARM config ARM_SMMU bool "ARM SMMUv1 and v2 driver" + depends on MMU default y ---help--- Support for implementations of the ARM System MMU architecture @@ -15,7 +16,7 @@ config ARM_SMMU config ARM_SMMU_V3 bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support" if EXPERT - depends on ARM_64 && (!ACPI || BROKEN) + depends on ARM_64 && (!ACPI || BROKEN) && MMU ---help--- Support for implementations of the ARM System MMU architecture version 3. Driver is in experimental stage and should not be used in