Re: Re: [PATCH] iommu/riscv: Add IOTINVAL after updating DDT/PDT entries

[fangyu.yu@linux.alibaba.com]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 5328 bytes --]

>> From: Fangyu Yu <fangyu.yu@linux.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@linux.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) {
>
>nit: This condition can stick out. We have 100 chars.
>
Ack.

>> +		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)) {
>
>nit: formatting

Ack.

>			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));
>> +		} 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));
>
>The memset isn't necessary since riscv_iommu_cmd_inval_gvma(), which
>doesn't yet exist, will overwrite dword0 and zero out dword1.

Agreed, I’ll drop memset in v2.

>
>> +			riscv_iommu_cmd_inval_gvma(&cmd);
>> +			riscv_iommu_cmd_inval_set_gscid(&cmd,
>> +				FIELD_GET(RISCV_IOMMU_DC_IOHGATP_GSCID, iohgatp));
>> +#endif
>> +		}
>> +	}
>> +	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);
>>  		sync_required = true;
>>  	}
>>
>> @@ -1055,6 +1135,11 @@ static void riscv_iommu_iodir_update(struct riscv_iommu_device *iommu,
>>  		/* 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);
>>  		riscv_iommu_cmd_send(iommu, &cmd);
>>  	}
>>
>> --
>> 2.50.1
>>
>
>A faithful implementation of the 6.3.1 and 6.3.2 guidelines for what the
>code currently supports. I presume this is fixing an issue? If so, can you
>point that out in the commit message?

Yes, this is fixing a functional issue: when software changes a leaf-level
DDT or PDT entry we weren't issuing the required IOTINVAL.
I'll update the commit message in v2.

>
>Otherwise,
>
>Reviewed-by: Andrew Jones <andrew.jones@oss.qualcomm.com>
>
>Thanks,
>drew
>
Thanks,
Fangyu


[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv