| Message ID | 2133922.nksBEtjI2q@avalon (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Dec 14, 2014 at 11:49:30PM +0000, Laurent Pinchart wrote: > Hi Will, Hi Laurent, > On Thursday 27 November 2014 11:51:14 Will Deacon wrote: > > This series introduces a generic IOMMU page table allocation framework, > > implements support for ARM long-descriptors and then ports the arm-smmu > > driver over to the new code. [...] > > All feedback welcome. > > I've successfully tested the patch set with the Renesas IPMMU-VMSA driver with > the following extension to the allocator. > > Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Wahey, that's really cool, thanks! I have a few minor comments on your patch below. If you don't object, then I can make them locally and include your patch on top of my v2 series? > From 4bebb7f3a5a48541d4c89ce7c61e6ff66686c3a9 Mon Sep 17 00:00:00 2001 > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > Date: Sun, 14 Dec 2014 23:34:50 +0200 > Subject: [PATCH] iommu: io-pgtable-arm: Add Non-Secure quirk > > The quirk causes the Non-Secure bit to be set in all page table entries. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/iommu/io-pgtable-arm.c | 7 +++++++ > drivers/iommu/io-pgtable.h | 3 +++ > 2 files changed, 10 insertions(+) > > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c > index 669e322a83a4..b6910e142734 100644 > --- a/drivers/iommu/io-pgtable-arm.c > +++ b/drivers/iommu/io-pgtable-arm.c > @@ -80,11 +80,13 @@ > #define ARM_LPAE_PTE_TYPE_TABLE 3 > #define ARM_LPAE_PTE_TYPE_PAGE 3 > > +#define ARM_LPAE_PTE_NSTABLE (((arm_lpae_iopte)1) << 63) > #define ARM_LPAE_PTE_XN (((arm_lpae_iopte)3) << 53) > #define ARM_LPAE_PTE_AF (((arm_lpae_iopte)1) << 10) > #define ARM_LPAE_PTE_SH_NS (((arm_lpae_iopte)0) << 8) > #define ARM_LPAE_PTE_SH_OS (((arm_lpae_iopte)2) << 8) > #define ARM_LPAE_PTE_SH_IS (((arm_lpae_iopte)3) << 8) > +#define ARM_LPAE_PTE_NS (((arm_lpae_iopte)1) << 5) > #define ARM_LPAE_PTE_VALID (((arm_lpae_iopte)1) << 0) > > #define ARM_LPAE_PTE_ATTR_LO_MASK (((arm_lpae_iopte)0x3ff) << 2) > @@ -201,6 +203,9 @@ static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data, > if (iopte_leaf(*ptep, lvl)) > return -EEXIST; > > + if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_NON_SECURE) > + pte |= ARM_LPAE_PTE_NS; > + > if (lvl == ARM_LPAE_MAX_LEVELS - 1) > pte |= ARM_LPAE_PTE_TYPE_PAGE; > else > @@ -244,6 +249,8 @@ static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long iova, > data->iop.cfg.tlb->flush_pgtable(cptep, 1UL << data->pg_shift, > cookie); > pte = __pa(cptep) | ARM_LPAE_PTE_TYPE_TABLE; > + if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_NON_SECURE) > + pte |= ARM_LPAE_PTE_NSTABLE; > *ptep = pte; > data->iop.cfg.tlb->flush_pgtable(ptep, sizeof(*ptep), cookie); > } else { > diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h > index c1cff3d045db..a41a15d30596 100644 > --- a/drivers/iommu/io-pgtable.h > +++ b/drivers/iommu/io-pgtable.h > @@ -24,6 +24,9 @@ struct iommu_gather_ops { > void (*flush_pgtable)(void *ptr, size_t size, void *cookie); > }; > > +/* Set the Non-Secure bit in the PTEs */ > +#define IO_PGTABLE_QUIRK_NON_SECURE (1 << 0) I think I'd stick an _ARM_ somewhere in here, so maybe IO_PGTABLE_QUIRK_ARM_NS? > + > struct io_pgtable_cfg { and I'd put the #define here, next to the member. > int quirks; /* IO_PGTABLE_QUIRK_* */ > unsigned long pgsize_bitmap; > -- Will
Hi Will, On Monday 15 December 2014 16:10:52 Will Deacon wrote: > On Sun, Dec 14, 2014 at 11:49:30PM +0000, Laurent Pinchart wrote: > > On Thursday 27 November 2014 11:51:14 Will Deacon wrote: > > > This series introduces a generic IOMMU page table allocation framework, > > > implements support for ARM long-descriptors and then ports the arm-smmu > > > driver over to the new code. > > [...] > > > > All feedback welcome. > > > > I've successfully tested the patch set with the Renesas IPMMU-VMSA driver > > with the following extension to the allocator. > > > > Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Wahey, that's really cool, thanks! I have a few minor comments on your patch > below. If you don't object, then I can make them locally and include your > patch on top of my v2 series? Sure. Please see my reply below. > > From 4bebb7f3a5a48541d4c89ce7c61e6ff66686c3a9 Mon Sep 17 00:00:00 2001 > > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > Date: Sun, 14 Dec 2014 23:34:50 +0200 > > Subject: [PATCH] iommu: io-pgtable-arm: Add Non-Secure quirk > > > > The quirk causes the Non-Secure bit to be set in all page table entries. > > > > Signed-off-by: Laurent Pinchart > > <laurent.pinchart+renesas@ideasonboard.com> > > --- > > > > drivers/iommu/io-pgtable-arm.c | 7 +++++++ > > drivers/iommu/io-pgtable.h | 3 +++ > > 2 files changed, 10 insertions(+) > > > > diff --git a/drivers/iommu/io-pgtable-arm.c > > b/drivers/iommu/io-pgtable-arm.c index 669e322a83a4..b6910e142734 100644 > > --- a/drivers/iommu/io-pgtable-arm.c > > +++ b/drivers/iommu/io-pgtable-arm.c > > @@ -80,11 +80,13 @@ > > > > #define ARM_LPAE_PTE_TYPE_TABLE 3 > > #define ARM_LPAE_PTE_TYPE_PAGE 3 > > +#define ARM_LPAE_PTE_NSTABLE (((arm_lpae_iopte)1) << 63) > > #define ARM_LPAE_PTE_XN (((arm_lpae_iopte)3) << 53) > > #define ARM_LPAE_PTE_AF (((arm_lpae_iopte)1) << 10) > > #define ARM_LPAE_PTE_SH_NS (((arm_lpae_iopte)0) << 8) > > #define ARM_LPAE_PTE_SH_OS (((arm_lpae_iopte)2) << 8) > > #define ARM_LPAE_PTE_SH_IS (((arm_lpae_iopte)3) << 8) > > +#define ARM_LPAE_PTE_NS (((arm_lpae_iopte)1) << 5) > > #define ARM_LPAE_PTE_VALID (((arm_lpae_iopte)1) << 0) > > #define ARM_LPAE_PTE_ATTR_LO_MASK (((arm_lpae_iopte)0x3ff) << 2) > > > > @@ -201,6 +203,9 @@ static int arm_lpae_init_pte(struct > > arm_lpae_io_pgtable *data,> > > if (iopte_leaf(*ptep, lvl)) > > return -EEXIST; > > > > + if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_NON_SECURE) > > + pte |= ARM_LPAE_PTE_NS; > > + > > if (lvl == ARM_LPAE_MAX_LEVELS - 1) > > pte |= ARM_LPAE_PTE_TYPE_PAGE; > > else > > @@ -244,6 +249,8 @@ static int __arm_lpae_map(struct arm_lpae_io_pgtable > > *data, unsigned long iova,> > > data->iop.cfg.tlb->flush_pgtable(cptep, 1UL << data->pg_shift, > > cookie); > > pte = __pa(cptep) | ARM_LPAE_PTE_TYPE_TABLE; > > + if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_NON_SECURE) > > + pte |= ARM_LPAE_PTE_NSTABLE; > > *ptep = pte; > > data->iop.cfg.tlb->flush_pgtable(ptep, sizeof(*ptep), cookie); > > } else { > > diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h > > index c1cff3d045db..a41a15d30596 100644 > > --- a/drivers/iommu/io-pgtable.h > > +++ b/drivers/iommu/io-pgtable.h > > @@ -24,6 +24,9 @@ struct iommu_gather_ops { > > void (*flush_pgtable)(void *ptr, size_t size, void *cookie); > > }; > > > > +/* Set the Non-Secure bit in the PTEs */ > > +#define IO_PGTABLE_QUIRK_NON_SECURE (1 << 0) > > I think I'd stick an _ARM_ somewhere in here, so maybe > IO_PGTABLE_QUIRK_ARM_NS? I'm fine with that. By the way, I'm only familiar with the Renesas implementation of the VMSA IOMMU, could you double-check whether setting the NSTABLE and NS bits on all levels make sense to you ? It seems to be required by my hardware, even though the ARM spec mentions that setting the NSTABLE bit causes non-secure accesses to page tables for all lower levels regardless of their NSTABLE/NS bits. > > + > > > > struct io_pgtable_cfg { > > and I'd put the #define here, next to the member. They're right before the structure so I don't think they're too far away, but if you prefer that coding style that's fine with me. > > int quirks; /* IO_PGTABLE_QUIRK_* */ > > unsigned long pgsize_bitmap;
On Mon, Dec 15, 2014 at 05:33:32PM +0000, Laurent Pinchart wrote: > On Monday 15 December 2014 16:10:52 Will Deacon wrote: > > On Sun, Dec 14, 2014 at 11:49:30PM +0000, Laurent Pinchart wrote: > > > diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h > > > index c1cff3d045db..a41a15d30596 100644 > > > --- a/drivers/iommu/io-pgtable.h > > > +++ b/drivers/iommu/io-pgtable.h > > > @@ -24,6 +24,9 @@ struct iommu_gather_ops { > > > void (*flush_pgtable)(void *ptr, size_t size, void *cookie); > > > }; > > > > > > +/* Set the Non-Secure bit in the PTEs */ > > > +#define IO_PGTABLE_QUIRK_NON_SECURE (1 << 0) > > > > I think I'd stick an _ARM_ somewhere in here, so maybe > > IO_PGTABLE_QUIRK_ARM_NS? > > I'm fine with that. > > By the way, I'm only familiar with the Renesas implementation of the VMSA > IOMMU, could you double-check whether setting the NSTABLE and NS bits on all > levels make sense to you ? It seems to be required by my hardware, even though > the ARM spec mentions that setting the NSTABLE bit causes non-secure accesses > to page tables for all lower levels regardless of their NSTABLE/NS bits. The ARM ARM is very clear that subsequent levels of lookup must ignore the NSTABLE/NS bits since otherwise you potentially have a security violation where you can use the table walker to access secure memory from non-secure... So, you might want to check up on that, but given that this is a quirk I'm happy for it to do whatever you need. Will
Hi Will, On Monday 15 December 2014 17:39:11 Will Deacon wrote: > On Mon, Dec 15, 2014 at 05:33:32PM +0000, Laurent Pinchart wrote: > > On Monday 15 December 2014 16:10:52 Will Deacon wrote: > > > On Sun, Dec 14, 2014 at 11:49:30PM +0000, Laurent Pinchart wrote: > > > > diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h > > > > index c1cff3d045db..a41a15d30596 100644 > > > > --- a/drivers/iommu/io-pgtable.h > > > > +++ b/drivers/iommu/io-pgtable.h > > > > @@ -24,6 +24,9 @@ struct iommu_gather_ops { > > > > > > > > void (*flush_pgtable)(void *ptr, size_t size, void *cookie); > > > > > > > > }; > > > > > > > > +/* Set the Non-Secure bit in the PTEs */ > > > > +#define IO_PGTABLE_QUIRK_NON_SECURE (1 << 0) > > > > > > I think I'd stick an _ARM_ somewhere in here, so maybe > > > IO_PGTABLE_QUIRK_ARM_NS? > > > > I'm fine with that. > > > > By the way, I'm only familiar with the Renesas implementation of the VMSA > > IOMMU, could you double-check whether setting the NSTABLE and NS bits on > > all levels make sense to you ? It seems to be required by my hardware, > > even though the ARM spec mentions that setting the NSTABLE bit causes > > non-secure accesses to page tables for all lower levels regardless of > > their NSTABLE/NS bits. > > The ARM ARM is very clear that subsequent levels of lookup must ignore the > NSTABLE/NS bits since otherwise you potentially have a security violation > where you can use the table walker to access secure memory from > non-secure... > > So, you might want to check up on that, but given that this is a quirk I'm > happy for it to do whatever you need. Given that the ARM ARM states that subsequent levels must consider the NSTABLE/NS bits to be set, I think it's harmless to actually set them. I just wanted to double-check with you, as we agree let's proceed with the proposed patch.
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c index 669e322a83a4..b6910e142734 100644 --- a/drivers/iommu/io-pgtable-arm.c +++ b/drivers/iommu/io-pgtable-arm.c @@ -80,11 +80,13 @@ #define ARM_LPAE_PTE_TYPE_TABLE 3 #define ARM_LPAE_PTE_TYPE_PAGE 3 +#define ARM_LPAE_PTE_NSTABLE (((arm_lpae_iopte)1) << 63) #define ARM_LPAE_PTE_XN (((arm_lpae_iopte)3) << 53) #define ARM_LPAE_PTE_AF (((arm_lpae_iopte)1) << 10) #define ARM_LPAE_PTE_SH_NS (((arm_lpae_iopte)0) << 8) #define ARM_LPAE_PTE_SH_OS (((arm_lpae_iopte)2) << 8) #define ARM_LPAE_PTE_SH_IS (((arm_lpae_iopte)3) << 8) +#define ARM_LPAE_PTE_NS (((arm_lpae_iopte)1) << 5) #define ARM_LPAE_PTE_VALID (((arm_lpae_iopte)1) << 0) #define ARM_LPAE_PTE_ATTR_LO_MASK (((arm_lpae_iopte)0x3ff) << 2) @@ -201,6 +203,9 @@ static int arm_lpae_init_pte(struct arm_lpae_io_pgtable *data, if (iopte_leaf(*ptep, lvl)) return -EEXIST; + if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_NON_SECURE) + pte |= ARM_LPAE_PTE_NS; + if (lvl == ARM_LPAE_MAX_LEVELS - 1) pte |= ARM_LPAE_PTE_TYPE_PAGE; else @@ -244,6 +249,8 @@ static int __arm_lpae_map(struct arm_lpae_io_pgtable *data, unsigned long iova, data->iop.cfg.tlb->flush_pgtable(cptep, 1UL << data->pg_shift, cookie); pte = __pa(cptep) | ARM_LPAE_PTE_TYPE_TABLE; + if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_NON_SECURE) + pte |= ARM_LPAE_PTE_NSTABLE; *ptep = pte; data->iop.cfg.tlb->flush_pgtable(ptep, sizeof(*ptep), cookie); } else { diff --git a/drivers/iommu/io-pgtable.h b/drivers/iommu/io-pgtable.h index c1cff3d045db..a41a15d30596 100644 --- a/drivers/iommu/io-pgtable.h +++ b/drivers/iommu/io-pgtable.h @@ -24,6 +24,9 @@ struct iommu_gather_ops { void (*flush_pgtable)(void *ptr, size_t size, void *cookie); }; +/* Set the Non-Secure bit in the PTEs */ +#define IO_PGTABLE_QUIRK_NON_SECURE (1 << 0) + struct io_pgtable_cfg { int quirks; /* IO_PGTABLE_QUIRK_* */ unsigned long pgsize_bitmap;