[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <bb5c3dc1-8100-4d50-843a-3755c873362e@redhat.com>
Date: Mon, 1 Jul 2024 11:36:52 +0800
From: Shaoqin Huang <shahuang@...hat.com>
To: Marc Zyngier <maz@...nel.org>
Cc: Oliver Upton <oliver.upton@...ux.dev>, kvmarm@...ts.linux.dev,
James Morse <james.morse@....com>, Suzuki K Poulose
<suzuki.poulose@....com>, Zenghui Yu <yuzenghui@...wei.com>,
Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] KVM: arm64: Allow userspace to change
ID_AA64PFR1_EL1
Hi Marc,
On 6/29/24 16:55, Marc Zyngier wrote:
> On Fri, 28 Jun 2024 07:04:51 +0100,
> Shaoqin Huang <shahuang@...hat.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
Powered by blists - more mailing lists