Re: [PATCH v5 13/13] xen/arm: mmu: enable SMMU subsystem only in MMU

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> On Aug 22, 2023, at 16:18, Julien Grall <julien@xxxxxxx> wrote:
> 
> On 22/08/2023 03:11, Henry Wang wrote:
>> Hi Julien,
> 
> Hi,
> 
>>> On Aug 22, 2023, at 05:34, Julien Grall <julien@xxxxxxx> wrote:
>>> 
>>> Hi,
>>> 
>>> On 14/08/2023 05:25, Henry Wang wrote:
>>>> From: Penny Zheng <Penny.Zheng@xxxxxxx>
>>>> 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