Re: [RFC PATCH 17/25] target/i386/mshv: Implement mshv_get_special_regs()

[Wei Liu]

On Tue, May 20, 2025 at 01:30:10PM +0200, Magnus Kulke wrote:
> Retrieve special registers (e.g. segment, control, and descriptor
> table registers) from MSHV vCPUs.
> 
> Various helper functions to map register state representations between
> Qemu and MSHV are introduced.
> 
> Signed-off-by: Magnus Kulke <magnusku...@linux.microsoft.com>
> ---
>  include/system/mshv.h       |   1 +
>  target/i386/mshv/mshv-cpu.c | 118 +++++++++++++++++++++++++++++++++++-
>  2 files changed, 117 insertions(+), 2 deletions(-)
> 
> diff --git a/include/system/mshv.h b/include/system/mshv.h
> index 9b78b66a24..055489a6f3 100644
> --- a/include/system/mshv.h
> +++ b/include/system/mshv.h
> @@ -109,6 +109,7 @@ void mshv_init_cpu_logic(void);
>  int mshv_create_vcpu(int vm_fd, uint8_t vp_index, int *cpu_fd);
>  void mshv_remove_vcpu(int vm_fd, int cpu_fd);
>  int mshv_get_standard_regs(CPUState *cpu);
> +int mshv_get_special_regs(CPUState *cpu);
>  int mshv_run_vcpu(int vm_fd, CPUState *cpu, hv_message *msg, MshvVmExit 
> *exit);
>  int mshv_load_regs(CPUState *cpu);
>  int mshv_store_regs(CPUState *cpu);
> diff --git a/target/i386/mshv/mshv-cpu.c b/target/i386/mshv/mshv-cpu.c
> index 41584c3f8e..979ee5b8c3 100644
> --- a/target/i386/mshv/mshv-cpu.c
> +++ b/target/i386/mshv/mshv-cpu.c
> @@ -58,6 +58,27 @@ static enum hv_register_name STANDARD_REGISTER_NAMES[18] = 
> {
>      HV_X64_REGISTER_RFLAGS,
>  };
>  
> +static enum hv_register_name SPECIAL_REGISTER_NAMES[18] = {
[...]
> +    HV_REGISTER_PENDING_INTERRUPTION,

Why do you think this is needed?

> +};
> +
>  static void add_cpu_guard(int cpu_fd)
>  {
>      QemuMutex *guard;
> @@ -215,6 +236,94 @@ int mshv_get_standard_regs(CPUState *cpu)
>      return 0;
>  }
>  
> +static void populate_segment_reg(const hv_x64_segment_register *hv_seg,
> +                                 SegmentCache *seg)
> +{
> +    memset(seg, 0, sizeof(SegmentCache));
> +
> +    seg->base = hv_seg->base;
> +    seg->limit = hv_seg->limit;
> +    seg->selector = hv_seg->selector;
> +
> +    seg->flags = (hv_seg->segment_type << DESC_TYPE_SHIFT)
> +                 | (hv_seg->present * DESC_P_MASK)
> +                 | (hv_seg->descriptor_privilege_level << DESC_DPL_SHIFT)
> +                 | (hv_seg->_default << DESC_B_SHIFT)
> +                 | (hv_seg->non_system_segment * DESC_S_MASK)
> +                 | (hv_seg->_long << DESC_L_SHIFT)
> +                 | (hv_seg->granularity * DESC_G_MASK)
> +                 | (hv_seg->available * DESC_AVL_MASK);
> +
> +}
> +
> +static void populate_table_reg(const hv_x64_table_register *hv_seg,
> +                               SegmentCache *tbl)
> +{
> +    memset(tbl, 0, sizeof(SegmentCache));
> +
> +    tbl->base = hv_seg->base;
> +    tbl->limit = hv_seg->limit;
> +}
> +
> +static void populate_special_regs(const hv_register_assoc *assocs,
> +                                  X86CPU *x86cpu)
> +{
> +    CPUX86State *env = &x86cpu->env;
> +
> +    populate_segment_reg(&assocs[0].value.segment, &env->segs[R_CS]);
> +    populate_segment_reg(&assocs[1].value.segment, &env->segs[R_DS]);
> +    populate_segment_reg(&assocs[2].value.segment, &env->segs[R_ES]);
> +    populate_segment_reg(&assocs[3].value.segment, &env->segs[R_FS]);
> +    populate_segment_reg(&assocs[4].value.segment, &env->segs[R_GS]);
> +    populate_segment_reg(&assocs[5].value.segment, &env->segs[R_SS]);
> +
> +    /* TODO: should we set TR + LDT? */
> +    /* populate_segment_reg(&assocs[6].value.segment, &regs->tr); */
> +    /* populate_segment_reg(&assocs[7].value.segment, &regs->ldt); */
> +
> +    populate_table_reg(&assocs[8].value.table, &env->gdt);
> +    populate_table_reg(&assocs[9].value.table, &env->idt);
> +
> +    env->cr[0] = assocs[10].value.reg64;
> +    env->cr[2] = assocs[11].value.reg64;
> +    env->cr[3] = assocs[12].value.reg64;
> +    env->cr[4] = assocs[13].value.reg64;
> +
> +    cpu_set_apic_tpr(x86cpu->apic_state, assocs[14].value.reg64);
> +    env->efer = assocs[15].value.reg64;
> +    cpu_set_apic_base(x86cpu->apic_state, assocs[16].value.reg64);
> +
> +    /* TODO: should we set those? */
> +    /* pending_reg = assocs[17].value.pending_interruption.as_uint64; */
> +    /* populate_interrupt_bitmap(pending_reg, regs->interrupt_bitmap); */

If QEMU never touches it, then there is no need to set it.

> +}
> +
> +
> +int mshv_get_special_regs(CPUState *cpu)
> +{
> +    size_t n_regs = sizeof(SPECIAL_REGISTER_NAMES) / 
> sizeof(hv_register_name);
> +    struct hv_register_assoc *assocs;
> +    int ret;
> +    X86CPU *x86cpu = X86_CPU(cpu);
> +    int cpu_fd = mshv_vcpufd(cpu);
> +
> +    assocs = g_new0(hv_register_assoc, n_regs);
> +    for (size_t i = 0; i < n_regs; i++) {
> +        assocs[i].name = SPECIAL_REGISTER_NAMES[i];
> +    }
> +    ret = get_generic_regs(cpu_fd, assocs, n_regs);
> +    if (ret < 0) {
> +        error_report("failed to get special registers");
> +        g_free(assocs);
> +        return -errno;
> +    }
> +
> +    populate_special_regs(assocs, x86cpu);
> +
> +    g_free(assocs);
> +    return 0;
> +}
> +
>  int mshv_load_regs(CPUState *cpu)
>  {
>      int ret;
> @@ -225,8 +334,13 @@ int mshv_load_regs(CPUState *cpu)
>          return -1;
>      }
>  
> -     error_report("unimplemented");
> -     abort();
> +    ret = mshv_get_special_regs(cpu);
> +    if (ret < 0) {
> +        error_report("Failed to load special registers");
> +        return -1;
> +    }
> +
> +    return 0;

Ah so you changed the code in this patch.

Thanks,
Wei.

>  }
>  
>  int mshv_arch_put_registers(const CPUState *cpu)
> -- 
> 2.34.1
>