Re: [PATCH v1 1/1] target/i386: Save/restore the nested flag of an exception

[Xin Li <xin@zytor.com>]

On 8/23/2025 3:11 AM, Zhao Liu wrote:
> On Wed, Jul 23, 2025 at 11:22:11AM -0700, Xin Li (Intel) wrote:
>> Date: Wed, 23 Jul 2025 11:22:11 -0700
>> From: "Xin Li (Intel)" <xin@zytor.com>
>> Subject: [PATCH v1 1/1] target/i386: Save/restore the nested flag of an
>>   exception
>> X-Mailer: git-send-email 2.50.1
>>
>> Save/restore the nested flag of an exception during VM save/restore
>> and live migration to ensure a correct event stack level is chosen
>> when a nested exception is injected through FRED event delivery.
>>
>> The event stack level used by FRED event delivery depends on whether
>> the event was a nested exception encountered during delivery of an
>> earlier event, because a nested exception is "regarded" as happening
>> on ring 0.  E.g., when #PF is configured to use stack level 1 in
>> IA32_FRED_STKLVLS MSR:
>>    - nested #PF will be delivered on the stack pointed by IA32_FRED_RSP1
>>      MSR when encountered in ring 3 and ring 0.
>>    - normal #PF will be delivered on the stack pointed by IA32_FRED_RSP0
>>      MSR when encountered in ring 3.
>>    - normal #PF will be delivered on the stack pointed by IA32_FRED_RSP1
>>      MSR when encountered in ring 0.
>>
>> As such Qemu needs to track if an event is a nested event during VM
>> context save/restore and live migration.
>>
>> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
>> ---
>>   linux-headers/asm-x86/kvm.h |  4 +++-
>>   linux-headers/linux/kvm.h   |  1 +
>>   target/i386/cpu.c           |  1 +
>>   target/i386/cpu.h           |  1 +
>>   target/i386/kvm/kvm.c       | 35 +++++++++++++++++++++++++++++++++++
>>   target/i386/kvm/kvm_i386.h  |  1 +
>>   target/i386/machine.c       |  1 +
>>   7 files changed, 43 insertions(+), 1 deletion(-)
> 
>> diff --git a/target/i386/kvm/kvm_i386.h b/target/i386/kvm/kvm_i386.h
>> index 5f83e8850a..7e765b6833 100644
>> --- a/target/i386/kvm/kvm_i386.h
>> +++ b/target/i386/kvm/kvm_i386.h
>> @@ -54,6 +54,7 @@ typedef struct KvmCpuidInfo {
>>   bool kvm_is_vm_type_supported(int type);
>>   bool kvm_has_adjust_clock_stable(void);
>>   bool kvm_has_exception_payload(void);
>> +bool kvm_has_exception_nested_flag(void);
>>   void kvm_synchronize_all_tsc(void);
>>   
>>   void kvm_get_apic_state(DeviceState *d, struct kvm_lapic_state *kapic);
>> diff --git a/target/i386/machine.c b/target/i386/machine.c
>> index dd2dac1d44..a452d2c97e 100644
>> --- a/target/i386/machine.c
>> +++ b/target/i386/machine.c
>> @@ -458,6 +458,7 @@ static const VMStateDescription vmstate_exception_info = {
>>           VMSTATE_UINT8(env.exception_injected, X86CPU),
>>           VMSTATE_UINT8(env.exception_has_payload, X86CPU),
>>           VMSTATE_UINT64(env.exception_payload, X86CPU),
>> +        VMSTATE_UINT8(env.exception_is_nested, X86CPU),
> 
> A new field needs to bump up the version of vmstate_exception_info, but
> I'm afraid this will break backward-migration compatibility. So what
> about adding a subsction? For example,
> 
> diff --git a/target/i386/machine.c b/target/i386/machine.c
> index a452d2c97e4c..6ce3cb8af6a6 100644
> --- a/target/i386/machine.c
> +++ b/target/i386/machine.c
> @@ -433,6 +433,24 @@ static bool steal_time_msr_needed(void *opaque)
>       return cpu->env.steal_time_msr != 0;
>   }
> 
> +static bool exception_nested_needed(void *opaque)
> +{
> +    X86CPU *cpu = opaque;
> +
> +    return cpu->env.exception_is_nested;
> +}
> +
> +static const VMStateDescription vmstate_exceprtion_nested = {
> +    .name = "cpu/exception_nested",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = exception_nested_needed,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_UINT8(env.exception_is_nested, X86CPU),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>   static bool exception_info_needed(void *opaque)
>   {
>       X86CPU *cpu = opaque;
> @@ -458,8 +476,11 @@ static const VMStateDescription vmstate_exception_info = {
>           VMSTATE_UINT8(env.exception_injected, X86CPU),
>           VMSTATE_UINT8(env.exception_has_payload, X86CPU),
>           VMSTATE_UINT64(env.exception_payload, X86CPU),
> -        VMSTATE_UINT8(env.exception_is_nested, X86CPU),
>           VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (const VMStateDescription * const []) {
> +        &vmstate_exceprtion_nested,
> +        NULL,
>       }
>   };
> 
> ---

Yeah, looks the right way to go.


> In addition, I think it's better to update header files in a seperate
> patch.
> 

Is it a protocol that the Qemu community prefers?

Otherwise, the patch is self-contained, and not big, why break it?

Thanks!
     Xin