diff mbox series

[PULL,21/26] hw/arm/smmu: Refactor SMMU OAS

Message ID 20240718132028.697927-22-peter.maydell@linaro.org (mailing list archive)
State New, archived
Headers show
Series [PULL,01/26] target/arm: Fix handling of LDAPR/STLR with negative offset | expand

Commit Message

Peter Maydell July 18, 2024, 1:20 p.m. UTC 
From: Mostafa Saleh <smostafa@google.com>

SMMUv3 OAS is currently hardcoded in the code to 44 bits, for nested
configurations that can be a problem, as stage-2 might be shared with
the CPU which might have different PARANGE, and according to SMMU manual
ARM IHI 0070F.b:
    6.3.6 SMMU_IDR5, OAS must match the system physical address size.

This patch doesn't change the SMMU OAS, but refactors the code to
make it easier to do that:
- Rely everywhere on IDR5 for reading OAS instead of using the
  SMMU_IDR5_OAS macro, so, it is easier just to change IDR5 and
  it propagages correctly.
- Add additional checks when OAS is greater than 48bits.
- Remove unused functions/macros: pa_range/MAX_PA.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Mostafa Saleh <smostafa@google.com>
Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Message-id: 20240715084519.1189624-19-smostafa@google.com
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/smmuv3-internal.h | 13 -------------
 hw/arm/smmu-common.c     |  7 ++++---
 hw/arm/smmuv3.c          | 35 ++++++++++++++++++++++++++++-------
 3 files changed, 32 insertions(+), 23 deletions(-)

Comments

Peter Maydell July 20, 2024, 3:05 p.m. UTC | #1 
On Thu, 18 Jul 2024 at 14:20, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> From: Mostafa Saleh <smostafa@google.com>
>
> SMMUv3 OAS is currently hardcoded in the code to 44 bits, for nested
> configurations that can be a problem, as stage-2 might be shared with
> the CPU which might have different PARANGE, and according to SMMU manual
> ARM IHI 0070F.b:
>     6.3.6 SMMU_IDR5, OAS must match the system physical address size.
>
> This patch doesn't change the SMMU OAS, but refactors the code to
> make it easier to do that:
> - Rely everywhere on IDR5 for reading OAS instead of using the
>   SMMU_IDR5_OAS macro, so, it is easier just to change IDR5 and
>   it propagages correctly.
> - Add additional checks when OAS is greater than 48bits.
> - Remove unused functions/macros: pa_range/MAX_PA.

Hi; Coverity has spotted a possible problem with the OAS handling
in this code (CID 1558464). I'm not sure if that's directly because of
this patch or if it's just that the code change has caused Coverity to
flag up a preexisting problem.

It's quite possible this is a false-positive because Coverity hasn't
noticed that the situation can't happen; but if so I think it's also
sufficiently unclear to a human reader to be worth addressing anyway.

