Re: [PATCH v2 7/7] target/i386/mshv: fix pio handlers clobbering device-modified registers

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

On Tue, May 05, 2026 at 09:50:28PM +0300, Doru Blânzeanu wrote:
> When a device handler (e.g. vmport) calls cpu_synchronize_state() during
> I/O port dispatch, it sets cpu->accel->dirty = true and may modify
> registers directly in env. The old PIO code ignored this: it
> unconditionally wrote the stale info->rax from the VM-exit intercept
> message back to the hypervisor and then cleared dirty, discarding any
> register changes made by the device.
> 
> Bifurcate both handlers on cpu->accel->dirty:
> 
> handle_pio_non_str:
> - dirty path: update env->eip directly. For reads (IN), merge the I/O
>   result into env->regs[R_EAX] (which may have been modified by the
>   device) rather than info->rax. For writes (OUT), leave RAX untouched.
>   Flush all registers via mshv_store_regs() and clear dirty.
> - non-dirty path: write RIP and RAX via set_x64_registers hypercall as
>   before.
> 
> handle_pio_str:
> - dirty path: update env->eip and the appropriate index register
>   (RSI for OUTS, RDI for INS) directly. Flush via mshv_store_regs()
>   and clear dirty.
> - non-dirty path: write the index register and RIP via
>   set_x64_registers. Drop the RAX assignment that was here before;
>   string I/O does not modify RAX, and set_x64_registers is hardcoded
>   to write only 2 registers so the third slot was silently ignored
>   anyway.
> 
> Remove the unconditional "cpu->accel->dirty = false" at the end of both
> handlers. In the non-dirty fast path it was redundant (already false).
> In the dirty path it was actively harmful: it told the vcpu run loop
> that env was clean when it was not, losing the device's modifications.
> 
> Signed-off-by: Doru Blânzeanu <dblanzeanu@linux.microsoft.com>
> ---
>  target/i386/mshv/mshv-cpu.c | 82 ++++++++++++++++++++++++++-----------
>  1 file changed, 59 insertions(+), 23 deletions(-)
> 
> diff --git a/target/i386/mshv/mshv-cpu.c b/target/i386/mshv/mshv-cpu.c
> index 0cfac26a5c..7be3fdcc45 100644
> --- a/target/i386/mshv/mshv-cpu.c
> +++ b/target/i386/mshv/mshv-cpu.c
> @@ -1348,7 +1348,7 @@ static int pio_write(uint64_t port, const uint8_t *data, uintptr_t size,
>      return ret;
>  }
>  
> -static int handle_pio_non_str(const CPUState *cpu,
> +static int handle_pio_non_str(CPUState *cpu,
>                                hv_x64_io_port_intercept_message *info)
>  {
>      size_t len = info->access_info.access_size;
> @@ -1357,10 +1357,12 @@ static int handle_pio_non_str(const CPUState *cpu,
>      uint32_t val, eax;
>      const uint32_t eax_mask =  0xffffffffu >> (32 - len * 8);
>      size_t insn_len;
> -    uint64_t rip, rax;
> +    uint64_t rip;
>      uint32_t reg_names[2];
>      uint64_t reg_values[2];
>      uint16_t port = info->port_number;
> +    X86CPU *x86_cpu = X86_CPU(cpu);
> +    CPUX86State *env = &x86_cpu->env;
>  
>      if (access_type == HV_X64_INTERCEPT_ACCESS_TYPE_WRITE) {
>          union {
> @@ -1391,21 +1393,40 @@ static int handle_pio_non_str(const CPUState *cpu,
>  
>      /* Advance RIP and update RAX */
>      rip = info->header.rip + insn_len;
> -    rax = info->rax;
>  
> -    reg_names[0] = HV_X64_REGISTER_RIP;
> -    reg_values[0] = rip;
> -    reg_names[1] = HV_X64_REGISTER_RAX;
> -    reg_values[1] = rax;
> +    if (cpu->accel->dirty) {
> +        env->eip = rip;
> +        if (access_type != HV_X64_INTERCEPT_ACCESS_TYPE_WRITE) {
> +            /*
> +             * For reads, merge the I/O result into the current RAX.
> +             * Use env->regs[R_EAX] as the base since a device handler
> +             * (e.g. vmport) may have called cpu_synchronize_state()
> +             * and modified registers.
> +             */
> +            eax = (((uint32_t)env->regs[R_EAX]) & ~eax_mask)
> +                  | (val & eax_mask);
> +            env->regs[R_EAX] = (uint64_t)eax;
> +        }
> +        /* Sync modified standard registers back and clear dirty. */
> +        ret = mshv_store_regs(cpu);
> +        if (ret < 0) {
> +            error_report("Failed to store registers after PIO");
> +            return -1;
> +        }
> +        cpu->accel->dirty = false;
> +    } else {
> +        reg_names[0] = HV_X64_REGISTER_RIP;
> +        reg_values[0] = rip;
> +        reg_names[1] = HV_X64_REGISTER_RAX;
> +        reg_values[1] = info->rax;
>  
> -    ret = set_x64_registers(cpu, reg_names, reg_values);
> -    if (ret < 0) {
> -        error_report("Failed to set x64 registers");
> -        return -1;
> +        ret = set_x64_registers(cpu, reg_names, reg_values);
> +        if (ret < 0) {
> +            error_report("Failed to set x64 registers");
> +            return -1;
> +        }
>      }
>  
> -    cpu->accel->dirty = false;
> -
>      return 0;
>  }
>  
> @@ -1521,6 +1542,7 @@ static int handle_pio_str(CPUState *cpu, hv_x64_io_port_intercept_message *info)
>      bool repop = info->access_info.rep_prefix == 1;
>      size_t repeat = repop ? info->rcx : 1;
>      size_t insn_len = info->header.instruction_length;
> +    uint64_t rip;
>      bool direction_flag;
>      uint32_t reg_names[3];
>      uint64_t reg_values[3];
> @@ -1554,18 +1576,32 @@ static int handle_pio_str(CPUState *cpu, hv_x64_io_port_intercept_message *info)
>          reg_values[0] = info->rdi;
>      }
>  
> -    reg_names[1] = HV_X64_REGISTER_RIP;
> -    reg_values[1] = info->header.rip + insn_len;
> -    reg_names[2] = HV_X64_REGISTER_RAX;
> -    reg_values[2] = info->rax;
> +    rip = info->header.rip + insn_len;
>  
> -    ret = set_x64_registers(cpu, reg_names, reg_values);
> -    if (ret < 0) {
> -        error_report("Failed to set x64 registers");
> -        return -1;
> -    }
> +    if (cpu->accel->dirty) {
> +        env->eip = rip;
> +        if (access_type == HV_X64_INTERCEPT_ACCESS_TYPE_WRITE) {
> +            env->regs[R_ESI] = info->rsi;
> +        } else {
> +            env->regs[R_EDI] = info->rdi;
> +        }
> +        /* Sync modified standard registers back and clear dirty. */
> +        ret = mshv_store_regs(cpu);
> +        if (ret < 0) {
> +            error_report("Failed to store registers after string PIO");
> +            return -1;
> +        }
> +        cpu->accel->dirty = false;
> +    } else {
> +        reg_names[1] = HV_X64_REGISTER_RIP;
> +        reg_values[1] = rip;
>  
> -    cpu->accel->dirty = false;
> +        ret = set_x64_registers(cpu, reg_names, reg_values);
> +        if (ret < 0) {
> +            error_report("Failed to set x64 registers");
> +            return -1;
> +        }
> +    }
>  
>      return 0;
>  }
> -- 
> 2.53.0

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