Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

2023-03-21 Thread Peter Maydell
On Tue, 21 Mar 2023 at 13:55, Mostafa Saleh  wrote:
>
> Hi Peter,
>
> On Tue, Mar 21, 2023 at 01:34:55PM +, Peter Maydell wrote:
> > > >>> + */
> > > >>> +ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
> > > >>> +uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, 
> > > >>> ID_AA64MMFR0, PARANGE);
> > > >> is this working in accelerated mode?
> > > > I didn't try with accel, I will give it a try, but from what I see, that
> > > > ARM_CPU() is used to get the CPU in traget/arm/kvm.c which is used from
> > > > accel/kvm-all.c, so it seems this would work for accelerated mode.
> > >
> > > yeah I ma not familiar enough with that code but it is worth to be 
> > > checked.
> >
> > I'm a bit unsure about fishing around in the CPU ID registers for this.
> > That's not what you would do in real hardware, you'd just say "the
> > system is supposed to configure the CPU and the SMMU correctly".
> >
> > Also, there is no guarantee that CPU 0 is related to this SMMU in
> > particular.
> Sorry, missed this point in last email.
>
> So, we leave it this way, or there is a better way to discover PARANGE?

If you really need to know it, put a QOM property on the SMMU device
and have the board code set it. (This is analogous to how it works
in hardware: there are tie-off signals on the SMMU for the OAS value.)

-- PMM



Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

2023-03-21 Thread Mostafa Saleh
Hi Peter,

On Tue, Mar 21, 2023 at 01:34:55PM +, Peter Maydell wrote:
> > >>> + */
> > >>> +ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
> > >>> +uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, ID_AA64MMFR0, 
> > >>> PARANGE);
> > >> is this working in accelerated mode?
> > > I didn't try with accel, I will give it a try, but from what I see, that
> > > ARM_CPU() is used to get the CPU in traget/arm/kvm.c which is used from
> > > accel/kvm-all.c, so it seems this would work for accelerated mode.
> >
> > yeah I ma not familiar enough with that code but it is worth to be checked.
> 
> I'm a bit unsure about fishing around in the CPU ID registers for this.
> That's not what you would do in real hardware, you'd just say "the
> system is supposed to configure the CPU and the SMMU correctly".
> 
> Also, there is no guarantee that CPU 0 is related to this SMMU in
> particular.
Sorry, missed this point in last email.

So, we leave it this way, or there is a better way to discover PARANGE?

Thanks,
Mostafa




Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

2023-03-21 Thread Eric Auger
Hi Peter,

On 3/21/23 14:34, Peter Maydell wrote:
> thout having read much of the context, but why
> would we need to migrate the ID registers? They are constant, read-only,
> so they will be the same value on both source and destination.
this series modifies the values of IDR[5] (oas).  So my understanding is
the guest is likely to behave differently on src and dst, depending on
the qemu version, no?

Thanks

Eric




Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

2023-03-21 Thread Mostafa Saleh
Hi Peter,

On Tue, Mar 21, 2023 at 01:34:55PM +, Peter Maydell wrote:
> > >>> +s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
> > >> I am not sure you can change that easily. In case of migration this is
> > >> going to change the behavior of the device, no?
> > > I see IDR registers are not migrated. I guess we can add them in a
> > > subsection and if they were not passed (old instances) we set OAS to
> > > 44.
> > > Maybe this should be another change outside of this series.
> > Indeed tehy are not migrated so it can lead to inconsistent behavior in
> > both source and dest. This deserves more analysis to me. In case you
> > would decide to migrate IDR regs this would need to be done in that
> > series I think. Migration must not be broken by this series.
> 
> Jumping in here without having read much of the context, but why
> would we need to migrate the ID registers? They are constant, read-only,
> so they will be the same value on both source and destination.

Currently OAS for SMMU is hardcoded to 44 bits, and the SMMU manual says
"OAS reflects the maximum usable PA output from the last stage of
AArch64 translations, and must match the system physical address size.
The OAS is discoverable from SMMU_IDR5.OAS."
This patch implements OAS based on CPU PARANGE, but this would break
migration from old instances that ran with 44 bits OAS to new code that
configures it based on the current CPU.
So one idea is to migrate the IDRs (or atleast IDR5).
Maybe that is not the best solution, we just need a way to know if the
old instance needs to be 44 bits or not.


