| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20260118143309.94209-1-fangyu.yu@linux.alibaba.com>
Date: Sun, 18 Jan 2026 22:33:09 +0800
From: fangyu.yu@...ux.alibaba.com
To: guoren@...nel.org
Cc: ajones@...tanamicro.com,
alex@...ti.fr,
aou@...s.berkeley.edu,
fangyu.yu@...ux.alibaba.com,
iommu@...ts.linux.dev,
joro@...tes.org,
linux-kernel@...r.kernel.org,
linux-riscv@...ts.infradead.org,
palmer@...belt.com,
pjw@...nel.org,
robin.murphy@....com,
tjeznach@...osinc.com,
will@...nel.org
Subject: Re: Re: [PATCH] iommu/riscv: Add IOTINVAL after updating DDT/PDT entries
>Hi fangyu,
>
>On Thu, Jan 8, 2026 at 9:49 PM <fangyu.yu@...ux.alibaba.com> wrote:
>>
>> From: Fangyu Yu <fangyu.yu@...ux.alibaba.com>
>>
>> Add riscv_iommu_iodir_iotinval() to perform required TLB and context cache
>> invalidations after updating DDT or PDT entries, as mandated by the RISC-V
>> IOMMU specification (Section 6.3.1 and 6.3.2).
>>
>> Signed-off-by: Fangyu Yu <fangyu.yu@...ux.alibaba.com>
>> ---
>> drivers/iommu/riscv/iommu.c | 85 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 85 insertions(+)
>>
>> diff --git a/drivers/iommu/riscv/iommu.c b/drivers/iommu/riscv/iommu.c
>> index d9429097a2b5..2900170133fc 100644
>> --- a/drivers/iommu/riscv/iommu.c
>> +++ b/drivers/iommu/riscv/iommu.c
>> @@ -996,7 +996,82 @@ static void riscv_iommu_iotlb_inval(struct riscv_iommu_domain *domain,
>> }
>>
>> #define RISCV_IOMMU_FSC_BARE 0
>> +/*
>> + * This function sends IOTINVAL commands as required by the RISC-V
>> + * IOMMU specification (Section 6.3.1 and 6.3.2 in 1.0 spec version)
>> + * after modifying DDT or PDT entries
>> + */
>> +static void riscv_iommu_iodir_iotinval(struct riscv_iommu_device *iommu,
>> + bool inval_pdt, unsigned long iohgatp,
>> + struct riscv_iommu_dc *dc, struct riscv_iommu_pc *pc)
>> +{
>> + struct riscv_iommu_command cmd;
>>
>> + if (FIELD_GET(RISCV_IOMMU_DC_IOHGATP_MODE, iohgatp) ==
>> + RISCV_IOMMU_DC_IOHGATP_MODE_BARE) {
>> + if (inval_pdt) {
>> + /*
>> + * IOTINVAL.VMA with GV=AV=0, and PSCV=1, and
>> + * PSCID=PC.PSCID
>> + */
>> + riscv_iommu_cmd_inval_vma(&cmd);
>> + riscv_iommu_cmd_inval_set_pscid(&cmd,
>> + FIELD_GET(RISCV_IOMMU_PC_TA_PSCID, pc->ta));
>> + } else {
>> + if (FIELD_GET(RISCV_IOMMU_DC_TC_PDTV, dc->tc) || (
>> + FIELD_GET(RISCV_IOMMU_DC_FSC_MODE, dc->fsc) ==
>> + RISCV_IOMMU_DC_FSC_MODE_BARE)) {
>> + /* IOTINVAL.VMA with GV=AV=PSCV=0 */
>> + riscv_iommu_cmd_inval_vma(&cmd);
>> + } else {
>> + /*
>> + * IOTINVAL.VMA with GV=AV=0, and PSCV=1, and
>> + * PSCID=DC.ta.PSCID
>> + */
>> + riscv_iommu_cmd_inval_vma(&cmd);
>> + riscv_iommu_cmd_inval_set_pscid(&cmd,
>> + FIELD_GET(RISCV_IOMMU_DC_TA_PSCID, dc->ta));
>> + }
>> + }
>> + } else {
>> + if (inval_pdt) {
>> + /*
>> + * IOTINVAL.VMA with GV=1, AV=0, and PSCV=1, and
>> + * GSCID=DC.iohgatp.GSCID, PSCID=PC.PSCID
>> + */
>> + riscv_iommu_cmd_inval_vma(&cmd);
>> + riscv_iommu_cmd_inval_set_gscid(&cmd,
>> + FIELD_GET(RISCV_IOMMU_DC_IOHGATP_GSCID, iohgatp));
>> + riscv_iommu_cmd_inval_set_pscid(&cmd,
>> + FIELD_GET(RISCV_IOMMU_PC_TA_PSCID, pc->ta));
>The riscv_iommu_cmd_inval_vma() and riscv_iommu_cmd_inval_set_pscid()
>could be moved out to prevent duplicate code.
Thanks, Agreed on reducing duplication.
I'll move riscv_iommu_cmd_inval_vma() to a common path (build the VMA command
once), and only conditionally call riscv_iommu_cmd_inval_set_pscid() when
PSCV=1 is required by the spec.
>
>> + } else {
>> + /*
>> + * IOTINVAL.VMA with GV=1,AV=PSCV=0,and
>> + * GSCID=DC.iohgatp.GSCID
>> + */
>> + riscv_iommu_cmd_inval_vma(&cmd);
>> + riscv_iommu_cmd_inval_set_gscid(&cmd,
>> + FIELD_GET(RISCV_IOMMU_DC_IOHGATP_GSCID, iohgatp));
>> +
>> + /*
>> + * IOTINVAL.GVMA with GV=1,AV=0,and
>> + * GSCID=DC.iohgatp.GSCID
>> + */
>> + /*
>> + * For now, the Second-Stage feature have not yet been merged, so
>> + * let's comment out the code first.
>> + */
>> +#if 0
>> + riscv_iommu_cmd_send(iommu, &cmd);
>> + memset(&cmd, 0, sizeof(cmd));
>> + riscv_iommu_cmd_inval_gvma(&cmd);
>> + riscv_iommu_cmd_inval_set_gscid(&cmd,
>> + FIELD_GET(RISCV_IOMMU_DC_IOHGATP_GSCID, iohgatp));
>> +#endif
>All the above should be removed from the patch; we don't need draft code.
Agreed, I will drop the entire #if 0 draft block and add a TODO here.
>
>> + }
>> + }
>> + riscv_iommu_cmd_send(iommu, &cmd);
>> +}
>> /*
>> * Update IODIR for the device.
>> *
>> @@ -1031,6 +1106,11 @@ static void riscv_iommu_iodir_update(struct riscv_iommu_device *iommu,
>> riscv_iommu_cmd_iodir_inval_ddt(&cmd);
>> riscv_iommu_cmd_iodir_set_did(&cmd, fwspec->ids[i]);
>> riscv_iommu_cmd_send(iommu, &cmd);
>> + /*
>> + * For now, the SVA and PASID features have not yet been merged, the
>> + * default configuration is inval_pdt=false and pc=NULL.
>> + */
>> + riscv_iommu_iodir_iotinval(iommu, false, dc->iohgatp, dc, NULL);
>The riscv_iommu_iodir_iotinval() is the same level as
>riscv_iommu_iodir_update(). Could we move it out and put it after
>riscv_iommu_iodir_update()?
In riscv_iommu_iodir_update() we first clear the valid bit in dc->tc, an
IOTINVAL is required immediately after modifying the DDT/PDT entry to make
the change effective.
So riscv_iommu_iodir_iotinval() is paired with the DDT/PDT update sequence at the
same level as riscv_iommu_cmd_iodir_inval_ddt(), and keeping it adjacent preserves
the required ordering and avoids a window where stale cached context could be used.
>
>> sync_required = true;
>> }
>>
>> @@ -1055,6 +1135,11 @@ static void riscv_iommu_iodir_update(struct riscv_iommu_device *iommu,
>Why do you have two riscv_iommu_iodir_update() function definitions?
>
>> /* Invalidate device context after update */
>> riscv_iommu_cmd_iodir_inval_ddt(&cmd);
>> riscv_iommu_cmd_iodir_set_did(&cmd, fwspec->ids[i]);
>> + /*
>> + * For now, the SVA and PASID features have not yet been merged, the
>> + * default configuration is inval_pdt=false and pc=NULL.
>> + */
>> + riscv_iommu_iodir_iotinval(iommu, false, dc->iohgatp, dc, NULL);
>If IOTLB invalidation occurs before DDT_CACHE invalidation, the IOTLB
>may use DDT_CACHE's stall info, which may cause IOTLB invalidation to
>fail.
You're right. I placed riscv_iommu_iodir_iotinval() before riscv_iommu_cmd_send()
by mistake, I’ll fix the ordering in v2.
>
>> riscv_iommu_cmd_send(iommu, &cmd);
>> }
>>
>> --
>> 2.50.1
>>
>
>--
>Best Regards
> Guo Ren
Thanks,
Fangyu
Powered by blists - more mailing lists