> -static int decode_ste_s2_cfg(SMMUTransCfg *cfg, STE *ste)
> +static int decode_ste_s2_cfg(SMMUv3State *s, SMMUTransCfg *cfg,
> +                             STE *ste)
>  {
> +    uint8_t oas = FIELD_EX32(s->idr[5], IDR5, OAS);
> +
>      if (STE_S2AA64(ste) == 0x0) {
>          qemu_log_mask(LOG_UNIMP,
>                        "SMMUv3 AArch32 tables not supported\n");
> @@ -460,7 +463,15 @@ static int decode_ste_s2_cfg(SMMUTransCfg *cfg, STE *ste)
>      }
>
>      /* For AA64, The effective S2PS size is capped to the OAS. */
> -    cfg->s2cfg.eff_ps = oas2bits(MIN(STE_S2PS(ste), SMMU_IDR5_OAS));
> +    cfg->s2cfg.eff_ps = oas2bits(MIN(STE_S2PS(ste), oas));

oas2bits() is implemented as a function that returns -1 if the input
isn't a valid OAS. But we don't check for that failure here, so we put
the result into a uint8_t, which ends up as 255. Then later in
the function we will do
  MAKE_64BIT_MASK(0, cfg->s2cfg.eff_ps)
which will do an undefined-behaviour shift by a negative number
if eff_ps is 255.

If the invalid-OAS case is impossible we should assert rather
than returning -1; if it's not impossible we should handle it.

Mostafa, could you have a look at this, please?

thanks
-- PMM
Mostafa Saleh July 20, 2024, 10:07 p.m. UTC | #2 
Hi Peter,

On Sat, Jul 20, 2024 at 04:05:40PM +0100, Peter Maydell wrote:
> On Thu, 18 Jul 2024 at 14:20, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > From: Mostafa Saleh <smostafa@google.com>
> >
> > SMMUv3 OAS is currently hardcoded in the code to 44 bits, for nested
> > configurations that can be a problem, as stage-2 might be shared with
> > the CPU which might have different PARANGE, and according to SMMU manual
> > ARM IHI 0070F.b:
> >     6.3.6 SMMU_IDR5, OAS must match the system physical address size.
> >
> > This patch doesn't change the SMMU OAS, but refactors the code to
> > make it easier to do that:
> > - Rely everywhere on IDR5 for reading OAS instead of using the
> >   SMMU_IDR5_OAS macro, so, it is easier just to change IDR5 and
> >   it propagages correctly.
> > - Add additional checks when OAS is greater than 48bits.
> > - Remove unused functions/macros: pa_range/MAX_PA.
> 
> Hi; Coverity has spotted a possible problem with the OAS handling
> in this code (CID 1558464). I'm not sure if that's directly because of
> this patch or if it's just that the code change has caused Coverity to
> flag up a preexisting problem.
> 
> It's quite possible this is a false-positive because Coverity hasn't
> noticed that the situation can't happen; but if so I think it's also
> sufficiently unclear to a human reader to be worth addressing anyway.
> 
> > -static int decode_ste_s2_cfg(SMMUTransCfg *cfg, STE *ste)
> > +static int decode_ste_s2_cfg(SMMUv3State *s, SMMUTransCfg *cfg,
> > +                             STE *ste)
> >  {
> > +    uint8_t oas = FIELD_EX32(s->idr[5], IDR5, OAS);
> > +
> >      if (STE_S2AA64(ste) == 0x0) {
> >          qemu_log_mask(LOG_UNIMP,
> >                        "SMMUv3 AArch32 tables not supported\n");
> > @@ -460,7 +463,15 @@ static int decode_ste_s2_cfg(SMMUTransCfg *cfg, STE *ste)
> >      }
> >
> >      /* For AA64, The effective S2PS size is capped to the OAS. */
> > -    cfg->s2cfg.eff_ps = oas2bits(MIN(STE_S2PS(ste), SMMU_IDR5_OAS));
> > +    cfg->s2cfg.eff_ps = oas2bits(MIN(STE_S2PS(ste), oas));
> 
> oas2bits() is implemented as a function that returns -1 if the input
> isn't a valid OAS. But we don't check for that failure here, so we put
> the result into a uint8_t, which ends up as 255. Then later in
> the function we will do
>   MAKE_64BIT_MASK(0, cfg->s2cfg.eff_ps)
> which will do an undefined-behaviour shift by a negative number
> if eff_ps is 255.
> 
> If the invalid-OAS case is impossible we should assert rather
> than returning -1; if it's not impossible we should handle it.
> 
> Mostafa, could you have a look at this, please?

Yes, it should be impossible to have an invalid OAS.

This patch doesn't change the old behaviour, it just consolidate OAS
setting in one place, instead hardcoding it everywhere, so here
instead of using the macro (SMMU_IDR5_OAS) directly we now read it
from IDR5, which is set to SMMU_IDR5_OAS at smmuv3_init_regs().

The other field S2PS is casted to 6 bits, and as we use MIN, and
all the previous values are valid, so it should be fine:
- 0b000: 32 bits
- 0b001: 36 bits
- 0b010: 40 bits
- 0b011: 42 bits
- 0b100: 44 bits

Adding an assertion makes sense to me. Please, let me know if you
want me to send a patch for that.

Thanks,
Mostafa

> 
> thanks
> -- PMM
>
Peter Maydell July 22, 2024, 9:33 a.m. UTC | #3 
On Sat, 20 Jul 2024 at 23:07, Mostafa Saleh <smostafa@google.com> wrote:
>
> Hi Peter,
>
> On Sat, Jul 20, 2024 at 04:05:40PM +0100, Peter Maydell wrote:
> > On Thu, 18 Jul 2024 at 14:20, Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > From: Mostafa Saleh <smostafa@google.com>
> > >
> > > SMMUv3 OAS is currently hardcoded in the code to 44 bits, for nested
> > > configurations that can be a problem, as stage-2 might be shared with
> > > the CPU which might have different PARANGE, and according to SMMU manual
> > > ARM IHI 0070F.b:
> > >     6.3.6 SMMU_IDR5, OAS must match the system physical address size.
> > >
> > > This patch doesn't change the SMMU OAS, but refactors the code to
> > > make it easier to do that:
> > > - Rely everywhere on IDR5 for reading OAS instead of using the
> > >   SMMU_IDR5_OAS macro, so, it is easier just to change IDR5 and
> > >   it propagages correctly.
> > > - Add additional checks when OAS is greater than 48bits.
> > > - Remove unused functions/macros: pa_range/MAX_PA.
> >
> > Hi; Coverity has spotted a possible problem with the OAS handling
> > in this code (CID 1558464). I'm not sure if that's directly because of
> > this patch or if it's just that the code change has caused Coverity to
> > flag up a preexisting problem.
> >
> > It's quite possible this is a false-positive because Coverity hasn't
> > noticed that the situation can't happen; but if so I think it's also
> > sufficiently unclear to a human reader to be worth addressing anyway.
> >
> > > -static int decode_ste_s2_cfg(SMMUTransCfg *cfg, STE *ste)
> > > +static int decode_ste_s2_cfg(SMMUv3State *s, SMMUTransCfg *cfg,
> > > +                             STE *ste)
> > >  {
> > > +    uint8_t oas = FIELD_EX32(s->idr[5], IDR5, OAS);
> > > +
> > >      if (STE_S2AA64(ste) == 0x0) {
> > >          qemu_log_mask(LOG_UNIMP,
> > >                        "SMMUv3 AArch32 tables not supported\n");
> > > @@ -460,7 +463,15 @@ static int decode_ste_s2_cfg(SMMUTransCfg *cfg, STE *ste)
> > >      }
> > >
> > >      /* For AA64, The effective S2PS size is capped to the OAS. */
> > > -    cfg->s2cfg.eff_ps = oas2bits(MIN(STE_S2PS(ste), SMMU_IDR5_OAS));
> > > +    cfg->s2cfg.eff_ps = oas2bits(MIN(STE_S2PS(ste), oas));
> >
> > oas2bits() is implemented as a function that returns -1 if the input
> > isn't a valid OAS. But we don't check for that failure here, so we put
> > the result into a uint8_t, which ends up as 255. Then later in
> > the function we will do
> >   MAKE_64BIT_MASK(0, cfg->s2cfg.eff_ps)
> > which will do an undefined-behaviour shift by a negative number
> > if eff_ps is 255.
> >
> > If the invalid-OAS case is impossible we should assert rather
> > than returning -1; if it's not impossible we should handle it.
> >
> > Mostafa, could you have a look at this, please?
>
> Yes, it should be impossible to have an invalid OAS.
>
> This patch doesn't change the old behaviour, it just consolidate OAS
> setting in one place, instead hardcoding it everywhere, so here
> instead of using the macro (SMMU_IDR5_OAS) directly we now read it
> from IDR5, which is set to SMMU_IDR5_OAS at smmuv3_init_regs().
>
> The other field S2PS is casted to 6 bits, and as we use MIN, and
> all the previous values are valid, so it should be fine:
> - 0b000: 32 bits
> - 0b001: 36 bits
> - 0b010: 40 bits
> - 0b011: 42 bits
> - 0b100: 44 bits
>
> Adding an assertion makes sense to me. Please, let me know if you
> want me to send a patch for that.

Yes, if this can't happen even with invalid guest input, please
send a patch that asserts that.

thanks
-- PMM
diff mbox series

Patch

diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index 0f3ecec804d..0ebf2eebcff 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -602,19 +602,6 @@  static inline int oas2bits(int oas_field)
     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/smmu-common.c b/hw/arm/smmu-common.c
index 00d7579cd3d..d73ad622113 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -452,7 +452,8 @@  static int smmu_ptw_64_s1(SMMUState *bs, SMMUTransCfg *cfg,
     inputsize = 64 - tt->tsz;
     level = 4 - (inputsize - 4) / stride;
     indexmask = VMSA_IDXMSK(inputsize, stride, level);
-    baseaddr = extract64(tt->ttb, 0, 48);
+
+    baseaddr = extract64(tt->ttb, 0, cfg->oas);
     baseaddr &= ~indexmask;
 
     while (level < VMSA_LEVELS) {
@@ -576,8 +577,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.eff_ps) +
+                                  (1 << stride) * idx * sizeof(uint64_t);
     dma_addr_t indexmask = VMSA_IDXMSK(inputsize, stride, level);
 
     baseaddr &= ~indexmask;
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 3db6c7c1357..39719763897 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -402,10 +402,10 @@  static bool s2t0sz_valid(SMMUTransCfg *cfg)
     }
 
     if (cfg->s2cfg.granule_sz == 16) {
-        return (cfg->s2cfg.tsz >= 64 - oas2bits(SMMU_IDR5_OAS));
+        return (cfg->s2cfg.tsz >= 64 - cfg->s2cfg.eff_ps);
     }
 
-    return (cfg->s2cfg.tsz >= MAX(64 - oas2bits(SMMU_IDR5_OAS), 16));
+    return (cfg->s2cfg.tsz >= MAX(64 - cfg->s2cfg.eff_ps, 16));
 }
 
 /*
@@ -426,8 +426,11 @@  static bool s2_pgtable_config_valid(uint8_t sl0, uint8_t t0sz, uint8_t gran)
     return nr_concat <= VMSA_MAX_S2_CONCAT;
 }
 
-static int decode_ste_s2_cfg(SMMUTransCfg *cfg, STE *ste)
+static int decode_ste_s2_cfg(SMMUv3State *s, SMMUTransCfg *cfg,
+                             STE *ste)
 {
+    uint8_t oas = FIELD_EX32(s->idr[5], IDR5, OAS);
+
     if (STE_S2AA64(ste) == 0x0) {
         qemu_log_mask(LOG_UNIMP,
                       "SMMUv3 AArch32 tables not supported\n");
@@ -460,7 +463,15 @@  static int decode_ste_s2_cfg(SMMUTransCfg *cfg, STE *ste)
     }
 
     /* For AA64, The effective S2PS size is capped to the OAS. */
-    cfg->s2cfg.eff_ps = oas2bits(MIN(STE_S2PS(ste), SMMU_IDR5_OAS));
+    cfg->s2cfg.eff_ps = 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.eff_ps = MIN(cfg->s2cfg.eff_ps, 48);
+    }
     /*
      * It is ILLEGAL for the address in S2TTB to be outside the range
      * described by the effective S2PS value.
@@ -536,6 +547,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);
     int ret;
 
     if (!STE_VALID(ste)) {
@@ -579,8 +591,8 @@  static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
          * Stage-1 OAS defaults to OAS even if not enabled as it would be used
          * in input address check for stage-2.
          */
-        cfg->oas = oas2bits(SMMU_IDR5_OAS);
-        ret = decode_ste_s2_cfg(cfg, ste);
+        cfg->oas = oas2bits(oas);
+        ret = decode_ste_s2_cfg(s, cfg, ste);
         if (ret) {
             goto bad_ste;
         }
@@ -706,6 +718,7 @@  static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg,
     int i;
     SMMUTranslationStatus status;
     SMMUTLBEntry *entry;
+    uint8_t oas = FIELD_EX32(s->idr[5], IDR5, OAS);
 
     if (!CD_VALID(cd) || !CD_AARCH64(cd)) {
         goto bad_cd;
@@ -724,7 +737,7 @@  static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg,
     cfg->aa64 = true;
 
     cfg->oas = oas2bits(CD_IPS(cd));
-    cfg->oas = MIN(oas2bits(SMMU_IDR5_OAS), cfg->oas);
+    cfg->oas = MIN(oas2bits(oas), cfg->oas);
     cfg->tbi = CD_TBI(cd);
     cfg->asid = CD_ASID(cd);
     cfg->affd = CD_AFFD(cd);
@@ -753,6 +766,14 @@  static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg,
             goto bad_cd;
         }
 
+        /*
+         * An address greater than 48 bits in size can only be output from a
+         * TTD when, in SMMUv3.1 and later, the effective IPS is 52 and a 64KB
+         * granule is in use for that translation table
+         */
+        if (tt->granule_sz != 16) {
+            cfg->oas = MIN(cfg->oas, 48);
+        }
         tt->tsz = tsz;
         tt->ttb = CD_TTB(cd, i);