Thanks,
Mostafa



Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

2023-03-21 Thread Peter Maydell
On Tue, 21 Mar 2023 at 13:23, Eric Auger  wrote:
>
> Hi Mostafa,
>
> On 3/21/23 14:06, Mostafa Saleh wrote:
> > Hi Eric,
> >
> >>> + * According to 6.3.6 SMMU_IDR5, OAS must match the system physical 
> >>> address
> >>> + * size.
> >>> + */
> >>> +ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
> >>> +uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, ID_AA64MMFR0, 
> >>> PARANGE);
> >> is this working in accelerated mode?
> > I didn't try with accel, I will give it a try, but from what I see, that
> > ARM_CPU() is used to get the CPU in traget/arm/kvm.c which is used from
> > accel/kvm-all.c, so it seems this would work for accelerated mode.
>
> yeah I ma not familiar enough with that code but it is worth to be checked.

I'm a bit unsure about fishing around in the CPU ID registers for this.
That's not what you would do in real hardware, you'd just say "the
system is supposed to configure the CPU and the SMMU correctly".

Also, there is no guarantee that CPU 0 is related to this SMMU in
particular.

> >
> >>> +
> >>>  /**
> >>>   * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
> >>>   *   multi-level stream table
> >>> @@ -265,7 +272,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
> >>>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, 1);
> >>>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN16K, 1);
> >>>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
> >>> -s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 
> >>> bits */
> >>> +s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
> >> I am not sure you can change that easily. In case of migration this is
> >> going to change the behavior of the device, no?
> > I see IDR registers are not migrated. I guess we can add them in a
> > subsection and if they were not passed (old instances) we set OAS to
> > 44.
> > Maybe this should be another change outside of this series.
> Indeed tehy are not migrated so it can lead to inconsistent behavior in
> both source and dest. This deserves more analysis to me. In case you
> would decide to migrate IDR regs this would need to be done in that
> series I think. Migration must not be broken by this series.

Jumping in here without having read much of the context, but why
would we need to migrate the ID registers? They are constant, read-only,
so they will be the same value on both source and destination.

thanks
-- PMM



Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

2023-03-21 Thread Eric Auger



On 3/21/23 14:29, Mostafa Saleh wrote:
> On Tue, Mar 21, 2023 at 02:23:03PM +0100, Eric Auger wrote:
>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
> -s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 
> bits */
> +s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
 I am not sure you can change that easily. In case of migration this is
 going to change the behavior of the device, no?
>>> I see IDR registers are not migrated. I guess we can add them in a
>>> subsection and if they were not passed (old instances) we set OAS to
>>> 44.
>>> Maybe this should be another change outside of this series.
>> Indeed tehy are not migrated so it can lead to inconsistent behavior in
>> both source and dest. This deserves more analysis to me. In case you
>> would decide to migrate IDR regs this would need to be done in that
>> series I think. Migration must not be broken by this series
> I agree, I meant to drop this patch from the series as it is not
> really related to stage-2, and we can have another patch for this +
> migration for IDR if needed after doing proper analysis.

Ah OK. I get it now. Yes this looks sensible

Thanks

Eric
>
> Thanks,
> Mostafa
>




Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

2023-03-21 Thread Mostafa Saleh
On Tue, Mar 21, 2023 at 02:23:03PM +0100, Eric Auger wrote:
> >>>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
> >>> -s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 
> >>> bits */
> >>> +s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
> >> I am not sure you can change that easily. In case of migration this is
> >> going to change the behavior of the device, no?
> > I see IDR registers are not migrated. I guess we can add them in a
> > subsection and if they were not passed (old instances) we set OAS to
> > 44.
> > Maybe this should be another change outside of this series.
> Indeed tehy are not migrated so it can lead to inconsistent behavior in
> both source and dest. This deserves more analysis to me. In case you
> would decide to migrate IDR regs this would need to be done in that
> series I think. Migration must not be broken by this series

I agree, I meant to drop this patch from the series as it is not
really related to stage-2, and we can have another patch for this +
migration for IDR if needed after doing proper analysis.

Thanks,
Mostafa



Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

