Re: [PATCH] KVM: arm64: nv: Set ISTATUS for emulated timers, If timer expired

[Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>]

Hi Marc,

On 15-02-2025 11:20 pm, Marc Zyngier wrote:
> On Mon, 10 Feb 2025 18:26:48 +0000,
> Eric Auger <eauger@redhat.com> wrote:
>>
>> Hi Marc,
>>
>> On 2/7/25 7:38 PM, Marc Zyngier wrote:
>>> On Fri, 07 Feb 2025 18:09:58 +0000,
>>> Oliver Upton <oliver.upton@linux.dev> wrote:
>>>>
>>>> Hey,
>>>>
>>>> On Fri, Feb 07, 2025 at 05:45:33PM +0000, Marc Zyngier wrote:
>>>>> I found at least one issue that could fail the migration. Before the
>>>>> VM starts running, we limit the feature set to the subset we actually
>>>>> support with NV.
>>>>>
>>>>> By doing this, we also change the value of IDreg fields that are not
>>>>> writable, because they describe features that we don't support.
>>>>> Obviously, that fails on restore.
>>>>>
>>>>> I need to have a think...
>>>>
>>>> We spoke about this a while ago (and I forgot til now), but I was
>>>> wondering if we could use vCPU feature flags to describe NV, including
>>>> the selection between FEAT_E2H0 and FEAT_VHE.
>>>>
>>>> I think this might match userspace expectations a bit more closely where
>>>> the state of the ID registers after init gives the actual feature set
>>>> supported by the VM.
>>>
>>> I'm not sure that's enough. Let me give you an example:
>>>
>>> My host has FEAT_XNX, described in ID_AA64MMFR1_EL1.XNX. For whatever
>>> reason, we don't allow this field to be written to, even out of NV
>>> context. This is odd, because for an EL1 VM, this field means nothing
>>> at all.
>> So the curprit fields for me look like
>>
>> - ID_AA64MMFR1_EL1.XNX
>> - ID_AA64DFR0_EL1.DoubleLock
>> - ID_AA64PFR0_EL1.RAS
>>
>> This is still based on your nv-next branch from Jan 9
>> https://github.com/eauger/linux/tree/nv_next_jan9_2025
> 
> I have now pushed out a new nv-next branch with the new and improved
> UAPI. I expect migration to work a bit better, or at least not to
> explode on ID register restore. You will notice that things have
> changed a bit (extra flag and cap for FEAT_E2H0), but nothing really
> major.
> 

Tried nv-next branch and it is breaking(kernel Oops) for normal VM boot 
itself with qemu. Looks like this is happening since qemu is trying to 
write to ID_UNALLOCATED mapped registers as part of save-restore of 
registers.

Below diff fixes the issue,

[root@sut08sys-r112 arm-platforms]# git diff arch/arm64/kvm/sys_regs.c
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index e6f4599dca48..9459d25d4902 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2310,6 +2310,7 @@ static bool bad_redir_trap(struct kvm_vcpu *vcpu,
         .get_user = get_id_reg,                         \
         .set_user = set_id_reg,                         \
         .visibility = raz_visibility,                   \
+       .reset = kvm_read_sanitised_id_reg,             \
         .val = 0,                                       \
  }

#define ID_UNALLOCATED(crm, op2) {                      \
         Op0(3), Op1(0), CRn(0), CRm(crm), Op2(op2),     \
         .access = access_id_reg,                        \
         .get_user = get_id_reg,                         \
         .set_user = set_id_reg,                         \
         .visibility = raz_visibility,                   \
         .reset = kvm_read_sanitised_id_reg,             \
         .val = 0,                                       \
}


-- 
Thanks,
Ganapat/GK