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

[Daniel Henrique Barboza]

On 5/10/24 08:14, Andrew Jones wrote:
> On Fri, May 10, 2024 at 06:36:51PM GMT, Frank Chang wrote:
> ...
> > >   static int riscv_iommu_spa_fetch(RISCVIOMMUState *s, RISCVIOMMUContext *ctx,
> > > -    IOMMUTLBEntry *iotlb)
> > > +    IOMMUTLBEntry *iotlb, bool gpa)
> > >   {
> > > +    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;
> > > +
> > > +    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 && !gpa;
> > > +    en_g = gatp != RISCV_IOMMU_DC_IOHGATP_MODE_BARE;
> > > +
> > >       /* Early check for MSI address match when IOVA == GPA */
> > > -    if (iotlb->perm & IOMMU_WO &&
> > > +    if (!en_s && (iotlb->perm & IOMMU_WO) &&
> >
> > I'm wondering do we need to check "en_s" for MSI writes?
> >
> > IOMMU spec Section 2.3.3. Process to translate addresses of MSIs says:
> > "Determine if the address A is an access to a virtual interrupt file
> > as specified in Section 2.1.3.6."
> >
> > and Section 2.1.3.6 says:
> >
> > "An incoming memory access made by a device is recognized as
> > an access to a virtual interrupt file if the destination guest physical page
> > matches the supplied address pattern in all bit positions that are zeros
> > in the supplied address mask. In detail, a memory access to
> > guest physical address A is recognized as an access to a virtual
> > interrupt file’s
> > memory-mapped page if:
> > (A >> 12) & ~msi_addr_mask = (msi_addr_pattern & ~msi_addr_mask)"
> >
> > Is checking the address pattern sufficient enough to determine
> > the address is an MSI to a virtual interrupt file?
>
> I think so. In fact, I've removed that en_s check on our internal build in
> order to get things working for my irqbypass work, as we can do device
> assignment with VFIO with only S-stage enabled.

The following code will be fixed up here:

 static int riscv_iommu_spa_fetch(RISCVIOMMUState *s, RISCVIOMMUContext *ctx,
-    IOMMUTLBEntry *iotlb, bool gpa)
+    IOMMUTLBEntry *iotlb)
 {
     dma_addr_t addr, base;
     uint64_t satp, gatp, pte;
@@ -238,11 +237,11 @@ static int riscv_iommu_spa_fetch(RISCVIOMMUState *s, 
RISCVIOMMUContext *ctx,
     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 && !gpa;
+    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 */
-    if (!en_s && (iotlb->perm & IOMMU_WO) &&
+    if ((iotlb->perm & IOMMU_WO) &&
         riscv_iommu_msi_check(s, ctx, iotlb->iova)) {
         iotlb->target_as = &s->trap_as;
         iotlb->translated_addr = iotlb->iova;
@@ -1203,7 +1202,7 @@ static int riscv_iommu_translate(RISCVIOMMUState *s, 
RISCVIOMMUContext *ctx,
     }
 
     /* Translate using device directory / page table information. */
-    fault = riscv_iommu_spa_fetch(s, ctx, iotlb, false);
+    fault = riscv_iommu_spa_fetch(s, ctx, iotlb);
 
     if (!fault && iotlb->target_as == &s->trap_as) {
         /* Do not cache trapped MSI translations */

'gpa' is eliminated since it was only being used as 'false' by the only
caller of riscv_iommu_spa_fetch(). The boolean was used only to calculate
en_s as "&& !gpa", so it's always 'true' and had no impact in en_s. My
understand here is that 'gpa' was a prototype of the first implementation
that got left behind and ended up not being used.

As for the MSI check, we won't skip translation if satp is bare (!en_s) because
we might be using just stage2 for a guest, thus en_s is removed from the
conditional. As Frank said, this change also complies with the spec since we 
don't
need to check satp to determine if the address is an MSI to a virtual interrupt
file.

And, last but not the least, this change doesn't break my KVM VFIO passthrough
test case :) I'll document more about the test case I'm using in the v3 cover
letter.


Thanks,

Daniel


> Thanks,
> drew