| Message ID | 20240429032403.74910-10-smostafa@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SMMUv3 nested translation support | expand |
On 4/29/24 05:23, Mostafa Saleh wrote: > In the previous patch, comine_tlb() was added which combines 2 TLB combine > entries into one, which chooses the granule and level from the > smallest entry. > > This means that a nested translation, an entry can be cached with the that with nested translation > granule of stage-2 and not stage-1. > > However, the lookup for an IOVA in nested configuration is done with > stage-1 granule, this patch reworks lookup in that case, so it falls > back to stage-2 granule if no entry is found using stage-1 granule. I should have read that before commenting previous patch ;-) Anyway this shows that something is missing in previous patch, at least the above explanation ;-) Eric > > Signed-off-by: Mostafa Saleh <smostafa@google.com> > --- > hw/arm/smmu-common.c | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c > index 0d6945fa54..c67af3bc6d 100644 > --- a/hw/arm/smmu-common.c > +++ b/hw/arm/smmu-common.c > @@ -66,8 +66,10 @@ SMMUIOTLBKey smmu_get_iotlb_key(int asid, int vmid, uint64_t iova, > return key; > } > > -SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg, > - SMMUTransTableInfo *tt, hwaddr iova) > +static SMMUTLBEntry *smmu_iotlb_lookup_all_levels(SMMUState *bs, > + SMMUTransCfg *cfg, > + SMMUTransTableInfo *tt, > + hwaddr iova) > { > uint8_t tg = (tt->granule_sz - 10) / 2; > uint8_t inputsize = 64 - tt->tsz; > @@ -88,6 +90,24 @@ SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg, > } > level++; > } > + return entry; > +} > + > +SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg, > + SMMUTransTableInfo *tt, hwaddr iova) > +{ > + SMMUTLBEntry *entry = NULL; > + > + entry = smmu_iotlb_lookup_all_levels(bs, cfg, tt, iova); > + /* > + * For nested translation also try the s2 granule, as the TLB will insert > + * it if the size of s2 tlb entry was smaller. > + */ > + if (!entry && (cfg->stage == SMMU_NESTED) && > + (cfg->s2cfg.granule_sz != tt->granule_sz)) { > + tt->granule_sz = cfg->s2cfg.granule_sz; > + entry = smmu_iotlb_lookup_all_levels(bs, cfg, tt, iova); > + } > > if (entry) { > cfg->iotlb_hits++;
Hi Eric, On Wed, May 15, 2024 at 03:54:36PM +0200, Eric Auger wrote: > > > On 4/29/24 05:23, Mostafa Saleh wrote: > > In the previous patch, comine_tlb() was added which combines 2 TLB > combine Will do. > > entries into one, which chooses the granule and level from the > > smallest entry. > > > > This means that a nested translation, an entry can be cached with the > that with nested translation Will do. > > granule of stage-2 and not stage-1. > > > > However, the lookup for an IOVA in nested configuration is done with > > stage-1 granule, this patch reworks lookup in that case, so it falls > > back to stage-2 granule if no entry is found using stage-1 granule. > I should have read that before commenting previous patch ;-) > Anyway this shows that something is missing in previous patch, at least > the above explanation ;-) Yup, I can add a comment in the previous patch or reorder them, let me know what you prefer. Thanks, Mostafa > > Eric > > > > Signed-off-by: Mostafa Saleh <smostafa@google.com> > > --- > > hw/arm/smmu-common.c | 24 ++++++++++++++++++++++-- > > 1 file changed, 22 insertions(+), 2 deletions(-) > > > > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c > > index 0d6945fa54..c67af3bc6d 100644 > > --- a/hw/arm/smmu-common.c > > +++ b/hw/arm/smmu-common.c > > @@ -66,8 +66,10 @@ SMMUIOTLBKey smmu_get_iotlb_key(int asid, int vmid, uint64_t iova, > > return key; > > } > > > > -SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg, > > - SMMUTransTableInfo *tt, hwaddr iova) > > +static SMMUTLBEntry *smmu_iotlb_lookup_all_levels(SMMUState *bs, > > + SMMUTransCfg *cfg, > > + SMMUTransTableInfo *tt, > > + hwaddr iova) > > { > > uint8_t tg = (tt->granule_sz - 10) / 2; > > uint8_t inputsize = 64 - tt->tsz; > > @@ -88,6 +90,24 @@ SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg, > > } > > level++; > > } > > + return entry; > > +} > > + > > +SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg, > > + SMMUTransTableInfo *tt, hwaddr iova) > > +{ > > + SMMUTLBEntry *entry = NULL; > > + > > + entry = smmu_iotlb_lookup_all_levels(bs, cfg, tt, iova); > > + /* > > + * For nested translation also try the s2 granule, as the TLB will insert > > + * it if the size of s2 tlb entry was smaller. > > + */ > > + if (!entry && (cfg->stage == SMMU_NESTED) && > > + (cfg->s2cfg.granule_sz != tt->granule_sz)) { > > + tt->granule_sz = cfg->s2cfg.granule_sz; > > + entry = smmu_iotlb_lookup_all_levels(bs, cfg, tt, iova); > > + } > > > > if (entry) { > > cfg->iotlb_hits++; >
Hi Mostafa, On 4/29/24 05:23, Mostafa Saleh wrote: > In the previous patch, comine_tlb() was added which combines 2 TLB > entries into one, which chooses the granule and level from the > smallest entry. > > This means that a nested translation, an entry can be cached with the > granule of stage-2 and not stage-1. > > However, the lookup for an IOVA in nested configuration is done with > stage-1 granule, this patch reworks lookup in that case, so it falls > back to stage-2 granule if no entry is found using stage-1 granule. > > Signed-off-by: Mostafa Saleh <smostafa@google.com> > --- > hw/arm/smmu-common.c | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c > index 0d6945fa54..c67af3bc6d 100644 > --- a/hw/arm/smmu-common.c > +++ b/hw/arm/smmu-common.c > @@ -66,8 +66,10 @@ SMMUIOTLBKey smmu_get_iotlb_key(int asid, int vmid, uint64_t iova, > return key; > } > > -SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg, > - SMMUTransTableInfo *tt, hwaddr iova) > +static SMMUTLBEntry *smmu_iotlb_lookup_all_levels(SMMUState *bs, > + SMMUTransCfg *cfg, > + SMMUTransTableInfo *tt, > + hwaddr iova) > { > uint8_t tg = (tt->granule_sz - 10) / 2; > uint8_t inputsize = 64 - tt->tsz; > @@ -88,6 +90,24 @@ SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg, > } > level++; > } > + return entry; > +} > + > +SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg, > + SMMUTransTableInfo *tt, hwaddr iova) > +{ > + SMMUTLBEntry *entry = NULL; > + > + entry = smmu_iotlb_lookup_all_levels(bs, cfg, tt, iova); > + /* > + * For nested translation also try the s2 granule, as the TLB will insert > + * it if the size of s2 tlb entry was smaller. > + */ > + if (!entry && (cfg->stage == SMMU_NESTED) && > + (cfg->s2cfg.granule_sz != tt->granule_sz)) { > + tt->granule_sz = cfg->s2cfg.granule_sz; is it safe to alter the tt->granule_sz without restoring it? In the positive I think this would deserve a comment. Eric > + entry = smmu_iotlb_lookup_all_levels(bs, cfg, tt, iova); > + } > > if (entry) { > cfg->iotlb_hits++;
Hi Eric, On Mon, May 20, 2024 at 10:27:50AM +0200, Eric Auger wrote: > Hi Mostafa, > > On 4/29/24 05:23, Mostafa Saleh wrote: > > In the previous patch, comine_tlb() was added which combines 2 TLB > > entries into one, which chooses the granule and level from the > > smallest entry. > > > > This means that a nested translation, an entry can be cached with the > > granule of stage-2 and not stage-1. > > > > However, the lookup for an IOVA in nested configuration is done with > > stage-1 granule, this patch reworks lookup in that case, so it falls > > back to stage-2 granule if no entry is found using stage-1 granule. > > > > Signed-off-by: Mostafa Saleh <smostafa@google.com> > > --- > > hw/arm/smmu-common.c | 24 ++++++++++++++++++++++-- > > 1 file changed, 22 insertions(+), 2 deletions(-) > > > > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c > > index 0d6945fa54..c67af3bc6d 100644 > > --- a/hw/arm/smmu-common.c > > +++ b/hw/arm/smmu-common.c > > @@ -66,8 +66,10 @@ SMMUIOTLBKey smmu_get_iotlb_key(int asid, int vmid, uint64_t iova, > > return key; > > } > > > > -SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg, > > - SMMUTransTableInfo *tt, hwaddr iova) > > +static SMMUTLBEntry *smmu_iotlb_lookup_all_levels(SMMUState *bs, > > + SMMUTransCfg *cfg, > > + SMMUTransTableInfo *tt, > > + hwaddr iova) > > { > > uint8_t tg = (tt->granule_sz - 10) / 2; > > uint8_t inputsize = 64 - tt->tsz; > > @@ -88,6 +90,24 @@ SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg, > > } > > level++; > > } > > + return entry; > > +} > > + > > +SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg, > > + SMMUTransTableInfo *tt, hwaddr iova) > > +{ > > + SMMUTLBEntry *entry = NULL; > > + > > + entry = smmu_iotlb_lookup_all_levels(bs, cfg, tt, iova); > > + /* > > + * For nested translation also try the s2 granule, as the TLB will insert > > + * it if the size of s2 tlb entry was smaller. > > + */ > > + if (!entry && (cfg->stage == SMMU_NESTED) && > > + (cfg->s2cfg.granule_sz != tt->granule_sz)) { > > + tt->granule_sz = cfg->s2cfg.granule_sz; > is it safe to alter the tt->granule_sz without restoring it? In the > positive I think this would deserve a comment. It should be safe in the current usage, I will add a comment to clarify how the function behaves (something as the the granule_sz would be updated to the entry tg if found) Thanks, Mostafa > > Eric > > + entry = smmu_iotlb_lookup_all_levels(bs, cfg, tt, iova); > > + } > > > > if (entry) { > > cfg->iotlb_hits++; >
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c index 0d6945fa54..c67af3bc6d 100644 --- a/hw/arm/smmu-common.c +++ b/hw/arm/smmu-common.c @@ -66,8 +66,10 @@ SMMUIOTLBKey smmu_get_iotlb_key(int asid, int vmid, uint64_t iova, return key; } -SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg, - SMMUTransTableInfo *tt, hwaddr iova) +static SMMUTLBEntry *smmu_iotlb_lookup_all_levels(SMMUState *bs, + SMMUTransCfg *cfg, + SMMUTransTableInfo *tt, + hwaddr iova) { uint8_t tg = (tt->granule_sz - 10) / 2; uint8_t inputsize = 64 - tt->tsz; @@ -88,6 +90,24 @@ SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg, } level++; } + return entry; +} + +SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg, + SMMUTransTableInfo *tt, hwaddr iova) +{ + SMMUTLBEntry *entry = NULL; + + entry = smmu_iotlb_lookup_all_levels(bs, cfg, tt, iova); + /* + * For nested translation also try the s2 granule, as the TLB will insert + * it if the size of s2 tlb entry was smaller. + */ + if (!entry && (cfg->stage == SMMU_NESTED) && + (cfg->s2cfg.granule_sz != tt->granule_sz)) { + tt->granule_sz = cfg->s2cfg.granule_sz; + entry = smmu_iotlb_lookup_all_levels(bs, cfg, tt, iova); + } if (entry) { cfg->iotlb_hits++;
In the previous patch, comine_tlb() was added which combines 2 TLB entries into one, which chooses the granule and level from the smallest entry. This means that a nested translation, an entry can be cached with the granule of stage-2 and not stage-1. However, the lookup for an IOVA in nested configuration is done with stage-1 granule, this patch reworks lookup in that case, so it falls back to stage-2 granule if no entry is found using stage-1 granule. Signed-off-by: Mostafa Saleh <smostafa@google.com> --- hw/arm/smmu-common.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)