2023-03-21 Thread Eric Auger
Hi Mostafa,

On 3/21/23 14:06, Mostafa Saleh wrote:
> Hi Eric,
>
>>> + * According to 6.3.6 SMMU_IDR5, OAS must match the system physical 
>>> address
>>> + * size.
>>> + */
>>> +ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
>>> +uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, ID_AA64MMFR0, 
>>> PARANGE);
>> is this working in accelerated mode?
> I didn't try with accel, I will give it a try, but from what I see, that
> ARM_CPU() is used to get the CPU in traget/arm/kvm.c which is used from
> accel/kvm-all.c, so it seems this would work for accelerated mode.

yeah I ma not familiar enough with that code but it is worth to be checked.
>
>>> +
>>>  /**
>>>   * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
>>>   *   multi-level stream table
>>> @@ -265,7 +272,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
>>>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, 1);
>>>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN16K, 1);
>>>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
>>> -s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 
>>> bits */
>>> +s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
>> I am not sure you can change that easily. In case of migration this is
>> going to change the behavior of the device, no?
> I see IDR registers are not migrated. I guess we can add them in a
> subsection and if they were not passed (old instances) we set OAS to
> 44.
> Maybe this should be another change outside of this series.
Indeed tehy are not migrated so it can lead to inconsistent behavior in
both source and dest. This deserves more analysis to me. In case you
would decide to migrate IDR regs this would need to be done in that
series I think. Migration must not be broken by this series.

Thanks

Eric
>
> Thanks,
> Mostafa
>




Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

2023-03-21 Thread Mostafa Saleh
Hi Eric,

> > + * According to 6.3.6 SMMU_IDR5, OAS must match the system physical 
> > address
> > + * size.
> > + */
> > +ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
> > +uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, ID_AA64MMFR0, 
> > PARANGE);
> is this working in accelerated mode?
I didn't try with accel, I will give it a try, but from what I see, that
ARM_CPU() is used to get the CPU in traget/arm/kvm.c which is used from
accel/kvm-all.c, so it seems this would work for accelerated mode.

> > +
> >  /**
> >   * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
> >   *   multi-level stream table
> > @@ -265,7 +272,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
> >  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, 1);
> >  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN16K, 1);
> >  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
> > -s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 
> > bits */
> > +s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
> I am not sure you can change that easily. In case of migration this is
> going to change the behavior of the device, no?

I see IDR registers are not migrated. I guess we can add them in a
subsection and if they were not passed (old instances) we set OAS to
44.
Maybe this should be another change outside of this series.

Thanks,
Mostafa



Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

2023-03-20 Thread Eric Auger
Hi Mostafa,

