Re: Re: [PATCH v3 1/2] RISC-V: KVM: Support runtime configuration for per-VM's HGATP mode

[fangyu.yu@linux.alibaba.com]

>> From: Fangyu Yu <fangyu.yu@linux.alibaba.com>
>>
>> Introduces one per-VM architecture-specific fields to support runtime
>> configuration of the G-stage page table format:
>>
>> - kvm->arch.kvm_riscv_gstage_pgd_levels: the corresponding number of page
>>   table levels for the selected mode.
>>
>> These fields replace the previous global variables
>> kvm_riscv_gstage_mode and kvm_riscv_gstage_pgd_levels, enabling different
>> virtual machines to independently select their G-stage page table format
>> instead of being forced to share the maximum mode detected by the kernel
>> at boot time.
>>
>> Signed-off-by: Fangyu Yu <fangyu.yu@linux.alibaba.com>
>> ---
>> diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
>> @@ -87,6 +87,22 @@ struct kvm_vcpu_stat {
>>  struct kvm_arch_memory_slot {
>>  };
>>  
>> +static inline unsigned long kvm_riscv_gstage_mode(unsigned long pgd_levels)
>> +{
>> +	switch (pgd_levels) {
>> +	case 2:
>> +		return HGATP_MODE_SV32X4;
>> +	case 3:
>> +		return HGATP_MODE_SV39X4;
>> +	case 4:
>> +		return HGATP_MODE_SV48X4;
>> +	case 5:
>> +		return HGATP_MODE_SV57X4;
>> +	default:
>> +		return HGATP_MODE_OFF;
>
>I think default should be an internal error.
>We can do "case 0: return HGATP_MODE_OFF;", or just error it too since
>KVM shouldn't ever ask for mode without protection anyway.

Good point. Returning HGATP_MODE_OFF in the default case would hide an
internal bug (unexpected pgd_levels). I’ll treat it as an internal error
instead, Something like:
    default:
        WARN_ON_ONCE(1);
        return HGATP_MODE_OFF;
    }

>> diff --git a/arch/riscv/kvm/gstage.c b/arch/riscv/kvm/gstage.c
>> @@ -319,41 +321,48 @@ void __init kvm_riscv_gstage_mode_detect(void)
>> +unsigned long kvm_riscv_gstage_gpa_bits(struct kvm_arch *ka)
>> +{
>> +	return (HGATP_PAGE_SHIFT +
>> +		ka->kvm_riscv_gstage_pgd_levels * kvm_riscv_gstage_index_bits +
>> +		kvm_riscv_gstage_pgd_xbits);
>> +}
>> +
>> +gpa_t kvm_riscv_gstage_gpa_size(struct kvm_arch *ka)
>> +{
>> +	return BIT_ULL(kvm_riscv_gstage_gpa_bits(ka));
>> +}
>
>Please define these two functions as static inline in the header files.
>They used to be just macros there, so it'd be safer not put LTO into the
>equation.

Agreed.

>> diff --git a/arch/riscv/kvm/main.c b/arch/riscv/kvm/main.c
>> @@ -105,17 +105,17 @@ static int __init riscv_kvm_init(void)
>>  		return rc;
>>  
>>  	kvm_riscv_gstage_mode_detect();
>> -	switch (kvm_riscv_gstage_mode) {
>> -	case HGATP_MODE_SV32X4:
>> +	switch (kvm_riscv_gstage_max_pgd_levels) {
>> +	case 2:
>>  		str = "Sv32x4";
>>  		break;
>> -	case HGATP_MODE_SV39X4:
>> +	case 3:
>>  		str = "Sv39x4";
>>  		break;
>> -	case HGATP_MODE_SV48X4:
>> +	case 4:
>>  		str = "Sv48x4";
>>  		break;
>> -	case HGATP_MODE_SV57X4:
>> +	case 5:
>>  		str = "Sv57x4";
>>  		break;
>>  	default:
>> @@ -164,7 +164,7 @@ static int __init riscv_kvm_init(void)
>>  			 (rc) ? slist : "no features");
>>  	}
>>  
>> -	kvm_info("using %s G-stage page table format\n", str);
>> +	kvm_info("Max G-stage page table format %s\n", str);
>
>Fun fact: the ISA doesn't define the same hierarchy for hgatp modes as
>it does for satp modes, so we could have just Sv57x4 and nothing below.
>
>We could do just with a code comment that we're assuming vendors will do
>better, but I'd rather not introduce more assumptions...
>I think the easiest would be to kvm_riscv_gstage_mode_detect() levels in
>reverse and stop on the first one that is not supported.
>(I'll reply with a patch later.)

Please refer to the discussion here:
https://github.com/riscv/riscv-isa-manual/issues/2208
If Sv57x4 is implemented, then Sv48x4 and Sv39x4 must also be implemented.

>Thanks.

Thanks,
Fangyu