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

[Andrew Jones <ajones@ventanamicro.com>]

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.

Thanks,
drew