Re: [PATCH v3 3/3] xen/iommu: arm: Use p2m_ipa_bits as stage2 input size

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Thu, Apr 30, 2015 at 04:07:27PM +0100, Julien Grall wrote:
> Hi Edgar,
> 
> On 30/04/15 12:55, Edgar E. Iglesias wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> > 
> > The Stage2 input-size must match what the CPU uses because
> > the SMMU and the CPU share page-tables.
> > 
> > Assert that the SMMU supports the P2M IPA bit size and use it.
> > 
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  xen/drivers/passthrough/arm/smmu.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> > index 8a9b58b..5356046 100644
> > --- a/xen/drivers/passthrough/arm/smmu.c
> > +++ b/xen/drivers/passthrough/arm/smmu.c
> > @@ -2230,8 +2230,9 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
> >  	size = arm_smmu_id_size_to_bits((id >> ID2_IAS_SHIFT) & ID2_IAS_MASK);
> >  	smmu->s1_output_size = min_t(unsigned long, PHYS_MASK_SHIFT, size);
> >  
> > -	/* Xen: Stage-2 input size is not restricted */
> > -	smmu->s2_input_size = size;
> > +	/* Xen: Stage-2 input size has to match p2m_ipa_bits.  */
> > +	ASSERT(size >= p2m_ipa_bits);
> 
> Sorry, I was planning to comment on this patch in the previous version
> later but you send the new version quickly.

No worries, thanks for reviewing. I'll fix up the comments and send a new version.

> 
> In general, ASSERT should only be used when you think that the condition
> is always true. Although, we have some hardware where we know that this
> condition will be hit.
> 
> On non-debug, we would get an odd error later because ASSERT is turned
> into a no-op.
> 
> As this is a restriction of the driver we should print a error message
> and return an appropriate error value.
> 
> The generic IOMMU driver can then decide if it's safe to continue
> without the SMMU setup or panic.
> 
> FWIW, currently we use the later. I will send a patch to panic avoiding
> the user to think the SMMU is correctly setup.

This is what it looks like in my new version when starting XEN and forcing
a bad SMMU IPA size:

(XEN) P2M: 40-bit IPA with 40-bit PA
(XEN) P2M: 3 levels with order-1 root, VTCR 0x80023558
(XEN) smmu: /amba/smmu0@0xFD800000: probing hardware configuration...
(XEN) smmu: /amba/smmu0@0xFD800000: SMMUv2 with:
(XEN) smmu: /amba/smmu0@0xFD800000:     stage 2 translation
(XEN) smmu: /amba/smmu0@0xFD800000:     stream matching with 48 register groups, mask 0x7fff
(XEN) smmu: /amba/smmu0@0xFD800000:     16 context banks (0 stage-2 only)
(XEN) smmu: /amba/smmu0@0xFD800000: P2M IPA size not supported (P2M=40 SMMU=36)!
(XEN) I/O virtualisation disabled
(XEN) *** LOADING DOMAIN 0 ***

Dom0 boots fine but without IOMMU protections...

Cheers,
Edgar