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

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

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