| Message ID | 20230205094411.793816-13-smostafa@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add stage-2 translation for SMMUv3 | expand |
Hi Mostafa, On Sun, Feb 05, 2023 at 09:44:07AM +0000, Mostafa Saleh wrote: > Allow TLB to be tagged with VMID. > > If stage-1 is only supported, VMID is set to -1 and ignored from STE > and CMD_TLBI_NH* cmds. > > Signed-off-by: Mostafa Saleh <smostafa@google.com> > --- > hw/arm/smmu-common.c | 24 +++++++++++++++--------- > hw/arm/smmu-internal.h | 2 ++ > hw/arm/smmuv3.c | 12 +++++++++--- > include/hw/arm/smmu-common.h | 5 +++-- > 4 files changed, 29 insertions(+), 14 deletions(-) > > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c > index 541c427684..028a60949a 100644 > --- a/hw/arm/smmu-common.c > +++ b/hw/arm/smmu-common.c > @@ -56,10 +56,11 @@ static gboolean smmu_iotlb_key_equal(gconstpointer v1, gconstpointer v2) > (k1->level == k2->level) && (k1->tg == k2->tg); I'm getting some aliasing in the TLB, because smmu_iotlb_key_equal() is missing the VMID comparison. With that fixed my handful of tests pass Thanks, Jean
Hi, On 2/5/23 10:44, Mostafa Saleh wrote: > Allow TLB to be tagged with VMID. > > If stage-1 is only supported, VMID is set to -1 and ignored from STE > and CMD_TLBI_NH* cmds. > > Signed-off-by: Mostafa Saleh <smostafa@google.com> > --- > hw/arm/smmu-common.c | 24 +++++++++++++++--------- > hw/arm/smmu-internal.h | 2 ++ > hw/arm/smmuv3.c | 12 +++++++++--- > include/hw/arm/smmu-common.h | 5 +++-- > 4 files changed, 29 insertions(+), 14 deletions(-) > > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c > index 541c427684..028a60949a 100644 > --- a/hw/arm/smmu-common.c > +++ b/hw/arm/smmu-common.c > @@ -56,10 +56,11 @@ static gboolean smmu_iotlb_key_equal(gconstpointer v1, gconstpointer v2) > (k1->level == k2->level) && (k1->tg == k2->tg); > } > > -SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint64_t iova, > +SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova, > uint8_t tg, uint8_t level) > { > - SMMUIOTLBKey key = {.asid = asid, .iova = iova, .tg = tg, .level = level}; > + SMMUIOTLBKey key = {.asid = asid, .vmid = vmid, .iova = iova, > + .tg = tg, .level = level}; > > return key; > } > @@ -78,7 +79,8 @@ SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg, > uint64_t mask = subpage_size - 1; > SMMUIOTLBKey key; > > - key = smmu_get_iotlb_key(cfg->asid, iova & ~mask, tg, level); > + key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid, > + iova & ~mask, tg, level); > entry = g_hash_table_lookup(bs->iotlb, &key); > if (entry) { > break; > @@ -111,7 +113,8 @@ void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *new) > smmu_iotlb_inv_all(bs); > } > > - *key = smmu_get_iotlb_key(cfg->asid, new->entry.iova, tg, new->level); > + *key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid, new->entry.iova, > + tg, new->level); > trace_smmu_iotlb_insert(cfg->asid, new->entry.iova, tg, new->level); you may update the trace point as well Thanks Erric > g_hash_table_insert(bs->iotlb, key, new); > } > @@ -130,8 +133,7 @@ static gboolean smmu_hash_remove_by_asid(gpointer key, gpointer value, > > return SMMU_IOTLB_ASID(*iotlb_key) == asid; > } > - > -static gboolean smmu_hash_remove_by_asid_iova(gpointer key, gpointer value, > +static gboolean smmu_hash_remove_by_asid_vmid_iova(gpointer key, gpointer value, > gpointer user_data) > { > SMMUTLBEntry *iter = (SMMUTLBEntry *)value; > @@ -142,18 +144,21 @@ static gboolean smmu_hash_remove_by_asid_iova(gpointer key, gpointer value, > if (info->asid >= 0 && info->asid != SMMU_IOTLB_ASID(iotlb_key)) { > return false; > } > + if (info->vmid >= 0 && info->vmid != SMMU_IOTLB_VMID(iotlb_key)) { > + return false; > + } > return ((info->iova & ~entry->addr_mask) == entry->iova) || > ((entry->iova & ~info->mask) == info->iova); > } > > -void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova, > +void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova, > uint8_t tg, uint64_t num_pages, uint8_t ttl) > { > /* if tg is not set we use 4KB range invalidation */ > uint8_t granule = tg ? tg * 2 + 10 : 12; > > if (ttl && (num_pages == 1) && (asid >= 0)) { > - SMMUIOTLBKey key = smmu_get_iotlb_key(asid, iova, tg, ttl); > + SMMUIOTLBKey key = smmu_get_iotlb_key(asid, vmid, iova, tg, ttl); > > if (g_hash_table_remove(s->iotlb, &key)) { > return; > @@ -166,10 +171,11 @@ void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova, > > SMMUIOTLBPageInvInfo info = { > .asid = asid, .iova = iova, > + .vmid = vmid, > .mask = (num_pages * 1 << granule) - 1}; > > g_hash_table_foreach_remove(s->iotlb, > - smmu_hash_remove_by_asid_iova, > + smmu_hash_remove_by_asid_vmid_iova, > &info); > } > > diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h > index 7d3f76ce14..3a14e5dca5 100644 > --- a/hw/arm/smmu-internal.h > +++ b/hw/arm/smmu-internal.h > @@ -136,9 +136,11 @@ static inline int pgd_idx(int start_level, int granule, dma_addr_t iova) > } > > #define SMMU_IOTLB_ASID(key) ((key).asid) > +#define SMMU_IOTLB_VMID(key) ((key).vmid) > > typedef struct SMMUIOTLBPageInvInfo { > int asid; > + int vmid; > uint64_t iova; > uint64_t mask; > } SMMUIOTLBPageInvInfo; > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > index 35a0149bbf..8b070f6bb5 100644 > --- a/hw/arm/smmuv3.c > +++ b/hw/arm/smmuv3.c > @@ -986,7 +986,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd) > { > dma_addr_t end, addr = CMD_ADDR(cmd); > uint8_t type = CMD_TYPE(cmd); > - uint16_t vmid = CMD_VMID(cmd); > + int vmid = -1; > uint8_t scale = CMD_SCALE(cmd); > uint8_t num = CMD_NUM(cmd); > uint8_t ttl = CMD_TTL(cmd); > @@ -995,6 +995,12 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd) > uint64_t num_pages; > uint8_t granule; > int asid = -1; > + SMMUv3State *smmuv3 = ARM_SMMUV3(s); > + > + /* Only consider VMID if stage-2 is supported. */ > + if (STAGE2_SUPPORTED(smmuv3->features)) { > + vmid = CMD_VMID(cmd); > + } > > if (type == SMMU_CMD_TLBI_NH_VA) { > asid = CMD_ASID(cmd); > @@ -1003,7 +1009,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd) > if (!tg) { > trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, 1, ttl, leaf); > smmuv3_inv_notifiers_iova(s, asid, addr, tg, 1); > - smmu_iotlb_inv_iova(s, asid, addr, tg, 1, ttl); > + smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl); > return; > } > > @@ -1021,7 +1027,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd) > num_pages = (mask + 1) >> granule; > trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, num_pages, ttl, leaf); > smmuv3_inv_notifiers_iova(s, asid, addr, tg, num_pages); > - smmu_iotlb_inv_iova(s, asid, addr, tg, num_pages, ttl); > + smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl); > addr += mask + 1; > } > } > diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h > index 7906e359d9..5cca1c17f5 100644 > --- a/include/hw/arm/smmu-common.h > +++ b/include/hw/arm/smmu-common.h > @@ -113,6 +113,7 @@ typedef struct SMMUPciBus { > typedef struct SMMUIOTLBKey { > uint64_t iova; > uint16_t asid; > + uint16_t vmid; > uint8_t tg; > uint8_t level; > } SMMUIOTLBKey; > @@ -176,11 +177,11 @@ IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid); > SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg, > SMMUTransTableInfo *tt, hwaddr iova); > void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *entry); > -SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint64_t iova, > +SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova, > uint8_t tg, uint8_t level); > void smmu_iotlb_inv_all(SMMUState *s); > void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid); > -void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova, > +void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova, > uint8_t tg, uint64_t num_pages, uint8_t ttl); > > /* Unmap the range of all the notifiers registered to any IOMMU mr */
Hi Jean, Thanks for taking the time to check the patches. On Wed, Feb 15, 2023 at 07:47:18PM +0000, Jean-Philippe Brucker wrote: > > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c > > index 541c427684..028a60949a 100644 > > --- a/hw/arm/smmu-common.c > > +++ b/hw/arm/smmu-common.c > > @@ -56,10 +56,11 @@ static gboolean smmu_iotlb_key_equal(gconstpointer v1, gconstpointer v2) > > (k1->level == k2->level) && (k1->tg == k2->tg); > > I'm getting some aliasing in the TLB, because smmu_iotlb_key_equal() is > missing the VMID comparison. With that fixed my handful of tests pass > Oh, I missed that, I will update it in v2. Thanks, Mostafa
Hi Eric, On Thu, Feb 16, 2023 at 11:17:34AM +0100, Eric Auger wrote: > > } > > > > - *key = smmu_get_iotlb_key(cfg->asid, new->entry.iova, tg, new->level); > > + *key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid, new->entry.iova, > > + tg, new->level); > > trace_smmu_iotlb_insert(cfg->asid, new->entry.iova, tg, new->level); > you may update the trace point as well I can add a stage argument here, however I don’t think it is necessary now as TLBs are either used for stage-1 or stage-2, not both. Thanks, Mostafa
On 2/16/23 14:53, Mostafa Saleh wrote: > Hi Eric, > > On Thu, Feb 16, 2023 at 11:17:34AM +0100, Eric Auger wrote: >>> } >>> >>> - *key = smmu_get_iotlb_key(cfg->asid, new->entry.iova, tg, new->level); >>> + *key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid, new->entry.iova, >>> + tg, new->level); >>> trace_smmu_iotlb_insert(cfg->asid, new->entry.iova, tg, new->level); >> you may update the trace point as well > I can add a stage argument here, however I don’t think it is necessary > now as TLBs are either used for stage-1 or stage-2, not both. no I meant adding the vmid value there Eric > > Thanks, > Mostafa >
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c index 541c427684..028a60949a 100644 --- a/hw/arm/smmu-common.c +++ b/hw/arm/smmu-common.c @@ -56,10 +56,11 @@ static gboolean smmu_iotlb_key_equal(gconstpointer v1, gconstpointer v2) (k1->level == k2->level) && (k1->tg == k2->tg); } -SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint64_t iova, +SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova, uint8_t tg, uint8_t level) { - SMMUIOTLBKey key = {.asid = asid, .iova = iova, .tg = tg, .level = level}; + SMMUIOTLBKey key = {.asid = asid, .vmid = vmid, .iova = iova, + .tg = tg, .level = level}; return key; } @@ -78,7 +79,8 @@ SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg, uint64_t mask = subpage_size - 1; SMMUIOTLBKey key; - key = smmu_get_iotlb_key(cfg->asid, iova & ~mask, tg, level); + key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid, + iova & ~mask, tg, level); entry = g_hash_table_lookup(bs->iotlb, &key); if (entry) { break; @@ -111,7 +113,8 @@ void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *new) smmu_iotlb_inv_all(bs); } - *key = smmu_get_iotlb_key(cfg->asid, new->entry.iova, tg, new->level); + *key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid, new->entry.iova, + tg, new->level); trace_smmu_iotlb_insert(cfg->asid, new->entry.iova, tg, new->level); g_hash_table_insert(bs->iotlb, key, new); } @@ -130,8 +133,7 @@ static gboolean smmu_hash_remove_by_asid(gpointer key, gpointer value, return SMMU_IOTLB_ASID(*iotlb_key) == asid; } - -static gboolean smmu_hash_remove_by_asid_iova(gpointer key, gpointer value, +static gboolean smmu_hash_remove_by_asid_vmid_iova(gpointer key, gpointer value, gpointer user_data) { SMMUTLBEntry *iter = (SMMUTLBEntry *)value; @@ -142,18 +144,21 @@ static gboolean smmu_hash_remove_by_asid_iova(gpointer key, gpointer value, if (info->asid >= 0 && info->asid != SMMU_IOTLB_ASID(iotlb_key)) { return false; } + if (info->vmid >= 0 && info->vmid != SMMU_IOTLB_VMID(iotlb_key)) { + return false; + } return ((info->iova & ~entry->addr_mask) == entry->iova) || ((entry->iova & ~info->mask) == info->iova); } -void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova, +void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova, uint8_t tg, uint64_t num_pages, uint8_t ttl) { /* if tg is not set we use 4KB range invalidation */ uint8_t granule = tg ? tg * 2 + 10 : 12; if (ttl && (num_pages == 1) && (asid >= 0)) { - SMMUIOTLBKey key = smmu_get_iotlb_key(asid, iova, tg, ttl); + SMMUIOTLBKey key = smmu_get_iotlb_key(asid, vmid, iova, tg, ttl); if (g_hash_table_remove(s->iotlb, &key)) { return; @@ -166,10 +171,11 @@ void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova, SMMUIOTLBPageInvInfo info = { .asid = asid, .iova = iova, + .vmid = vmid, .mask = (num_pages * 1 << granule) - 1}; g_hash_table_foreach_remove(s->iotlb, - smmu_hash_remove_by_asid_iova, + smmu_hash_remove_by_asid_vmid_iova, &info); } diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h index 7d3f76ce14..3a14e5dca5 100644 --- a/hw/arm/smmu-internal.h +++ b/hw/arm/smmu-internal.h @@ -136,9 +136,11 @@ static inline int pgd_idx(int start_level, int granule, dma_addr_t iova) } #define SMMU_IOTLB_ASID(key) ((key).asid) +#define SMMU_IOTLB_VMID(key) ((key).vmid) typedef struct SMMUIOTLBPageInvInfo { int asid; + int vmid; uint64_t iova; uint64_t mask; } SMMUIOTLBPageInvInfo; diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c index 35a0149bbf..8b070f6bb5 100644 --- a/hw/arm/smmuv3.c +++ b/hw/arm/smmuv3.c @@ -986,7 +986,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd) { dma_addr_t end, addr = CMD_ADDR(cmd); uint8_t type = CMD_TYPE(cmd); - uint16_t vmid = CMD_VMID(cmd); + int vmid = -1; uint8_t scale = CMD_SCALE(cmd); uint8_t num = CMD_NUM(cmd); uint8_t ttl = CMD_TTL(cmd); @@ -995,6 +995,12 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd) uint64_t num_pages; uint8_t granule; int asid = -1; + SMMUv3State *smmuv3 = ARM_SMMUV3(s); + + /* Only consider VMID if stage-2 is supported. */ + if (STAGE2_SUPPORTED(smmuv3->features)) { + vmid = CMD_VMID(cmd); + } if (type == SMMU_CMD_TLBI_NH_VA) { asid = CMD_ASID(cmd); @@ -1003,7 +1009,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd) if (!tg) { trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, 1, ttl, leaf); smmuv3_inv_notifiers_iova(s, asid, addr, tg, 1); - smmu_iotlb_inv_iova(s, asid, addr, tg, 1, ttl); + smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl); return; } @@ -1021,7 +1027,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd) num_pages = (mask + 1) >> granule; trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, num_pages, ttl, leaf); smmuv3_inv_notifiers_iova(s, asid, addr, tg, num_pages); - smmu_iotlb_inv_iova(s, asid, addr, tg, num_pages, ttl); + smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl); addr += mask + 1; } } diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h index 7906e359d9..5cca1c17f5 100644 --- a/include/hw/arm/smmu-common.h +++ b/include/hw/arm/smmu-common.h @@ -113,6 +113,7 @@ typedef struct SMMUPciBus { typedef struct SMMUIOTLBKey { uint64_t iova; uint16_t asid; + uint16_t vmid; uint8_t tg; uint8_t level; } SMMUIOTLBKey; @@ -176,11 +177,11 @@ IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid); SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg, SMMUTransTableInfo *tt, hwaddr iova); void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *entry); -SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint64_t iova, +SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova, uint8_t tg, uint8_t level); void smmu_iotlb_inv_all(SMMUState *s); void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid); -void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova, +void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova, uint8_t tg, uint64_t num_pages, uint8_t ttl); /* Unmap the range of all the notifiers registered to any IOMMU mr */
Allow TLB to be tagged with VMID. If stage-1 is only supported, VMID is set to -1 and ignored from STE and CMD_TLBI_NH* cmds. Signed-off-by: Mostafa Saleh <smostafa@google.com> --- hw/arm/smmu-common.c | 24 +++++++++++++++--------- hw/arm/smmu-internal.h | 2 ++ hw/arm/smmuv3.c | 12 +++++++++--- include/hw/arm/smmu-common.h | 5 +++-- 4 files changed, 29 insertions(+), 14 deletions(-)