On 2/26/23 23:06, Mostafa Saleh wrote:
> OAS used to be hardcoded to 44 bits, however according to SMMU manual
> 6.3.6 SMMU_IDR5, OAS must match the system physical address size, so
> we read it from CPU PARANGE.
>
> Remove PA_MAX and pa_range as they were not used.
>
> Add SMMUv3State as an argument to decode_cd, so it can read the SMMU
> OAS.
>
> As CPU can use PARANGE with 52 bits, add 52 bits check to oas2bits,
> and cap OAS to 48 bits for stage-1 and stage-2 if granule is not 64KB
> as specified for SMMUv3.1 and later.
>
> Signed-off-by: Mostafa Saleh 
> ---
>  hw/arm/smmu-common.c | 13 +
>  hw/arm/smmuv3-internal.h | 15 ++-
>  hw/arm/smmuv3.c  | 41 ++--
>  3 files changed, 46 insertions(+), 23 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index e4b477af10..3a2b06fd7f 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -307,7 +307,7 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
>  dma_addr_t baseaddr, indexmask;
>  int stage = cfg->stage;
>  SMMUTransTableInfo *tt = select_tt(cfg, iova);
> -uint8_t level, granule_sz, inputsize, stride;
> +uint8_t level, granule_sz, inputsize, stride, oas;
>  
>  if (!tt || tt->disabled) {
>  info->type = SMMU_PTW_ERR_TRANSLATION;
> @@ -319,7 +319,12 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
>  inputsize = 64 - tt->tsz;
>  level = 4 - (inputsize - 4) / stride;
>  indexmask = SMMU_IDXMSK(inputsize, stride, level);
> -baseaddr = extract64(tt->ttb, 0, 48);
> +oas = cfg->oas;
> +if (tt->granule_sz != 16) {
> +oas = MIN(oas, 48);
> +}
> +
> +baseaddr = extract64(tt->ttb, 0, oas);
>  baseaddr &= ~indexmask;
>  
>  while (level < SMMU_LEVELS) {
> @@ -416,8 +421,8 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
>   * Get the ttb from concatenated structure.
>   * The offset is the idx * size of each ttb(number of ptes * 
> (sizeof(pte))
>   */
> -uint64_t baseaddr = extract64(cfg->s2cfg.vttb, 0, 48) + (1 << stride) *
> -  idx * sizeof(uint64_t);
> +uint64_t baseaddr = extract64(cfg->s2cfg.vttb, 0, cfg->s2cfg.oas) +
> +  (1 << stride) * idx * sizeof(uint64_t);
>  dma_addr_t indexmask = SMMU_IDXMSK(inputsize, stride, level);
>  
>  baseaddr &= ~indexmask;
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index 3388e1a5f8..25ae12fb5c 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -564,23 +564,12 @@ static inline int oas2bits(int oas_field)
>  return 44;
>  case 5:
>  return 48;
> +case 6:
> +return 52;
>  }
>  return -1;
>  }
>  
> -static inline int pa_range(STE *ste)
> -{
> -int oas_field = MIN(STE_S2PS(ste), SMMU_IDR5_OAS);
> -
> -if (!STE_S2AA64(ste)) {
> -return 40;
> -}
> -
> -return oas2bits(oas_field);
> -}
> -
> -#define MAX_PA(ste) ((1 << pa_range(ste)) - 1)
> -
>  /* CD fields */
>  
>  #define CD_VALID(x)   extract32((x)->word[0], 31, 1)
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 7297f6adc1..bc4ec202f4 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -238,6 +238,13 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo 
> *info)
>  
>  static void smmuv3_init_regs(SMMUv3State *s)
>  {
> +/*
> + * According to 6.3.6 SMMU_IDR5, OAS must match the system physical 
> address
> + * size.
> + */
> +ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
> +uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, ID_AA64MMFR0, 
> PARANGE);
is this working in accelerated mode?
> +
>  /**
>   * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
>   *   multi-level stream table
> @@ -265,7 +272,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, 1);
>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN16K, 1);
>  s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
> -s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 bits 
> */
> +s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
I am not sure you can change that easily. In case of migration this is
going to change the behavior of the device, no?

Thanks

