[{"id":3687132,"web_url":"http://patchwork.ozlabs.org/comment/3687132/","msgid":"","list_archive_url":null,"date":"2026-05-06T14:38:57","subject":"Re: [PATCH v2 7/7] target/i386/mshv: fix pio handlers clobbering\n device-modified registers","submitter":{"id":90753,"url":"http://patchwork.ozlabs.org/api/people/90753/","name":"Magnus Kulke","email":"magnuskulke@linux.microsoft.com"},"content":"On Tue, May 05, 2026 at 09:50:28PM +0300, Doru Blânzeanu wrote:\n> When a device handler (e.g. vmport) calls cpu_synchronize_state() during\n> I/O port dispatch, it sets cpu->accel->dirty = true and may modify\n> registers directly in env. The old PIO code ignored this: it\n> unconditionally wrote the stale info->rax from the VM-exit intercept\n> message back to the hypervisor and then cleared dirty, discarding any\n> register changes made by the device.\n> \n> Bifurcate both handlers on cpu->accel->dirty:\n> \n> handle_pio_non_str:\n> - dirty path: update env->eip directly. For reads (IN), merge the I/O\n> result into env->regs[R_EAX] (which may have been modified by the\n> device) rather than info->rax. For writes (OUT), leave RAX untouched.\n> Flush all registers via mshv_store_regs() and clear dirty.\n> - non-dirty path: write RIP and RAX via set_x64_registers hypercall as\n> before.\n> \n> handle_pio_str:\n> - dirty path: update env->eip and the appropriate index register\n> (RSI for OUTS, RDI for INS) directly. Flush via mshv_store_regs()\n> and clear dirty.\n> - non-dirty path: write the index register and RIP via\n> set_x64_registers. Drop the RAX assignment that was here before;\n> string I/O does not modify RAX, and set_x64_registers is hardcoded\n> to write only 2 registers so the third slot was silently ignored\n> anyway.\n> \n> Remove the unconditional \"cpu->accel->dirty = false\" at the end of both\n> handlers. In the non-dirty fast path it was redundant (already false).\n> In the dirty path it was actively harmful: it told the vcpu run loop\n> that env was clean when it was not, losing the device's modifications.\n> \n> Signed-off-by: Doru Blânzeanu \n> ---\n> target/i386/mshv/mshv-cpu.c | 82 ++++++++++++++++++++++++++-----------\n> 1 file changed, 59 insertions(+), 23 deletions(-)\n> \n> diff --git a/target/i386/mshv/mshv-cpu.c b/target/i386/mshv/mshv-cpu.c\n> index 0cfac26a5c..7be3fdcc45 100644\n> --- a/target/i386/mshv/mshv-cpu.c\n> +++ b/target/i386/mshv/mshv-cpu.c\n> @@ -1348,7 +1348,7 @@ static int pio_write(uint64_t port, const uint8_t *data, uintptr_t size,\n> return ret;\n> }\n> \n> -static int handle_pio_non_str(const CPUState *cpu,\n> +static int handle_pio_non_str(CPUState *cpu,\n> hv_x64_io_port_intercept_message *info)\n> {\n> size_t len = info->access_info.access_size;\n> @@ -1357,10 +1357,12 @@ static int handle_pio_non_str(const CPUState *cpu,\n> uint32_t val, eax;\n> const uint32_t eax_mask = 0xffffffffu >> (32 - len * 8);\n> size_t insn_len;\n> - uint64_t rip, rax;\n> + uint64_t rip;\n> uint32_t reg_names[2];\n> uint64_t reg_values[2];\n> uint16_t port = info->port_number;\n> + X86CPU *x86_cpu = X86_CPU(cpu);\n> + CPUX86State *env = &x86_cpu->env;\n> \n> if (access_type == HV_X64_INTERCEPT_ACCESS_TYPE_WRITE) {\n> union {\n> @@ -1391,21 +1393,40 @@ static int handle_pio_non_str(const CPUState *cpu,\n> \n> /* Advance RIP and update RAX */\n> rip = info->header.rip + insn_len;\n> - rax = info->rax;\n> \n> - reg_names[0] = HV_X64_REGISTER_RIP;\n> - reg_values[0] = rip;\n> - reg_names[1] = HV_X64_REGISTER_RAX;\n> - reg_values[1] = rax;\n> + if (cpu->accel->dirty) {\n> + env->eip = rip;\n> + if (access_type != HV_X64_INTERCEPT_ACCESS_TYPE_WRITE) {\n> + /*\n> + * For reads, merge the I/O result into the current RAX.\n> + * Use env->regs[R_EAX] as the base since a device handler\n> + * (e.g. vmport) may have called cpu_synchronize_state()\n> + * and modified registers.\n> + */\n> + eax = (((uint32_t)env->regs[R_EAX]) & ~eax_mask)\n> + | (val & eax_mask);\n> + env->regs[R_EAX] = (uint64_t)eax;\n> + }\n> + /* Sync modified standard registers back and clear dirty. */\n> + ret = mshv_store_regs(cpu);\n> + if (ret < 0) {\n> + error_report(\"Failed to store registers after PIO\");\n> + return -1;\n> + }\n> + cpu->accel->dirty = false;\n> + } else {\n> + reg_names[0] = HV_X64_REGISTER_RIP;\n> + reg_values[0] = rip;\n> + reg_names[1] = HV_X64_REGISTER_RAX;\n> + reg_values[1] = info->rax;\n> \n> - ret = set_x64_registers(cpu, reg_names, reg_values);\n> - if (ret < 0) {\n> - error_report(\"Failed to set x64 registers\");\n> - return -1;\n> + ret = set_x64_registers(cpu, reg_names, reg_values);\n> + if (ret < 0) {\n> + error_report(\"Failed to set x64 registers\");\n> + return -1;\n> + }\n> }\n> \n> - cpu->accel->dirty = false;\n> -\n> return 0;\n> }\n> \n> @@ -1521,6 +1542,7 @@ static int handle_pio_str(CPUState *cpu, hv_x64_io_port_intercept_message *info)\n> bool repop = info->access_info.rep_prefix == 1;\n> size_t repeat = repop ? info->rcx : 1;\n> size_t insn_len = info->header.instruction_length;\n> + uint64_t rip;\n> bool direction_flag;\n> uint32_t reg_names[3];\n> uint64_t reg_values[3];\n> @@ -1554,18 +1576,32 @@ static int handle_pio_str(CPUState *cpu, hv_x64_io_port_intercept_message *info)\n> reg_values[0] = info->rdi;\n> }\n> \n> - reg_names[1] = HV_X64_REGISTER_RIP;\n> - reg_values[1] = info->header.rip + insn_len;\n> - reg_names[2] = HV_X64_REGISTER_RAX;\n> - reg_values[2] = info->rax;\n> + rip = info->header.rip + insn_len;\n> \n> - ret = set_x64_registers(cpu, reg_names, reg_values);\n> - if (ret < 0) {\n> - error_report(\"Failed to set x64 registers\");\n> - return -1;\n> - }\n> + if (cpu->accel->dirty) {\n> + env->eip = rip;\n> + if (access_type == HV_X64_INTERCEPT_ACCESS_TYPE_WRITE) {\n> + env->regs[R_ESI] = info->rsi;\n> + } else {\n> + env->regs[R_EDI] = info->rdi;\n> + }\n> + /* Sync modified standard registers back and clear dirty. */\n> + ret = mshv_store_regs(cpu);\n> + if (ret < 0) {\n> + error_report(\"Failed to store registers after string PIO\");\n> + return -1;\n> + }\n> + cpu->accel->dirty = false;\n> + } else {\n> + reg_names[1] = HV_X64_REGISTER_RIP;\n> + reg_values[1] = rip;\n> \n> - cpu->accel->dirty = false;\n> + ret = set_x64_registers(cpu, reg_names, reg_values);\n> + if (ret < 0) {\n> + error_report(\"Failed to set x64 registers\");\n> + return -1;\n> + }\n> + }\n> \n> return 0;\n> }\n> -- \n> 2.53.0\n\nReviewed-by: Magnus Kulke ","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (1024-bit key;\n unprotected) header.d=linux.microsoft.com header.i=@linux.microsoft.com\n header.a=rsa-sha256 header.s=default header.b=GEmI4q6+;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=nongnu.org\n (client-ip=209.51.188.17; helo=lists1p.gnu.org;\n envelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n receiver=patchwork.ozlabs.org)"],"Received":["from lists1p.gnu.org (lists1p.gnu.org [209.51.188.17])\n\t(using TLSv1.2 with cipher ECDHE-ECDSA-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4g9dLn48C8z1yJV\n\tfor ; Thu, 07 May 2026 00:39:27 +1000 (AEST)","from localhost ([::1] helo=lists1p.gnu.org)\n\tby lists1p.gnu.org with esmtp (Exim 4.90_1)\n\t(envelope-from )\n\tid 1wKdP5-00016U-2w; Wed, 06 May 2026 10:39:11 -0400","from eggs.gnu.org ([2001:470:142:3::10])\n by lists1p.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256)\n (Exim 4.90_1) (envelope-from )\n id 1wKdOz-00015O-Sw\n for qemu-devel@nongnu.org; Wed, 06 May 2026 10:39:07 -0400","from linux.microsoft.com ([13.77.154.182])\n by eggs.gnu.org with esmtp (Exim 4.90_1)\n (envelope-from ) id 1wKdOx-0000NH-9p\n for qemu-devel@nongnu.org; Wed, 06 May 2026 10:39:05 -0400","from example.com (unknown [167.220.208.68])\n by linux.microsoft.com (Postfix) with ESMTPSA id E128420B7165;\n Wed, 6 May 2026 07:38:57 -0700 (PDT)"],"DKIM-Filter":"OpenDKIM Filter v2.11.0 linux.microsoft.com E128420B7165","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com;\n s=default; t=1778078339;\n bh=1tPpnBNTN4DbbMHb+HNxfG3F4TosufSfNcZ8AD4u7nY=;\n h=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n b=GEmI4q6+W9V+g0C6Br5smNb9srUJMXtRiBEJARHM/65QAKGDF79Q2XTHU9W9WbkYe\n 4c6+gTp5Xbar5Jn44M2D7BUH0R5koSbxDR6iMrR5DnaQabfms/ZSUQqOmBs9QSc3b0\n xl7BnfMiXFaMUM+9o3LdMiBYs19VCIAhAzSfgSKg=","Date":"Wed, 6 May 2026 16:38:57 +0200","From":"Magnus Kulke ","To":"Doru =?iso-8859-1?q?Bl=E2nzeanu?= ","Cc":"qemu-devel@nongnu.org, Zhao Liu ,\n Wei Liu , Paolo Bonzini ","Subject":"Re: [PATCH v2 7/7] target/i386/mshv: fix pio handlers clobbering\n device-modified registers","Message-ID":"","References":"<20260505185028.237207-1-dblanzeanu@linux.microsoft.com>\n <20260505185028.237207-8-dblanzeanu@linux.microsoft.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20260505185028.237207-8-dblanzeanu@linux.microsoft.com>","Received-SPF":"pass client-ip=13.77.154.182;\n envelope-from=magnuskulke@linux.microsoft.com; helo=linux.microsoft.com","X-Spam_score_int":"-19","X-Spam_score":"-2.0","X-Spam_bar":"--","X-Spam_report":"(-2.0 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1,\n DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, RCVD_IN_DNSWL_NONE=-0.0001,\n SPF_HELO_PASS=-0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no","X-Spam_action":"no action","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"qemu development ","List-Unsubscribe":",\n ","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org"}}]