Re: [PATCH for-9.2 v6 03/12] hw/riscv: add RISC-V IOMMU base emulation

[Daniel Henrique Barboza]

On 8/17/24 8:34 AM, Andrew Jones wrote:
> On Thu, Aug 01, 2024 at 12:43:24PM GMT, Daniel Henrique Barboza wrote:
> > From: Tomasz Jeznach <tjeznach@rivosinc.com>
> >
> > The RISC-V IOMMU specification is now ratified as-per the RISC-V
> > international process. The latest frozen specifcation can be found at:
> >
> > https://github.com/riscv-non-isa/riscv-iommu/releases/download/v1.0/riscv-iommu.pdf
> >
> > Add the foundation of the device emulation for RISC-V IOMMU. It includes
> > support for s-stage (sv32, sv39, sv48, sv57 caps) and g-stage (sv32x4,
> > sv39x4, sv48x4, sv57x4 caps).
> >
> > Other capabilities like ATS and DBG support will be added incrementally
> > in the next patches.
> >
> > Co-developed-by: Sebastien Boeuf <seb@rivosinc.com>
> > Signed-off-by: Sebastien Boeuf <seb@rivosinc.com>
> > Signed-off-by: Tomasz Jeznach <tjeznach@rivosinc.com>
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---

 (...)

> > +/* Redirect MSI write for given GPA. */
> > +static MemTxResult riscv_iommu_msi_write(RISCVIOMMUState *s,
> > +    RISCVIOMMUContext *ctx, uint64_t gpa, uint64_t data,
> > +    unsigned size, MemTxAttrs attrs)
> > +{
> > +    MemTxResult res;
> > +    dma_addr_t addr;
> > +    uint64_t intn;
> > +    uint32_t n190;
> > +    uint64_t pte[2];
> > +    int fault_type = RISCV_IOMMU_FQ_TTYPE_UADDR_WR;
> > +    int cause;
> > +
> > +    /* Interrupt File Number */
> > +    intn = _pext_u64(PPN_DOWN(gpa), ctx->msi_addr_mask);
> > +    if (intn >= 256) {
> > +        /* Interrupt file number out of range */
> > +        res = MEMTX_ACCESS_ERROR;
> > +        cause = RISCV_IOMMU_FQ_CAUSE_MSI_LOAD_FAULT;
> > +        goto err;
> > +    }
> > +
> > +    /* fetch MSI PTE */
> > +    addr = PPN_PHYS(get_field(ctx->msiptp, RISCV_IOMMU_DC_MSIPTP_PPN));
> > +    addr = addr | (intn * sizeof(pte));
> > +    res = dma_memory_read(s->target_as, addr, &pte, sizeof(pte),
> > +            MEMTXATTRS_UNSPECIFIED);
> > +    if (res != MEMTX_OK) {
> > +        if (res == MEMTX_DECODE_ERROR) {
> > +            cause = RISCV_IOMMU_FQ_CAUSE_MSI_PT_CORRUPTED;
> > +        } else {
> > +            cause = RISCV_IOMMU_FQ_CAUSE_MSI_LOAD_FAULT;
> > +        }
> > +        goto err;
> > +    }
> > +
> > +    le64_to_cpus(&pte[0]);
> > +    le64_to_cpus(&pte[1]);
> > +
> > +    if (!(pte[0] & RISCV_IOMMU_MSI_PTE_V) || (pte[0] & RISCV_IOMMU_MSI_PTE_C)) 
> > {
> > +        /*
> > +         * The spec mentions that: "If msipte.C == 1, then further
> > +         * processing to interpret the PTE is implementation
> > +         * defined.". We'll abort with cause = 262 for this
> > +         * case too.
> > +         */
> > +        res = MEMTX_ACCESS_ERROR;
> > +        cause = RISCV_IOMMU_FQ_CAUSE_MSI_INVALID;
> > +        goto err;
> > +    }
> > +
> > +    switch (get_field(pte[0], RISCV_IOMMU_MSI_PTE_M)) {
> > +    case RISCV_IOMMU_MSI_PTE_M_BASIC:
> > +        /* MSI Pass-through mode */
> > +        addr = PPN_PHYS(get_field(pte[0], RISCV_IOMMU_MSI_PTE_PPN));
> > +        addr = addr | (gpa & TARGET_PAGE_MASK);
>
> I'm not sure what the idea was with this (maybe a misunderstanding of how
> guest interrupt files get targeted?), but we shouldn't be modifying the
> result of a translation with the input to that translation. It breaks
> translations where guest imsic address bits don't strictly overlap host
> imsic address bits and it allows the guest to access host memory it
> shouldn't. The fix is just to remove the line.

I'll remove this line in v7. Thanks,


Daniel

> Thanks,
> drew