Re: [PATCH v1 2/2] xen/mmu: enable SMMU subsystem only in MMU

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Luca/Julien,

On 11/11/2024 16:33, Luca Fancellu wrote:
> Hi Ayan,
>
>
> > On 11 Nov 2024, at 16:00, Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:
> >
> >
> > On 11/11/2024 13:45, Julien Grall wrote:
> > > Hi Ayan,
> > Hi Julien,
> > > On 11/11/2024 13:24, Ayan Kumar Halder wrote:
> > > > On 11/11/2024 11:12, Julien Grall wrote:
> > > > > Hi,
> > > > Hi Julien,
> > > > > On 08/11/2024 19:59, Ayan Kumar Halder wrote:
> > > > > > From: Penny Zheng <Penny.Zheng@xxxxxxx>
> > > > > >
> > > > > > In Xen, SMMU subsystem is supported for MMU system only. The reason being SMMU
> > > > > > driver uses the same page tables as MMU.
> > > > > > Thus, we make it dependent on CONFIG_MMU.
> > > > > >
> > > > > > Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>
> > > > > > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > > > > > ---
> > > > > >    xen/arch/arm/Kconfig            | 2 +-
> > > > > >    xen/drivers/passthrough/Kconfig | 3 ++-
> > > > > >    2 files changed, 3 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > > > > > index 15b2e4a227..3699e148e9 100644
> > > > > > --- a/xen/arch/arm/Kconfig
> > > > > > +++ b/xen/arch/arm/Kconfig
> > > > > > @@ -16,7 +16,7 @@ config ARM
> > > > > >        select HAS_DEVICE_TREE
> > > > > >        select HAS_PASSTHROUGH
> > > > > >        select HAS_UBSAN
> > > > > > -    select IOMMU_FORCE_PT_SHARE
> > > > > > +    select IOMMU_FORCE_PT_SHARE if MMU
> > > > > Realistically, everything under drivers/passthrough is MMU specific. So does it 
> > > > > actually make any sense to select HAS_PASSTHROUGH right now?
> > > > Actually we are able to assign devices to different DomUs (eg UART1 to domU1) 
> > > > as long as the device isn't behind an IOMMU. So in our case, the passthrough 
> > > > device tree has this node
> > > >
> > > >           uart@9c0b0000 {
> > > >               compatible = "arm,pl011\0arm,primecell";
> > > >               reg = <0x00 0x9c0b0000 0x00 0x10000>;
> > > >               interrupt-parent = <0x01>;
> > > >               interrupts = <0x00 0x07 0x04>;
> > > >               clock-names = "uartclk\0apb_pclk";
> > > >               clocks = <0x06 0x07>;
> > > >               xen,path = "/uart@9c0b0000";
> > > >               xen,reg = <0x00 0x9c0b0000 0x00 0x10000 0x00 0x9c0b0000>;
> > > >               xen,force-assign-without-iommu;
> > > So how devices will be protected on an MPU systems?
> > >
> > > >           };> So, should we still disable HAS_PASSTHROUGH for MPU ?
> > > While it may work, a lot of code in drivers/passthrough is IOMMU specific (see 
> > > all the function named iommu_*). So I find really odd that you disable 
> > > IOMMU_FORCE_PT_SHARE but all the rest is still present...
> > >
> > > I think we need some consistency. If you are planning to do device passthrough 
> > > without any protection, then I don't think you need any code within 
> > > drivers/passthrough/ (at least for platform devices).
> > >
> > > Overall, for this patch, I think it would be better to simply select 
> > > HAS_PASSTHROUGH when MMU is enabled. We can revisit device passthrough once we 
> > > have the patches on the ML.
> > Yes, this makes sense. I will wait for Luca to confirm as well.
> It makes sense to don’t compile all that stuff, anyway we are using some 
> functions from drivers/passthrough/device_tree.c to pass the pl011 to the guests, we 
> will try to handle them later in the series then.
Agreed.

Actually, MPU does not use much from drivers/passthrough. However, if 
you disable CONFIG_HAS_PASSTHROUGH for MPU, then we need to add a bunch 
of #if-def in the arm specific common code.
Please have a look at "xen/arm: Enclose all iommu related access within 
CONFIG_HAS_PASSTHROUGH" and let me know if you still prefer to disable it.
- Ayan