Re: [PATCH v3 09/13] hw/riscv/riscv-iommu: add s-stage and g-stage support

[Daniel Henrique Barboza]

On 6/18/24 7:30 AM, Jason Chien wrote:
> Hi Daniel,
>
> On 2024/5/24 上午 01:39, Daniel Henrique Barboza wrote:
> > From: Tomasz Jeznach <tjeznach@rivosinc.com>
> >
> > Add support for s-stage (sv32, sv39, sv48, sv57 caps) and g-stage
> > (sv32x4, sv39x4, sv48x4, sv57x4 caps). Most of the work is done in the
> > riscv_iommu_spa_fetch() function that now has to consider how many
> > translation stages we need to walk the page table.
> >
> > Signed-off-by: Tomasz Jeznach <tjeznach@rivosinc.com>
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> >   hw/riscv/riscv-iommu-bits.h |  11 ++
> >   hw/riscv/riscv-iommu.c      | 331 +++++++++++++++++++++++++++++++++++-
> >   hw/riscv/riscv-iommu.h      |   2 +
> >   3 files changed, 336 insertions(+), 8 deletions(-)

(...)

> >       /* Set translation context. */
> >       ctx->tc = le64_to_cpu(dc.tc);
> > +    ctx->gatp = le64_to_cpu(dc.iohgatp);
> > +    ctx->satp = le64_to_cpu(dc.fsc);
> >       ctx->ta = le64_to_cpu(dc.ta);
> >       ctx->msiptp = le64_to_cpu(dc.msiptp);
> >       ctx->msi_addr_mask = le64_to_cpu(dc.msi_addr_mask);
> > @@ -564,14 +842,38 @@ static int riscv_iommu_ctx_fetch(RISCVIOMMUState *s, 
> > RISCVIOMMUContext *ctx)
> >           return RISCV_IOMMU_FQ_CAUSE_DDT_INVALID;
> >       }
> > +    /* FSC field checks */
> > +    mode = get_field(ctx->satp, RISCV_IOMMU_DC_FSC_MODE);
> > +    addr = PPN_PHYS(get_field(ctx->satp, RISCV_IOMMU_DC_FSC_PPN));
> > +
> > +    if (mode == RISCV_IOMMU_DC_FSC_MODE_BARE) {
> According to section 2.3, if the function returns here, some necessary checks are skipped. I 
> think this if scope should be moved down to after "if (ctx->pasid == 
> RISCV_IOMMU_NOPASID) {...}".
> > +        /* No S-Stage translation, done. */
> > +        return 0;
> > +    }
> > +
> >       if (!(ctx->tc & RISCV_IOMMU_DC_TC_PDTV)) {
> >           if (ctx->pasid != RISCV_IOMMU_NOPASID) {
> >               /* PASID is disabled */
> >               return RISCV_IOMMU_FQ_CAUSE_TTYPE_BLOCKED;
> >           }
> > +        if (mode > RISCV_IOMMU_DC_FSC_IOSATP_MODE_SV57) {
> > +            /* Invalid translation mode */
> > +            return RISCV_IOMMU_FQ_CAUSE_DDT_INVALID;
> > +        }
> >           return 0;
> >       }
> > +    if (ctx->pasid == RISCV_IOMMU_NOPASID) {
> > +        if (!(ctx->tc & RISCV_IOMMU_DC_TC_DPE)) {
> > +            /* No default PASID enabled, set BARE mode */
> > +            ctx->satp = 0ULL;
> > +            return 0;
> > +        } else {
> > +            /* Use default PASID #0 */
> > +            ctx->pasid = 0;
> > +        }
> > +    }
> > +
> return if mode is bare.

I agree. Just moved the 'if (bare) return' check to this point as you
suggested.


Thanks,


Daniel