Hi Daniel,
There is no response from Tomasz. Should this patch be merged?
Jason Chien <jason.ch...@sifive.com> 於 2024年12月18日 週三 下午5:52寫道：
> Ping.
>
> Jason Chien <jason.ch...@sifive.com> 於 2024年11月25日 週一 下午8:49寫道：
>
>> Hi Tomasz, any thoughs?
>>
>> Daniel Henrique Barboza <dbarb...@ventanamicro.com> 於 2024年11月12日 週二
>> 下午8:26寫道：
>>
>>>
>>> CCing Tomasz
>>>
>>> On 11/8/24 8:01 AM, Jason Chien wrote:
>>> > This commit introduces a translation tag to avoid invalidating an entry
>>> > that should not be invalidated when IOMMU executes invalidation
>>> commands.
>>> > E.g. IOTINVAL.VMA with GV=0, AV=0, PSCV=1 invalidates both a mapping
>>> > of single stage translation and a mapping of nested translation with
>>> > the same PSCID, but only the former one should be invalidated.
>>> >
>>> > Signed-off-by: Jason Chien <jason.ch...@sifive.com>
>>> > ---
>>>
>>>
>>> LGTM but I would like to hear from Tomasz if adding this new abstraction
>>> is
>>> the best way to what seems to be a bug in riscv_iommu_process_cq_tail().
>>>
>>>
>>> Daniel
>>>
>>>
>>> > hw/riscv/riscv-iommu.c | 205
>>> ++++++++++++++++++++++++++++++-----------
>>> > 1 file changed, 153 insertions(+), 52 deletions(-)
>>> >
>>> > diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
>>> > index ff9deefe37..ac6bbf91d6 100644
>>> > --- a/hw/riscv/riscv-iommu.c
>>> > +++ b/hw/riscv/riscv-iommu.c
>>> > @@ -64,8 +64,16 @@ struct RISCVIOMMUContext {
>>> > uint64_t msiptp; /* MSI redirection page table
>>> pointer */
>>> > };
>>> >
>>> > +typedef enum RISCVIOMMUTransTag {
>>> > + RISCV_IOMMU_TRANS_TAG_BY, /* Bypass */
>>> > + RISCV_IOMMU_TRANS_TAG_SS, /* Single Stage */
>>> > + RISCV_IOMMU_TRANS_TAG_VG, /* G-stage only */
>>> > + RISCV_IOMMU_TRANS_TAG_VN, /* Nested translation */
>>> > +} RISCVIOMMUTransTag;
>>> > +
>>> > /* Address translation cache entry */
>>> > struct RISCVIOMMUEntry {
>>> > + RISCVIOMMUTransTag tag; /* Translation Tag */
>>> > uint64_t iova:44; /* IOVA Page Number */
>>> > uint64_t pscid:20; /* Process Soft-Context identifier */
>>> > uint64_t phys:44; /* Physical Page Number */
>>> > @@ -1228,7 +1236,7 @@ static gboolean
>>> riscv_iommu_iot_equal(gconstpointer v1, gconstpointer v2)
>>> > RISCVIOMMUEntry *t1 = (RISCVIOMMUEntry *) v1;
>>> > RISCVIOMMUEntry *t2 = (RISCVIOMMUEntry *) v2;
>>> > return t1->gscid == t2->gscid && t1->pscid == t2->pscid &&
>>> > - t1->iova == t2->iova;
>>> > + t1->iova == t2->iova && t1->tag == t2->tag;
>>> > }
>>> >
>>> > static guint riscv_iommu_iot_hash(gconstpointer v)
>>> > @@ -1237,67 +1245,115 @@ static guint
>>> riscv_iommu_iot_hash(gconstpointer v)
>>> > return (guint)t->iova;
>>> > }
>>> >
>>> > -/* GV: 1 PSCV: 1 AV: 1 */
>>> > +/* GV: 0 AV: 0 PSCV: 0 GVMA: 0 */
>>> > +/* GV: 0 AV: 0 GVMA: 1 */
>>> > +static
>>> > +void riscv_iommu_iot_inval_all(gpointer key, gpointer value, gpointer
>>> data)
>>> > +{
>>> > + RISCVIOMMUEntry *iot = (RISCVIOMMUEntry *) value;
>>> > + RISCVIOMMUEntry *arg = (RISCVIOMMUEntry *) data;
>>> > + if (iot->tag == arg->tag) {
>>> > + iot->perm = IOMMU_NONE;
>>> > + }
>>> > +}
>>> > +
>>> > +/* GV: 0 AV: 0 PSCV: 1 GVMA: 0 */
>>> > +static
>>> > +void riscv_iommu_iot_inval_pscid(gpointer key, gpointer value,
>>> gpointer data)
>>> > +{
>>> > + RISCVIOMMUEntry *iot = (RISCVIOMMUEntry *) value;
>>> > + RISCVIOMMUEntry *arg = (RISCVIOMMUEntry *) data;
>>> > + if (iot->tag == arg->tag &&
>>> > + iot->pscid == arg->pscid) {
>>> > + iot->perm = IOMMU_NONE;
>>> > + }
>>> > +}
>>> > +
>>> > +/* GV: 0 AV: 1 PSCV: 0 GVMA: 0 */
>>> > +static
>>> > +void riscv_iommu_iot_inval_iova(gpointer key, gpointer value,
>>> gpointer data)
>>> > +{
>>> > + RISCVIOMMUEntry *iot = (RISCVIOMMUEntry *) value;
>>> > + RISCVIOMMUEntry *arg = (RISCVIOMMUEntry *) data;
>>> > + if (iot->tag == arg->tag &&
>>> > + iot->iova == arg->iova) {
>>> > + iot->perm = IOMMU_NONE;
>>> > + }
>>> > +}
>>> > +
>>> > +/* GV: 0 AV: 1 PSCV: 1 GVMA: 0 */
>>> > static void riscv_iommu_iot_inval_pscid_iova(gpointer key, gpointer
>>> value,
>>> > gpointer data)
>>> > {
>>> > RISCVIOMMUEntry *iot = (RISCVIOMMUEntry *) value;
>>> > RISCVIOMMUEntry *arg = (RISCVIOMMUEntry *) data;
>>> > - if (iot->gscid == arg->gscid &&
>>> > + if (iot->tag == arg->tag &&
>>> > iot->pscid == arg->pscid &&
>>> > iot->iova == arg->iova) {
>>> > iot->perm = IOMMU_NONE;
>>> > }
>>> > }
>>> >
>>> > -/* GV: 1 PSCV: 1 AV: 0 */
>>> > -static void riscv_iommu_iot_inval_pscid(gpointer key, gpointer value,
>>> > - gpointer data)
>>> > +/* GV: 1 AV: 0 PSCV: 0 GVMA: 0 */
>>> > +/* GV: 1 AV: 0 GVMA: 1 */
>>> > +static
>>> > +void riscv_iommu_iot_inval_gscid(gpointer key, gpointer value,
>>> gpointer data)
>>> > {
>>> > RISCVIOMMUEntry *iot = (RISCVIOMMUEntry *) value;
>>> > RISCVIOMMUEntry *arg = (RISCVIOMMUEntry *) data;
>>> > - if (iot->gscid == arg->gscid &&
>>> > - iot->pscid == arg->pscid) {
>>> > + if (iot->tag == arg->tag &&
>>> > + iot->gscid == arg->gscid) {
>>> > iot->perm = IOMMU_NONE;
>>> > }
>>> > }
>>> >
>>> > -/* GV: 1 GVMA: 1 */
>>> > -static void riscv_iommu_iot_inval_gscid_gpa(gpointer key, gpointer
>>> value,
>>> > - gpointer data)
>>> > +/* GV: 1 AV: 0 PSCV: 1 GVMA: 0 */
>>> > +static void riscv_iommu_iot_inval_gscid_pscid(gpointer key, gpointer
>>> value,
>>> > + gpointer data)
>>> > {
>>> > RISCVIOMMUEntry *iot = (RISCVIOMMUEntry *) value;
>>> > RISCVIOMMUEntry *arg = (RISCVIOMMUEntry *) data;
>>> > - if (iot->gscid == arg->gscid) {
>>> > - /* simplified cache, no GPA matching */
>>> > + if (iot->tag == arg->tag &&
>>> > + iot->gscid == arg->gscid &&
>>> > + iot->pscid == arg->pscid) {
>>> > iot->perm = IOMMU_NONE;
>>> > }
>>> > }
>>> >
>>> > -/* GV: 1 GVMA: 0 */
>>> > -static void riscv_iommu_iot_inval_gscid(gpointer key, gpointer value,
>>> > - gpointer data)
>>> > +/* GV: 1 AV: 1 PSCV: 0 GVMA: 0 */
>>> > +/* GV: 1 AV: 1 GVMA: 1 */
>>> > +static void riscv_iommu_iot_inval_gscid_iova(gpointer key, gpointer
>>> value,
>>> > + gpointer data)
>>> > {
>>> > RISCVIOMMUEntry *iot = (RISCVIOMMUEntry *) value;
>>> > RISCVIOMMUEntry *arg = (RISCVIOMMUEntry *) data;
>>> > - if (iot->gscid == arg->gscid) {
>>> > + if (iot->tag == arg->tag &&
>>> > + iot->gscid == arg->gscid &&
>>> > + iot->iova == arg->iova) {
>>> > iot->perm = IOMMU_NONE;
>>> > }
>>> > }
>>> >
>>> > -/* GV: 0 */
>>> > -static void riscv_iommu_iot_inval_all(gpointer key, gpointer value,
>>> > - gpointer data)
>>> > +/* GV: 1 AV: 1 PSCV: 1 GVMA: 0 */
>>> > +static void riscv_iommu_iot_inval_gscid_pscid_iova(gpointer key,
>>> gpointer value,
>>> > + gpointer data)
>>> > {
>>> > RISCVIOMMUEntry *iot = (RISCVIOMMUEntry *) value;
>>> > - iot->perm = IOMMU_NONE;
>>> > + RISCVIOMMUEntry *arg = (RISCVIOMMUEntry *) data;
>>> > + if (iot->tag == arg->tag &&
>>> > + iot->gscid == arg->gscid &&
>>> > + iot->pscid == arg->pscid &&
>>> > + iot->iova == arg->iova) {
>>> > + iot->perm = IOMMU_NONE;
>>> > + }
>>> > }
>>> >
>>> > /* caller should keep ref-count for iot_cache object */
>>> > static RISCVIOMMUEntry *riscv_iommu_iot_lookup(RISCVIOMMUContext
>>> *ctx,
>>> > - GHashTable *iot_cache, hwaddr iova)
>>> > + GHashTable *iot_cache, hwaddr iova, RISCVIOMMUTransTag transtag)
>>> > {
>>> > RISCVIOMMUEntry key = {
>>> > + .tag = transtag,
>>> > .gscid = get_field(ctx->gatp, RISCV_IOMMU_DC_IOHGATP_GSCID),
>>> > .pscid = get_field(ctx->ta, RISCV_IOMMU_DC_TA_PSCID),
>>> > .iova = PPN_DOWN(iova),
>>> > @@ -1323,10 +1379,11 @@ static void
>>> riscv_iommu_iot_update(RISCVIOMMUState *s,
>>> > }
>>> >
>>> > static void riscv_iommu_iot_inval(RISCVIOMMUState *s, GHFunc func,
>>> > - uint32_t gscid, uint32_t pscid, hwaddr iova)
>>> > + uint32_t gscid, uint32_t pscid, hwaddr iova, RISCVIOMMUTransTag
>>> transtag)
>>> > {
>>> > GHashTable *iot_cache;
>>> > RISCVIOMMUEntry key = {
>>> > + .tag = transtag,
>>> > .gscid = gscid,
>>> > .pscid = pscid,
>>> > .iova = PPN_DOWN(iova),
>>> > @@ -1337,9 +1394,24 @@ static void
>>> riscv_iommu_iot_inval(RISCVIOMMUState *s, GHFunc func,
>>> > g_hash_table_unref(iot_cache);
>>> > }
>>> >
>>> > +static RISCVIOMMUTransTag riscv_iommu_get_transtag(RISCVIOMMUContext
>>> *ctx)
>>> > +{
>>> > + uint64_t satp = get_field(ctx->satp, RISCV_IOMMU_ATP_MODE_FIELD);
>>> > + uint64_t gatp = get_field(ctx->gatp, RISCV_IOMMU_ATP_MODE_FIELD);
>>> > +
>>> > + if (satp == RISCV_IOMMU_DC_FSC_MODE_BARE) {
>>> > + return (gatp == RISCV_IOMMU_DC_IOHGATP_MODE_BARE) ?
>>> > + RISCV_IOMMU_TRANS_TAG_BY : RISCV_IOMMU_TRANS_TAG_VG;
>>> > + } else {
>>> > + return (gatp == RISCV_IOMMU_DC_IOHGATP_MODE_BARE) ?
>>> > + RISCV_IOMMU_TRANS_TAG_SS : RISCV_IOMMU_TRANS_TAG_VN;
>>> > + }
>>> > +}
>>> > +
>>> > static int riscv_iommu_translate(RISCVIOMMUState *s,
>>> RISCVIOMMUContext *ctx,
>>> > IOMMUTLBEntry *iotlb, bool enable_cache)
>>> > {
>>> > + RISCVIOMMUTransTag transtag = riscv_iommu_get_transtag(ctx);
>>> > RISCVIOMMUEntry *iot;
>>> > IOMMUAccessFlags perm;
>>> > bool enable_pid;
>>> > @@ -1365,7 +1437,7 @@ static int riscv_iommu_translate(RISCVIOMMUState
>>> *s, RISCVIOMMUContext *ctx,
>>> > }
>>> > }
>>> >
>>> > - iot = riscv_iommu_iot_lookup(ctx, iot_cache, iotlb->iova);
>>> > + iot = riscv_iommu_iot_lookup(ctx, iot_cache, iotlb->iova,
>>> transtag);
>>> > perm = iot ? iot->perm : IOMMU_NONE;
>>> > if (perm != IOMMU_NONE) {
>>> > iotlb->translated_addr = PPN_PHYS(iot->phys);
>>> > @@ -1396,6 +1468,7 @@ static int riscv_iommu_translate(RISCVIOMMUState
>>> *s, RISCVIOMMUContext *ctx,
>>> > iot->gscid = get_field(ctx->gatp,
>>> RISCV_IOMMU_DC_IOHGATP_GSCID);
>>> > iot->pscid = get_field(ctx->ta, RISCV_IOMMU_DC_TA_PSCID);
>>> > iot->perm = iotlb->perm;
>>> > + iot->tag = transtag;
>>> > riscv_iommu_iot_update(s, iot_cache, iot);
>>> > }
>>> >
>>> > @@ -1603,44 +1676,72 @@ static void
>>> riscv_iommu_process_cq_tail(RISCVIOMMUState *s)
>>> >
>>> > case RISCV_IOMMU_CMD(RISCV_IOMMU_CMD_IOTINVAL_FUNC_GVMA,
>>> > RISCV_IOMMU_CMD_IOTINVAL_OPCODE):
>>> > - if (cmd.dword0 & RISCV_IOMMU_CMD_IOTINVAL_PSCV) {
>>> > + {
>>> > + bool gv = !!(cmd.dword0 & RISCV_IOMMU_CMD_IOTINVAL_GV);
>>> > + bool av = !!(cmd.dword0 & RISCV_IOMMU_CMD_IOTINVAL_AV);
>>> > + bool pscv = !!(cmd.dword0 &
>>> RISCV_IOMMU_CMD_IOTINVAL_PSCV);
>>> > + uint32_t gscid = get_field(cmd.dword0,
>>> > +
>>> RISCV_IOMMU_CMD_IOTINVAL_GSCID);
>>> > + uint32_t pscid = get_field(cmd.dword0,
>>> > +
>>> RISCV_IOMMU_CMD_IOTINVAL_PSCID);
>>> > + hwaddr iova = (cmd.dword1 << 2) & TARGET_PAGE_MASK;
>>> > +
>>> > + if (pscv) {
>>> > /* illegal command arguments IOTINVAL.GVMA & PSCV ==
>>> 1 */
>>> > goto cmd_ill;
>>> > - } else if (!(cmd.dword0 & RISCV_IOMMU_CMD_IOTINVAL_GV)) {
>>> > - /* invalidate all cache mappings */
>>> > - func = riscv_iommu_iot_inval_all;
>>> > - } else if (!(cmd.dword0 & RISCV_IOMMU_CMD_IOTINVAL_AV)) {
>>> > - /* invalidate cache matching GSCID */
>>> > - func = riscv_iommu_iot_inval_gscid;
>>> > - } else {
>>> > - /* invalidate cache matching GSCID and ADDR (GPA) */
>>> > - func = riscv_iommu_iot_inval_gscid_gpa;
>>> > }
>>> > - riscv_iommu_iot_inval(s, func,
>>> > - get_field(cmd.dword0,
>>> RISCV_IOMMU_CMD_IOTINVAL_GSCID), 0,
>>> > - cmd.dword1 << 2 & TARGET_PAGE_MASK);
>>> > +
>>> > + func = riscv_iommu_iot_inval_all;
>>> > +
>>> > + if (gv) {
>>> > + func = (av) ? riscv_iommu_iot_inval_gscid_iova :
>>> > + riscv_iommu_iot_inval_gscid;
>>> > + }
>>> > +
>>> > + riscv_iommu_iot_inval(
>>> > + s, func, gscid, pscid, iova,
>>> RISCV_IOMMU_TRANS_TAG_VG);
>>> > +
>>> > + riscv_iommu_iot_inval(
>>> > + s, func, gscid, pscid, iova,
>>> RISCV_IOMMU_TRANS_TAG_VN);
>>> > break;
>>> > + }
>>> >
>>> > case RISCV_IOMMU_CMD(RISCV_IOMMU_CMD_IOTINVAL_FUNC_VMA,
>>> > RISCV_IOMMU_CMD_IOTINVAL_OPCODE):
>>> > - if (!(cmd.dword0 & RISCV_IOMMU_CMD_IOTINVAL_GV)) {
>>> > - /* invalidate all cache mappings, simplified model */
>>> > - func = riscv_iommu_iot_inval_all;
>>> > - } else if (!(cmd.dword0 & RISCV_IOMMU_CMD_IOTINVAL_PSCV))
>>> {
>>> > - /* invalidate cache matching GSCID, simplified model
>>> */
>>> > - func = riscv_iommu_iot_inval_gscid;
>>> > - } else if (!(cmd.dword0 & RISCV_IOMMU_CMD_IOTINVAL_AV)) {
>>> > - /* invalidate cache matching GSCID and PSCID */
>>> > - func = riscv_iommu_iot_inval_pscid;
>>> > + {
>>> > + bool gv = !!(cmd.dword0 & RISCV_IOMMU_CMD_IOTINVAL_GV);
>>> > + bool av = !!(cmd.dword0 & RISCV_IOMMU_CMD_IOTINVAL_AV);
>>> > + bool pscv = !!(cmd.dword0 &
>>> RISCV_IOMMU_CMD_IOTINVAL_PSCV);
>>> > + uint32_t gscid = get_field(cmd.dword0,
>>> > +
>>> RISCV_IOMMU_CMD_IOTINVAL_GSCID);
>>> > + uint32_t pscid = get_field(cmd.dword0,
>>> > +
>>> RISCV_IOMMU_CMD_IOTINVAL_PSCID);
>>> > + hwaddr iova = (cmd.dword1 << 2) & TARGET_PAGE_MASK;
>>> > + RISCVIOMMUTransTag transtag;
>>> > +
>>> > + if (gv) {
>>> > + transtag = RISCV_IOMMU_TRANS_TAG_VN;
>>> > + if (pscv) {
>>> > + func = (av) ?
>>> riscv_iommu_iot_inval_gscid_pscid_iova :
>>> > + riscv_iommu_iot_inval_gscid_pscid;
>>> > + } else {
>>> > + func = (av) ? riscv_iommu_iot_inval_gscid_iova :
>>> > + riscv_iommu_iot_inval_gscid;
>>> > + }
>>> > } else {
>>> > - /* invalidate cache matching GSCID and PSCID and ADDR
>>> (IOVA) */
>>> > - func = riscv_iommu_iot_inval_pscid_iova;
>>> > + transtag = RISCV_IOMMU_TRANS_TAG_SS;
>>> > + if (pscv) {
>>> > + func = (av) ? riscv_iommu_iot_inval_pscid_iova :
>>> > + riscv_iommu_iot_inval_pscid;
>>> > + } else {
>>> > + func = (av) ? riscv_iommu_iot_inval_iova :
>>> > + riscv_iommu_iot_inval_all;
>>> > + }
>>> > }
>>> > - riscv_iommu_iot_inval(s, func,
>>> > - get_field(cmd.dword0, RISCV_IOMMU_CMD_IOTINVAL_GSCID),
>>> > - get_field(cmd.dword0, RISCV_IOMMU_CMD_IOTINVAL_PSCID),
>>> > - cmd.dword1 << 2 & TARGET_PAGE_MASK);
>>> > +
>>> > + riscv_iommu_iot_inval(s, func, gscid, pscid, iova,
>>> transtag);
>>> > break;
>>> > + }
>>> >
>>> > case RISCV_IOMMU_CMD(RISCV_IOMMU_CMD_IODIR_FUNC_INVAL_DDT,
>>> > RISCV_IOMMU_CMD_IODIR_OPCODE):
>>>
>>>
