Re: [PATCH v3 1/2] KVM: arm64: Allow userspace to change ID_AA64PFR1_EL1

[Shaoqin Huang <shahuang@redhat.com>]

Hi Marc,

On 6/29/24 16:55, Marc Zyngier wrote:
> On Fri, 28 Jun 2024 07:04:51 +0100,
> Shaoqin Huang <shahuang@redhat.com> wrote:
>>
>> Allow userspace to change the guest-visible value of the register with
>> some severe limitation:
>>
>>    - No changes to features not virtualized by KVM (MPAM_frac, RAS_frac,
>>      SME, RNDP_trap).
>>
>>    - No changes to features (CSV2_frac, NMI, MTE_frac, GCS, THE, MTEX,
>>      DF2, PFAR) which haven't been added into the ftr_id_aa64pfr1[].
>>      Because the struct arm64_ftr_bits definition for each feature in the
>>      ftr_id_aa64pfr1[] is used by arm64_check_features. If they're not
>>      existing in the ftr_id_aa64pfr1[], the for loop won't check the if
>>      the new_val is safe for those features.
>> ---
>>   arch/arm64/kvm/sys_regs.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> This is getting very tiring:
> 
> - this isn't a valid patch, as it doesn't carry a proper SoB. You did
>    it last time, I pointed it out, and you ignored me.
> 
> - this is *wrong*. The moment the kernel publishes any of the fields
>    you have decided to ignore, the restoring of a VM on a new kernel
>    will fail if the host and the VM have different values.
> 

Thanks for giving me the detail about why it's wrong, and thanks to your 
patience to tolerate my mistakes.

This time I understand what you mean and it's a good opportunity for me 
to learn professional knowledge from you.

>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 22b45a15d068..159cded22357 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -2306,7 +2306,9 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>   		   ID_AA64PFR0_EL1_GIC |
>>   		   ID_AA64PFR0_EL1_AdvSIMD |
>>   		   ID_AA64PFR0_EL1_FP), },
>> -	ID_SANITISED(ID_AA64PFR1_EL1),
>> +	ID_WRITABLE(ID_AA64PFR1_EL1, ID_AA64PFR1_EL1_MTE |
> 
> Why? If the VM has been created with MTE support, this must be obeyed.

I misunderstood the MTE support in KVM, and thanks your explanation 
below. This is absolutely wrong.

> 
>> +				     ID_AA64PFR1_EL1_SSBS |
>> +				     ID_AA64PFR1_EL1_BT),
>>   	ID_UNALLOCATED(4,2),
>>   	ID_UNALLOCATED(4,3),
>>   	ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),
> 
> So let me be very blunt:
> 
> - you *must* handle *all* the fields described in that register. There
>    are 15 valid fields there, and I want to see all 15 fields being
>    explicitly dealt with.

Yes, I totally understand that.

> 
> - fields that can be changed without ill effect must be enabled for
>    write, irrespective of what the cpufeature code does (CSV2_frac, for
>    example).

Ok, I remember that.

> 
> - fields that have a fixed value because KVM doesn't handle the
>    corresponding feature must be explicitly disabled in the register
>    accessor (MPAM, RNDR, MTEX, THE...). Just like we do for SME.

Ok, I think this is the point I misunderstood. Thus cause some unhappy 
to you. I want to say sorry to you and I really appreciate your 
expertise and patience to explain this to me.

> 
> - fields that correspond to a feature that is controlled by an
>    internal flag (MTE) must not be writable. Just like we do for PAuth
>    in ID_AA64ISAR1_EL1.

Got that.

> 
> - you *must* handle *all* the fields described in that register.
> 
> Until I see all of the above, I will not take this patch. If you don't
> want to do this, that's absolutely fine by me. Just don't post another
> patch. But if you do, this is the deal.

Thanks a lot you provided so much useful information about the writable 
register. With those standard, I won't post the invalid patch like this.

Thanks,
Shaoqin

> 
> 	M.
> 

-- 
Shaoqin