From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 61E94C021A4 for ; Mon, 24 Feb 2025 12:00:43 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1tmX8T-0004Rh-0Z; Mon, 24 Feb 2025 07:00:33 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1tmX8I-0004PZ-2J for qemu-riscv@nongnu.org; Mon, 24 Feb 2025 07:00:23 -0500 Received: from mail-pl1-x634.google.com ([2607:f8b0:4864:20::634]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1tmX8F-0001s9-Ti for qemu-riscv@nongnu.org; Mon, 24 Feb 2025 07:00:21 -0500 Received: by mail-pl1-x634.google.com with SMTP id d9443c01a7336-220c665ef4cso71051575ad.3 for ; Mon, 24 Feb 2025 04:00:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; t=1740398418; x=1741003218; darn=nongnu.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=XlGtojGmpBGD+z3gNmEudQwL/D4vs6ZbvrYTcUQHSRg=; b=Vd/YTvbgFMBcv/iaASu2uQ8miAF2Tivu4afRj1RD5ZDMLNEvG3UyMnhzpJm+SfHnz1 /+M3jwd5IbpeLJcWuA5AUYHcVYtIh+7BHuy4sZ62gqNkcesBrwDT8lJy+VV+sGLP5rX5 v0Yw+skJSO/+zklVYiBSCldLlfIeFGm3vTE2lWj+WKnn7XsxwUbxJrY1PBU11myOUFvB 2jkpGvPs0wa1kxBj40/y07zZwKeTWizZZn5AsnIQbJ6XT0HXxzvsyc0KFuOOpfn/pGqb Zqi0QdS+714BdJgDuUKvI8WYrQPc6njONxbUljKasSQrRTjwCFPWIjcbSt7zK3jv3IjW GnaQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1740398418; x=1741003218; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=XlGtojGmpBGD+z3gNmEudQwL/D4vs6ZbvrYTcUQHSRg=; b=TeSv0zPPEcz8p5Z/09dfyH4h7HzRmnkzVhkUcSO4a++6taoueF+mX1zN9MCGd3qYkw HFR43g2x1uPhwK5LCJpyUb0niylcjw6DVFoGZwI4wtiD/vXB4MFNsNY1CHl778lapztu YzqBpUnDJ8Du56A4XiC+E99hRzQT+ZYMK34QxVBsBSjfykftpg5u2/dZ+rubBu1wwlGG ZxUVceq9a7DtonZ0rj8zwT1a9fJIT96aahnHCM4sQ2gvhUqlR/VRdBlYeN3S6NFRyCut tfM4t1bT3PXDIrn0BBvFmWo9MNDpvS4vImkztUp3VLByE0INjfStY3Qfps4Deq3wgggi SuRQ== X-Forwarded-Encrypted: i=1; AJvYcCUzJL03hzcqEvFuK05LmSBNl6k08MlAukzer0x1JlTwtwm/n7ni3i8j2mVdR8l5+puF0cHhObowfOLx@nongnu.org X-Gm-Message-State: AOJu0YzRxQORL/4aasatFAYbIWiKRUZXizrzFxpseJou+gYYeBQ3qq2n u/N9Egl+fbG+MfeSHcGT29yfO2S7igyRXZ+RXDVd+pUp3Mr5WAkfS4nRROm6ARI= X-Gm-Gg: ASbGncsQR8eYKFyJhnvCqlI57xtJ5dtCF/CfPkXEyvPJAofqF5CJsklt+H06a6qY77j cCtr1v1aAnXoVfLnMEo9phnNnpuqHdX1vWKgXfXPRm52xZfBBVo6IMUy4nJDU8uRtzmuMptZKlz vIHOwpJFSw6kNySYiDGb6P6lyNk/MN+pCWmwmThZGpsdb1+s6hdcWMsHuGfYRWbcM360Cn7YrLo xwjjcuFVxXMrr2zdKp+A7/F6HLfx2SacnQZEL0G2ZGAGUpr0lk22jXpJeifTjAPFI7UM1KSd40t 7yRVSB0HGTPvdX0s2rxGgy76jrSdKSaVI7GgruESiw== X-Google-Smtp-Source: AGHT+IEd0Uxsq1E6ktxKQ922eYvc7ZhzCHV4otBIQOsbCkUrDr4erbXc2VauwjlaJj8e5KfGLpStXg== X-Received: by 2002:a17:903:983:b0:21f:14c1:d58e with SMTP id d9443c01a7336-2219ff8279dmr228639555ad.1.1740398418264; Mon, 24 Feb 2025 04:00:18 -0800 (PST) Received: from [192.168.68.110] ([177.170.227.219]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-220d558ee0bsm178786515ad.236.2025.02.24.04.00.15 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 24 Feb 2025 04:00:17 -0800 (PST) Message-ID: <2a7ccc9e-a577-4b66-9f37-ade4bf3d768a@ventanamicro.com> Date: Mon, 24 Feb 2025 09:00:14 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 1/3] target/riscv/cpu: ignore TCG init for KVM CPUs in reset_hold To: Peter Maydell Cc: qemu-devel@nongnu.org, qemu-riscv@nongnu.org, alistair.francis@wdc.com, bmeng@tinylab.org, liwei1518@gmail.com, zhiwei_liu@linux.alibaba.com, palmer@rivosinc.com References: <20250220161313.127376-1-dbarboza@ventanamicro.com> <20250220161313.127376-2-dbarboza@ventanamicro.com> Content-Language: en-US From: Daniel Henrique Barboza In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Received-SPF: pass client-ip=2607:f8b0:4864:20::634; envelope-from=dbarboza@ventanamicro.com; helo=mail-pl1-x634.google.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-riscv@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-riscv-bounces+qemu-riscv=archiver.kernel.org@nongnu.org Sender: qemu-riscv-bounces+qemu-riscv=archiver.kernel.org@nongnu.org On 2/24/25 8:47 AM, Peter Maydell wrote: > On Mon, 24 Feb 2025 at 11:29, Daniel Henrique Barboza > wrote: >> >> >> >> On 2/24/25 6:59 AM, Peter Maydell wrote: >>> On Thu, 20 Feb 2025 at 16:14, Daniel Henrique Barboza >>> wrote: >>>> >>>> riscv_cpu_reset_hold() does a lot of TCG-related initializations that >>>> aren't relevant for KVM, but nevertheless are impacting the reset state >>>> of KVM vcpus. >>>> >>>> When running a KVM guest, kvm_riscv_reset_vcpu() is called at the end of >>>> reset_hold(). At that point env->mstatus is initialized to a non-zero >>>> value, and it will be use to write 'sstatus' in the vcpu >>>> (kvm_arch_put_registers() then kvm_riscv_put_regs_csr()). >>>> >>>> Do an early exit in riscv_cpu_reset_hold() if we're running KVM. All the >>>> KVM reset procedure will be centered in kvm_riscv_reset_vcpu(). >>>> >>>> While we're at it, remove the kvm_enabled() check in >>>> kvm_riscv_reset_vcpu() since it's already being gated in >>>> riscv_cpu_reset_hold(). > >>> This looks super odd, from an "I don't know anything about >>> riscv specifics" position. Generally the idea is: >>> * reset in QEMU should reset the CPU state >>> * what a reset CPU looks like doesn't differ between >>> accelerators >>> * when we start the KVM CPU, we copy the state from QEMU >>> to the kernel, and then the kernel's idea of the reset state >>> matches >>> >>> This patch looks like it's entirely skipping basically >>> all of the QEMU CPU state reset code specifically for KVM. >> >> Not sure I understood what you said here. >> >> Without this patch, riscv_cpu_reset_hold() is doing initializations that are TCG >> based, both for user mode and system mode, and in the end is calling the kvm >> specific reset function if we're running KVM. This patch is simply skipping >> all the TCG related reset procedures if we're running KVM. So the patch isn't >> skipping the KVM specific QEMU CPU reset code, it is skipping the TCG specific >> reset code if we're running KVM. > > What I'm saying is that you shouldn't have "TCG reset" and > "KVM reset" that are totally different code paths, but that the > reset function should be doing "reset the CPU", and then the > KVM codepath makes specific decisions about "for KVM these > particular things should be the kernel's reset settings" and > then passes that state over to the kernel. > >> Granted, after applying patches 2 and 3 we could discard this patch because >> now we're resetting all that KVM needs in kvm_reset_vcpu(), but why go >> through the reset steps for TCG if we're going to overwrite them later during >> kvm_reset_vcpu()? > > The idea is that you only overwrite specific state where > you've decided "actually the kernel is the authoritative > source for what the reset state for these registers is". > >>> So now you'll have two different pieces of code controlling >>> reset for different accelerators, and the resulting CPU >>> state won't be consistent between them... >> >> That is already the case even without this patch, doesn't it? If we have to call >> a specific kvm reset function during cpu reset_hold then we're already in a point >> where the reset procedure is differing between accelerators. I won't say that >> this is a good design but I don't see it as a problem. > >> For instance, going to a code you're probably more familiar with, target/arm/cpu.c, >> arm_cpu_reset_hold(), is doing something similar to what riscv_cpu_reset_hold() is >> doing without this patch: a lot of TCG setups are made, then kvm_arm_reset_vcpu() is >> called in the end if kvm_enabled(). kvm_arm_reset_vcpu() then overwrites at least some >> of the TCG specific setup that was done before: >> >> /* Re-init VCPU so that all registers are set to >> * their respective reset values. >> */ >> ret = kvm_arm_vcpu_init(cpu); >> >> kvm_arm_vcpu_init() is doing a KVM_ARM_VCPU_INIT ioctl that will populate the CPU object >> with the kernel specific feature bitmask and so on. Note that this is not copying the TCG >> setup into the kernel, it is in fact doing the opposite. > > The way this code path in Arm works is: > * we reset all the QEMU-side CPU state struct in arm_cpu_reset_hold() > * kvm_arm_reset_vcpu() calls kvm_arm_vcpu_init() which inits > the KVM side vcpu > * kvm_arm_reset_vcpu() calls the sequence write_kvmstate_to_list() > followed by write_list_to_cpustate() which does "for the system > registers specifically, read the values (and which registers we > have) from KVM, and write them to the QEMU CPU state struct". > We do this because on Arm we've said that the system registers > in particular have the kernel as their authoritative source. > (IIRC x86 makes QEMU the authority for its similar registers, > so it has to do even less in its kvm_arch_reset_vcpu().) > * the generic CPU core code will call kvm_arch_put_registers() > before starting the vCPU, which copies whatever is in the QEMU-side > CPU state struct into KVM > > The upshot is that QEMU is the authority and arm_cpu_reset_hold() > defines the reset value for all CPU state by default. (For instance, > reset values for general purpose registers are set there.) But for > specific parts of the state where we want KVM to be the authority, > kvm_arm_reset_vcpu() gets to override that by filling in the CPU > state struct by asking the kernel what its values are. > >> Note that my intention here isn't to make a case that the ARM KVM cpu doesn't need anything >> that is being done in arm_cpu_reset_hold(). My point here is that KVM and TCG CPUs will have >> different reset setups for some archs. For RISC-V I can say that KVM CPU does not rely on >> anything that the TCG reset code is doing, hence why I sent this patch to make it official. > > What I'm trying to suggest here is that we don't want different > architectures to end up doing things differently. There are > multiple different design patterns that will work, but it will > be easier to work on QEMU if we can standardise on doing it > the same way across architectures. I can get behind this argument. For this patch I'll just remove the redundant !kvm_enabled() check in kvm_riscv_reset_vcpu(). Let's keep the kvm_riscv_reset_vcpu() call at the end of riscv_cpu_reset_hold() to keep the design where we have a single reset procedure, overwriting the needed state where we want KVM to be the authority instead of QEMU. Thanks, Daniel > > thanks > -- PMM