Eric
>  
>  s->cmdq.base = deposit64(s->cmdq.base, 0, 5, SMMU_CMDQS);
>  s->cmdq.prod = 0;
> @@ -374,6 +381,7 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
>STE *ste, SMMUEventInfo *event)
>  {
>  uint32_t config;
> +uint8_t oas = FIELD_EX32(s->idr[5], IDR5, OAS);
>  
>  if (!STE_VALID(ste)) {
>  if (!event->inval_ste_allowed) {
> @@ -450,7 +458,16 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
>  }
>  
>  
> -cfg->s2cfg.oas = oas2bits(MIN(STE_S2PS(ste), SMMU_IDR5_OAS));
> +cfg->s2cfg.oas = oas2bits(MIN(STE_S2PS(ste), 

[RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE

2023-02-26 Thread Mostafa Saleh
OAS used to be hardcoded to 44 bits, however according to SMMU manual
6.3.6 SMMU_IDR5, OAS must match the system physical address size, so
we read it from CPU PARANGE.

Remove PA_MAX and pa_range as they were not used.

Add SMMUv3State as an argument to decode_cd, so it can read the SMMU
OAS.

As CPU can use PARANGE with 52 bits, add 52 bits check to oas2bits,
and cap OAS to 48 bits for stage-1 and stage-2 if granule is not 64KB
as specified for SMMUv3.1 and later.

Signed-off-by: Mostafa Saleh 
---
 hw/arm/smmu-common.c | 13 +
 hw/arm/smmuv3-internal.h | 15 ++-
 hw/arm/smmuv3.c  | 41 ++--
 3 files changed, 46 insertions(+), 23 deletions(-)

diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index e4b477af10..3a2b06fd7f 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -307,7 +307,7 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
 dma_addr_t baseaddr, indexmask;
 int stage = cfg->stage;
 SMMUTransTableInfo *tt = select_tt(cfg, iova);
-uint8_t level, granule_sz, inputsize, stride;
+uint8_t level, granule_sz, inputsize, stride, oas;
 
 if (!tt || tt->disabled) {
 info->type = SMMU_PTW_ERR_TRANSLATION;
@@ -319,7 +319,12 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
 inputsize = 64 - tt->tsz;
 level = 4 - (inputsize - 4) / stride;
 indexmask = SMMU_IDXMSK(inputsize, stride, level);
-baseaddr = extract64(tt->ttb, 0, 48);
+oas = cfg->oas;
+if (tt->granule_sz != 16) {
+oas = MIN(oas, 48);
+}
+
+baseaddr = extract64(tt->ttb, 0, oas);
 baseaddr &= ~indexmask;
 
 while (level < SMMU_LEVELS) {
@@ -416,8 +421,8 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
  * Get the ttb from concatenated structure.
  * The offset is the idx * size of each ttb(number of ptes * (sizeof(pte))
  */
-uint64_t baseaddr = extract64(cfg->s2cfg.vttb, 0, 48) + (1 << stride) *
-  idx * sizeof(uint64_t);
+uint64_t baseaddr = extract64(cfg->s2cfg.vttb, 0, cfg->s2cfg.oas) +
+  (1 << stride) * idx * sizeof(uint64_t);
 dma_addr_t indexmask = SMMU_IDXMSK(inputsize, stride, level);
 
 baseaddr &= ~indexmask;
diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index 3388e1a5f8..25ae12fb5c 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -564,23 +564,12 @@ static inline int oas2bits(int oas_field)
 return 44;
 case 5:
 return 48;
+case 6:
+return 52;
 }
 return -1;
 }
 
-static inline int pa_range(STE *ste)
-{
-int oas_field = MIN(STE_S2PS(ste), SMMU_IDR5_OAS);
-
-if (!STE_S2AA64(ste)) {
-return 40;
-}
-
-return oas2bits(oas_field);
-}
-
-#define MAX_PA(ste) ((1 << pa_range(ste)) - 1)
-
 /* CD fields */
 
 #define CD_VALID(x)   extract32((x)->word[0], 31, 1)
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 7297f6adc1..bc4ec202f4 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -238,6 +238,13 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo 
*info)
 
 static void smmuv3_init_regs(SMMUv3State *s)
 {
+/*
+ * According to 6.3.6 SMMU_IDR5, OAS must match the system physical address
+ * size.
+ */
+ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
+uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
+
 /**
  * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
  *   multi-level stream table
@@ -265,7 +272,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
 s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, 1);
 s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN16K, 1);
 s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
-s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 bits */
+s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
 
 s->cmdq.base = deposit64(s->cmdq.base, 0, 5, SMMU_CMDQS);
 s->cmdq.prod = 0;
@@ -374,6 +381,7 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
   STE *ste, SMMUEventInfo *event)
 {
 uint32_t config;
+uint8_t oas = FIELD_EX32(s->idr[5], IDR5, OAS);
 
 if (!STE_VALID(ste)) {
 if (!event->inval_ste_allowed) {
@@ -450,7 +458,16 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
 }
 
 
-cfg->s2cfg.oas = oas2bits(MIN(STE_S2PS(ste), SMMU_IDR5_OAS));
+cfg->s2cfg.oas = oas2bits(MIN(STE_S2PS(ste), oas));
+/*
+ * For SMMUv3.1 and later, when OAS == IAS == 52, the stage 2 input
+ * range is further limited to 48 bits unless STE.S2TG indicates a
+ * 64KB granule.
+ */
+if (cfg->s2cfg.granule_sz != 16) {
+cfg->s2cfg.oas = MIN(cfg->s2cfg.oas, 48);
+}
+
 /*
  * It is ILLEGAL for the address in S2TTB to be outside the range
  * described by the effective S2PS value.
@@ -607,10 +624,12 @@ static