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

[Andrew Jones]

On Thu, Oct 03, 2024 at 10:06:11AM GMT, Daniel Henrique Barboza wrote:
> 
> 
> On 10/3/24 6:26 AM, Andrew Jones wrote:
> > On Tue, Oct 01, 2024 at 10:02:58PM GMT, Daniel Henrique Barboza wrote:
> > ...
> > > +/*
> > > + * RISCV IOMMU Address Translation Lookup - Page Table Walk
> > > + *
> > > + * Note: Code is based on get_physical_address() from 
> > > target/riscv/cpu_helper.c
> > > + * Both implementation can be merged into single helper function in 
> > > future.
> > > + * Keeping them separate for now, as error reporting and flow specifics 
> > > are
> > > + * sufficiently different for separate implementation.
> > > + *
> > > + * @s        : IOMMU Device State
> > > + * @ctx      : Translation context for device id and process address 
> > > space id.
> > > + * @iotlb    : translation data: physical address and access mode.
> > > + * @return   : success or fault cause code.
> > > + */
> > > +static int riscv_iommu_spa_fetch(RISCVIOMMUState *s, RISCVIOMMUContext 
> > > *ctx,
> > > +    IOMMUTLBEntry *iotlb)
> > > +{
> > > +    dma_addr_t addr, base;
> > > +    uint64_t satp, gatp, pte;
> > > +    bool en_s, en_g;
> > > +    struct {
> > > +        unsigned char step;
> > > +        unsigned char levels;
> > > +        unsigned char ptidxbits;
> > > +        unsigned char ptesize;
> > > +    } sc[2];
> > > +    /* Translation stage phase */
> > > +    enum {
> > > +        S_STAGE = 0,
> > > +        G_STAGE = 1,
> > > +    } pass;
> > > +    MemTxResult ret;
> > > +
> > > +    satp = get_field(ctx->satp, RISCV_IOMMU_ATP_MODE_FIELD);
> > > +    gatp = get_field(ctx->gatp, RISCV_IOMMU_ATP_MODE_FIELD);
> > > +
> > > +    en_s = satp != RISCV_IOMMU_DC_FSC_MODE_BARE;
> > > +    en_g = gatp != RISCV_IOMMU_DC_IOHGATP_MODE_BARE;
> > > +
> > > +    /*
> > > +     * Early check for MSI address match when IOVA == GPA. This check
> > > +     * is required to ensure MSI translation is applied in case
> > > +     * first-stage translation is set to BARE mode. In this case IOVA
> > > +     * provided is a valid GPA. Running translation through page walk
> > > +     * second stage translation will incorrectly try to translate GPA
> > > +     * to host physical page, likely hitting IOPF.
> > > +     */
> > 
> > Why was this comment expanded from the simple
> > 
> > "Early check for MSI address match when IOVA == GPA."
> > 
> > The comment is now incorrect since the check is required even when
> > first-stage translation is not BARE. I just skimmed the spec again trying
> > to figure out if the removal of '!en_s' is a hack or a fix, and I'm
> > inclined to say "fix", but it's an incomplete fix. I found a sentence that
> > says
> > 
> > "If the virtual memory scheme selected for first-stage is Bare but the
> > scheme for the second-stage is not Bare then the IOVA is a GPA."
> > 
> > which, in a way, defines a GPA to only be a GPA when second-stage is used
> > (and all MSI translation specifications refer to GPAs). However, maybe I
> > missed it, but I couldn't find any actual reason that the MSI table can't
> > be used when first-stage is not BARE and second-stage is (and, of course,
> > it makes no difference for single-stage translations to call IOVAs GPAs
> > or not).
> > 
> > Now, I also see
> > 
> > "If the virtual memory scheme selected for neither stage is Bare then the
> > IOVA is a VA. Two-stage address translation is in effect. The first-stage
> > translates the VA to a GPA and the second-stage translates the GPA to a
> > SPA."
> > 
> > in the spec, which means we should probably change the removal of '!en_s'
> > to '!(en_s && en_g)'. VFIO+irqbypass would still work with that and, when
> > Linux learns to support two-stage translation, we wouldn't incorrectly try
> > to check for a GVA in the MSI table.
> 
> Ok. It seems to me that we can't rely on the riscv-iommu spec alone to let
> us know how to detect if IOVA == GPA, given that one of the main usages
> we have ATM (VFIO irqbypass) will use GPAs with S-stage enabled.
> 
> (Note: shouldn't we open a bug against the riscv-iommu spec?)
> 

I can try writing a sentence or two for the spec to clarify this and send
a PR.

> I like the idea of ruling out the case where IOVA == VA since that is clear
> in the spec. We also know that MSI entries will always contains GPAs.
> 
> This is the change I'm going to make in v9:
> 
> 
>     /*
>      * Early check for MSI address match when IOVA == GPA to
>      * support VFIO+irqbypass. The riscv-iommu spec doesn't
>      * consider the case where a GPA can be produced by S-stage
>      * only, but we have real life examples like Linux VFIO that
>      * work that way. The spec alone does not provide a reliable
>      * way of detecting if IOVA == GPA.
>      *
>      * The spec is clear about what is a VA: "If the virtual
>      * memory scheme selected for neither stage is Bare then
>      * the IOVA is a VA", in our case "(en_s && en_g)". We also
>      * know that MSI tables will always hold GPAs.
>      *
>      * Thus the check consists of ruling out VAs and checking
>      * the MSI table.
>      */
>     if (!(en_s && en_g) && (iotlb->perm & IOMMU_WO) &&
>         riscv_iommu_msi_check(s, ctx, iotlb->iova)) {
>         iotlb->target_as = &s->trap_as;
>         iotlb->translated_addr = iotlb->iova;
>         iotlb->addr_mask = ~TARGET_PAGE_MASK;
>         return 0;
>     }

LGTM

Thanks,
drew

> 
> Tomasz, let me know if you have any opinions against it. I intend to send
> the v9 start of next week.
> 
> 
> Thanks,
> 
> 
> Daniel
> 
> 
> > 
> > Thanks,
> > drew