Re: [PATCH v1 1/1] x86: Add support save/load HWCR MSR

[Zhao Liu <zhao1.liu@intel.com>]

Hi Shiyuan,

On Thu, Sep 26, 2024 at 12:08:08PM +0800, Gao Shiyuan via wrote:
> Date: Thu, 26 Sep 2024 12:08:08 +0800
> From: Gao Shiyuan via <qemu-devel@nongnu.org>
> Subject: [PATCH v1 1/1] x86: Add support save/load HWCR MSR
> X-Mailer: git-send-email 2.39.3 (Apple Git-146)
> 
> KVM commit 191c8137a939 ("x86/kvm: Implement HWCR support")
> introduced support for emulating HWCR MSR.
> 
> Add support for QEMU to save/load this MSR for migration purposes.
> 
> Signed-off-by: Gao Shiyuan <gaoshiyuan@baidu.com>
> ---
>  target/i386/cpu.c     |  1 +
>  target/i386/cpu.h     |  5 +++++
>  target/i386/kvm/kvm.c | 12 ++++++++++++
>  target/i386/machine.c | 20 ++++++++++++++++++++
>  4 files changed, 38 insertions(+)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 85ef7452c0..339131a39a 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7093,6 +7093,7 @@ static void x86_cpu_reset_hold(Object *obj, ResetType type)
>      env->a20_mask = ~0x0;
>      env->smbase = 0x30000;
>      env->msr_smi_count = 0;
> +    env->hwcr = 0;

Why we need to clear it here? This needs to be explained in the commit
message.

>      env->idt.limit = 0xffff;
>      env->gdt.limit = 0xffff;
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 14edd57a37..a19b1ceda4 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -539,6 +539,8 @@ typedef enum X86Seg {
>  
>  #define MSR_AMD64_TSC_RATIO_DEFAULT     0x100000000ULL
>  
> +#define MSR_K7_HWCR                     0xc0010015
> +
>  #define MSR_VM_HSAVE_PA                 0xc0010117
>  
>  #define MSR_IA32_XFD                    0x000001c4
> @@ -1859,6 +1861,9 @@ typedef struct CPUArchState {
>      uint64_t msr_lbr_depth;
>      LBREntry lbr_records[ARCH_LBR_NR_ENTRIES];
>  
> +    /* Hardware Configuration MSR */

We can keep the same comment as msr_hwcr in KVM to emphasize this is an
AMD-specific MSR, i.e.,

/* AMD MSRC001_0015 Hardware Configuration */

> +    uint64_t hwcr;

Add the msr_ prefix to indicate that this value is only intended to
store the MSR. Currently, for similar members, some have the msr_ prefix
and some do not, but it is better to have it for clarity.

> +
>      /* exception/interrupt handling */
>      int error_code;
>      int exception_is_int;

-Zhao