Re: [PATCH 3/5] target/i386/mshv: remove fallback for register page get registers

[Magnus Kulke <magnuskulke@linux.microsoft.com>]

On Wed, Jul 08, 2026 at 01:16:43AM +0300, Doru Blânzeanu wrote:
> Change `load_regs` to use the register page when it is mmapped
> and is valid.
> Eliminate the hypercall based logic and fail in case the register page
> is found in an unexpected state.
> 
> When retrieving the special registers, there are some registers that are
> not present in the register page: TR, LDTR, GDTR, IDTR, CR2, APIC_BASE.
> As this registers are not likely to be used in an MMIO/PIO operation,
> and to avoid a hypercall overhead we do not retrieve them.
> 
> Local testing showed no regression when using this logic. To properly
> retrieve all the necessary registers for each decoded operation implies
> having a mechanism that tracks the state of each register, which is
> beyond the scope of this patch series.
> 
> Fixes: 40072a7391
> Signed-off-by: Doru Blânzeanu <dblanzeanu@linux.microsoft.com>
> ---
>  target/i386/mshv/mshv-cpu.c | 107 +++++++++++-------------------------
>  1 file changed, 31 insertions(+), 76 deletions(-)
> 
> diff --git a/target/i386/mshv/mshv-cpu.c b/target/i386/mshv/mshv-cpu.c
> index c782715497..9ec5c19c67 100644
> --- a/target/i386/mshv/mshv-cpu.c
> +++ b/target/i386/mshv/mshv-cpu.c
> @@ -270,15 +270,6 @@ static int get_xc_reg(CPUState *cpu)
>      return 0;
>  }
>  
> -static enum hv_register_name NON_VP_PAGE_REGISTER_NAMES[6] = {
> -    HV_X64_REGISTER_TR,
> -    HV_X64_REGISTER_LDTR,
> -    HV_X64_REGISTER_GDTR,
> -    HV_X64_REGISTER_IDTR,
> -    HV_X64_REGISTER_CR2,
> -    HV_X64_REGISTER_APIC_BASE,
> -};
> -
>  static int translate_gva(const CPUState *cpu, uint64_t gva, uint64_t *gpa,
>                           uint64_t flags)
>  {
> @@ -615,7 +606,7 @@ static void populate_special_regs(const hv_register_assoc *assocs,
>      cpu_set_apic_base(x86cpu->apic_state, assocs[16].value.reg64);
>  }
>  
> -static void mshv_get_standard_regs_vp_page(CPUState *cpu)
> +static void get_standard_regs_vp_page(CPUState *cpu)
>  {
>      X86CPU *x86cpu = X86_CPU(cpu);
>      CPUX86State *env = &x86cpu->env;
> @@ -643,13 +634,19 @@ static void mshv_get_standard_regs_vp_page(CPUState *cpu)
>      rflags_to_lflags(env);
>  }
>  
> -static int mshv_get_special_regs_vp_page(CPUState *cpu)
> +/*
> + * This function synchronizes the special registers present in the
> + * register vp page, which are not all the special registers.
> + * The rest of the special registers (LD, TR, GDT, IDT, CR2, APIC_BASE)
> + * are not synchronized to avoid the overhead of a hypercall.
> + *
> + * These special registers are not normally used by the guest,
> + * and are only used in some specific cases.
> + */
> +static void get_special_regs_vp_page(CPUState *cpu)
>  {
>      X86CPU *x86cpu = X86_CPU(cpu);
>      CPUX86State *env = &x86cpu->env;
> -    struct hv_register_assoc assocs[ARRAY_SIZE(NON_VP_PAGE_REGISTER_NAMES)];
> -    int ret;
> -    size_t n_regs = ARRAY_SIZE(NON_VP_PAGE_REGISTER_NAMES);
>      hv_x64_segment_register seg;
>  
>      /* Populate special registers that are in the VP register page */
> @@ -672,46 +669,15 @@ static int mshv_get_special_regs_vp_page(CPUState *cpu)
>      populate_segment_reg(&seg, &env->segs[R_FS]);
>      memcpy(&seg, &env->regs_page->gs, sizeof(hv_x64_segment_register));
>      populate_segment_reg(&seg, &env->segs[R_GS]);
> -
> -    /* The rest of the special registers that are not in the VP register page */
> -    for (size_t i = 0; i < n_regs; i++) {
> -        assocs[i].name = NON_VP_PAGE_REGISTER_NAMES[i];
> -    }
> -
> -    ret = mshv_get_generic_regs(cpu, assocs, n_regs);
> -    if (ret < 0) {
> -        error_report("failed to get non-vp-page special registers");
> -        return -1;
> -    }
> -
> -    /* Non-VP page registers - TR, LDTR, GDTR, IDTR, CR2, APIC_BASE */
> -    populate_segment_reg(&assocs[0].value.segment, &env->tr);
> -    populate_segment_reg(&assocs[1].value.segment, &env->ldt);
> -
> -    populate_table_reg(&assocs[2].value.table, &env->gdt);
> -    populate_table_reg(&assocs[3].value.table, &env->idt);
> -    env->cr[2] = assocs[4].value.reg64;
> -
> -    cpu_set_apic_base(x86cpu->apic_state, assocs[5].value.reg64);
> -
> -    return ret;
>  }
>  
> -static int mshv_get_registers_vp_page(CPUState *cpu)
> +static void get_registers_vp_page(CPUState *cpu)
>  {
> -    int ret;
> -
>      /* General Purpose Registers  */
> -    mshv_get_standard_regs_vp_page(cpu);
> +    get_standard_regs_vp_page(cpu);
>  
> -    /* Special Registers - makes a hypercall */
> -    ret = mshv_get_special_regs_vp_page(cpu);
> -    if (ret < 0) {
> -        error_report("failed to get special registers for vp page");
> -        return -1;
> -    }
> -
> -    return 0;
> +    /* Special Registers */
> +    get_special_regs_vp_page(cpu);
>  }
>  
>  
> @@ -735,29 +701,26 @@ static int get_special_regs(CPUState *cpu)
>      return 0;
>  }
>  
> -static int load_regs(CPUState *cpu)
> +static void load_regs(CPUState *cpu)
>  {
>      X86CPU *x86_cpu = X86_CPU(cpu);
>      CPUX86State *env = &x86_cpu->env;
> -    int ret;
> -
> -    /* Use register vp page to optimize registers access */
> -    if (env->regs_page && env->regs_page->isvalid != 0) {
> -        ret = mshv_get_registers_vp_page(cpu);
> -        return ret;
> -    }
>  
> -    ret = get_standard_regs(cpu);
> -    if (ret < 0) {
> -        return ret;
> +    /* Check register page pointer and abort if in unexpected state */
> +    if (!env->regs_page) {
> +        error_report(
> +                "load regs: register page not set for vcpu %d",
> +                cpu->cpu_index);
> +        abort();
>      }
> -
> -    ret = get_special_regs(cpu);
> -    if (ret < 0) {
> -        return ret;
> +    if (env->regs_page->isvalid == 0) {
> +        error_report(
> +                "load regs: register page invalid for vcpu %d",
> +                cpu->cpu_index);
> +        abort();
>      }
>  
> -    return 0;
> +    get_registers_vp_page(cpu);
>  }
>  
>  static int get_vcpu_events(CPUState *cpu)
> @@ -1565,11 +1528,7 @@ static int emulate_instruction(CPUState *cpu,
>      int ret;
>      x86_insn_stream stream = { .bytes = insn_bytes, .len = insn_len };
>  
> -    ret = load_regs(cpu);
> -    if (ret < 0) {
> -        error_report("Failed to load registers");
> -        return -1;
> -    }
> +    load_regs(cpu);
>  
>      decode_instruction_stream(env, &decode, &stream);
>      exec_instruction(env, &decode);
> @@ -1872,11 +1831,7 @@ static int handle_pio_str(CPUState *cpu, hv_x64_io_port_intercept_message *info)
>      X86CPU *x86_cpu = X86_CPU(cpu);
>      CPUX86State *env = &x86_cpu->env;
>  
> -    ret = load_regs(cpu);
> -    if (ret < 0) {
> -        error_report("Failed to load registers");
> -        return -1;
> -    }
> +    load_regs(cpu);
>  
>      direction_flag = (env->eflags & DESC_E_MASK) != 0;
>  
> @@ -2013,7 +1968,7 @@ static void read_segment_descriptor(CPUState *cpu,
>  
>      /*
>       * SegmentCache stores the hypervisor-provided value verbatim (populated by
> -     * mshv_load_regs). We need to convert it to format expected by the
> +     * load_regs). We need to convert it to format expected by the
>       * instruction emulator. We can have a limit value > 0xfffff with
>       * granularity of 0 (byte granularity), which is not representable
>       * in real x86_segment_descriptor. In this case we set granularity to 1
> -- 
> 2.53.0

Reviewed-by: Magnus Kulke <magnuskulke@linux.microsoft.com>