Re: [RFC PATCH v3 01/10] iommupt: Add RISC-V Second-stage (iohgatp) page table support

[fangyu . yu]

>> @@ -126,6 +135,15 @@ riscvpt_entry_num_contig_lg2(const struct pt_state *pts)
>>
>>  static inline unsigned int riscvpt_num_items_lg2(const struct pt_state *pts)
>>  {
>> +	/*
>> +	 * Second-stage (iohgatp) root page tables have 4x the usual number of
>> +	 * entries (2048 = 2^11 instead of 512 = 2^9) to cover the 2 extra GPA
>> +	 * bits in Sv39x4/Sv48x4/Sv57x4.  Only the root (top) level is
>> +	 * enlarged; all other levels remain at the standard 9-bit index width.
>> +	 */
>> +	if (pts_feature(pts, PT_FEAT_RISCV_S2) &&
>> +	    pts->level == pts->range->top_level)
>> +		return PT_TABLEMEM_LG2SZ - ilog2(sizeof(u64)) + 2;
>
>You shouldn't need this, see how ARMv8 is constructed which does the
>same thing. Instead
>
>static inline unsigned int armv8pt_num_items_lg2(const struct pt_state *pts)
>{
>	/*
>	 * It is not allowed to call pt_num_items_lg2() at the top level, this
>	 * API restriction is specifically an optimization avoid overheads
>	 * dealing with concatenated tables here.
>	 */
>	PT_WARN_ON(pts->level == pts->range->top_level);
>

Thanks for pointing at ARMv8 for comparison. I hadn't noticed your
newly posted patch [1], Now that I've looked at it, I agree
armv8pt_num_items_lg2() is the right way to implement this and
will follow that pattern for RISC-V in the next version.

>> @@ -272,6 +291,19 @@ riscvpt_iommu_fmt_init(struct pt_iommu_riscv_64 *iommu_table,
>>  	case 57:
>>  		pt_top_set_level(&table->common, 4);
>>  		break;
>> +	/*
>> +	 * Second-stage (iohgatp): Sv39x4 / Sv48x4 / Sv57x4.
>> +	 * The top level is the same as for the first-stage counterpart.
>> +	 */
>> +	case 41:
>> +		pt_top_set_level(&table->common, 2);
>> +		break;
>> +	case 50:
>> +		pt_top_set_level(&table->common, 3);
>> +		break;
>> +	case 59:
>> +		pt_top_set_level(&table->common, 4);
>> +		break;
>
>Are all these widths valid for both S1 and S2? It should reject
>illegal vasz..
>

Agreed, will fix. 39/48/57 are only valid for S1 and 41/50/59 are only
valid for S2 -- I'll gate each group on PT_FEAT_RISCV_S2 and reject the
other combination.

>>  struct pt_iommu_riscv_64_hw_info {
>>  	u64 ppn;
>> -	u8 fsc_iosatp_mode;
>> +	union {
>> +		/*
>> +		 * First-stage (fsc/iosatp) MODE encoding:
>> +		 *   8 = Sv39, 9 = Sv48, 10 = Sv57
>> +		 * Used to program DC.fsc.iosatp.MODE.
>> +		 */
>> +		u8 fsc_iosatp_mode;
>> +		/*
>> +		 * Second-stage (iohgatp) MODE encoding:
>> +		 *   8 = Sv39x4, 9 = Sv48x4, 10 = Sv57x4
>> +		 * Used to program DC.iohgatp.MODE.
>> +		 * The numeric values are identical to fsc_iosatp_mode;
>> +		 * the caller selects the interpretation based on domain type.
>> +		 */
>> +		u8 iohgatp_mode;
>> +	};
>
>suggest not using a union and instead have the get_info populate only
>the one correct for the fmt. It makes it slightly more robust that
>s1/s2 don't get intermixed improperly?
>

Makes sense, will drop the union in favor of two separate fields and
have riscvpt_iommu_fmt_hw_info() only fill in the one that matches the
format's stage.

Thanks,
Fangyu


>Jason

[1] https://lore.kernel.org/linux-iommu/0-v2-563ee63886f0+1209-iommupt_armv8_jgg@xxxxxxxxxx/