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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 008F6C10F15 for ; Thu, 25 Apr 2024 20:44:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:Cc:To:From:Subject:Message-ID:Mime-Version: In-Reply-To:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:References: List-Owner; bh=NLsU9OxSnAJ2SQMJclk+FVmrbYmI/IrJI/92ArfhBjo=; b=FGWw/BLrLm6BBM Sp5aeEOGTEyfL49VTZv0s6n+kPiZksi4fXTxrTj1Zie5l/H0l0g9qocMZZQ5226YnEhMgDa12+non 1iAQcg3a9xULHZJhGzeFdGwJ5wyztF7EDOMo6LVEV3d+DopvATlmzPCvomiN2EswyU+qi/qkMJ8+x S1VNumFot29XZBt/wGBq345e6aD/D3VcVRvQc+U0OnnM0+hMEvLn5TMOjbfTUXAMxC6p4Ov/ZvZxK uxvPWNzDbdsaIrG2KT0b+S1FbyudaFV7xrGkNUrsh8AyPdzUov0CFMSbuFlhYHPZRUgga45h8VGmo oonAvIre6F6eVVxe8y6A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1s05xU-0000000A8LR-3loy; Thu, 25 Apr 2024 20:44:44 +0000 Received: from mail-yb1-xb49.google.com ([2607:f8b0:4864:20::b49]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1s05xS-0000000A8K1-0vSu for linux-arm-kernel@lists.infradead.org; Thu, 25 Apr 2024 20:44:43 +0000 Received: by mail-yb1-xb49.google.com with SMTP id 3f1490d57ef6-de59e612376so1094829276.3 for ; Thu, 25 Apr 2024 13:44:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1714077879; x=1714682679; darn=lists.infradead.org; h=cc:to:from:subject:message-id:mime-version:in-reply-to:date:from:to :cc:subject:date:message-id:reply-to; bh=7EPo+zEkHEFx1G3/t3IphIU996vlKHE8W7j3UdO+nD0=; b=1BctuhH9c+Kl3T9zdXKtiIlyKqDSyqe0nYUGNei4SDoVxRHCa4eBz9waGzzqNT6Bx9 7cjkzncf6+1PyInajLyg2rSZ/01HNeTZ5gi19Bk38IvADeOdj20+KfSDeR22In/cEPWa M+1Ia/iVkFFRJxgy3Acm+dDSnweW0OysNQ703ZPu9/Oc/u3srUpJ/ZAwTyRJSveqRybL 4gkP/nwhqzv0DJsct5Jcl0pPdNSsZnAlFfH+31goh+G+sjVqJmuA6QzU800qzG385dCM kUNiUowF3MxwSCA/8k1O+smvz3IPzrmG2Ikofj4qYjTP0O2IKRZYuX0xnyfJwUMALnP0 r4nA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714077879; x=1714682679; h=cc:to:from:subject:message-id:mime-version:in-reply-to:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=7EPo+zEkHEFx1G3/t3IphIU996vlKHE8W7j3UdO+nD0=; b=rBxUyMrRAj/C6HGTAAmx3JeP72QbF7i56tfl8I74I0iY0vAkRcAobXAZHeJb0CbH5C NxtJzVkU5qyccF18GtgbBzGiHsxdDe5irpEVnI3v/r4vyAekN38zfo7ClBo76RWxhLP/ 20WslOMo56Dz/Ga2I3kjT/khWmYRGAbuntITHlA1D6RQqNK30D/xkmUO9R9TGwsoY5eb JryqySH+Ny1GuQ3aHNkyYeWtnJwrVKYV3IKVHU1Zz2wuv2dki/CTVpthvtbRzfvv6wry JQ2HDhUGrKOSEeRemb8LotNGocO8St2qRqcfP1oUFmUay/78KCnBn5Y1ScuEuau5rrNZ i2fA== X-Forwarded-Encrypted: i=1; AJvYcCWnUtkpqO/s8DtEVE1qXys0Ij0s0tDiPOQ/lcfl3vw5eR9Vhztq6dqx6LEet3jtqUHbpPzBS4JnzZbFNQJcUqPfess2eAj0MwR7BgaGRWKhMO0EQvI= X-Gm-Message-State: AOJu0Yycbi2TE+vX4lXV2qLaIPyxj/GhSY7YEzJr4C8xSGL3+jj1M6yX BpOji3HoOgSNi26K+HXICS7FyyoOQFjAfd7tSoCRB+RnAhGH7FxfchlGU9vG+tyoJ5bXsrONxhN lfVKrtUHUz5oGpw5A7Is2Xg== X-Google-Smtp-Source: AGHT+IGAB6QOB7oBGzpObLQHA1ClHYJKANOvXwLDW//pW389wqrc02vbLvB6xtOYSun/X1gLD+tgNqvwAvZ/M4BCYw== X-Received: from coltonlewis-kvm.c.googlers.com ([fda3:e722:ac3:cc00:2b:ff92:c0a8:14ce]) (user=coltonlewis job=sendgmr) by 2002:a05:6902:10c3:b0:de5:c2b:389b with SMTP id w3-20020a05690210c300b00de50c2b389bmr100094ybu.5.1714077878658; Thu, 25 Apr 2024 13:44:38 -0700 (PDT) Date: Thu, 25 Apr 2024 20:44:37 +0000 In-Reply-To: (message from Oliver Upton on Mon, 22 Apr 2024 14:43:32 -0700) Mime-Version: 1.0 Message-ID: Subject: Re: [PATCH v4] KVM: arm64: Add early_param to control WFx trapping From: Colton Lewis To: Oliver Upton Cc: kvm@vger.kernel.org, corbet@lwn.net, maz@kernel.org, james.morse@arm.com, suzuki.poulose@arm.com, yuzenghui@huawei.com, catalin.marinas@arm.com, will@kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240425_134442_287397_136CF7EF X-CRM114-Status: GOOD ( 24.53 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed"; DelSp="yes" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Oliver Upton writes: > Hi Colton, > On Mon, Apr 22, 2024 at 06:17:16PM +0000, Colton Lewis wrote: >> @@ -2653,6 +2653,27 @@ >> [KVM,ARM] Allow use of GICv4 for direct injection of >> LPIs. >> + kvm-arm.wfe_trap_policy= >> + [KVM,ARM] Control when to set wfe instruction trap. > nitpick: when referring to the instruction, please capitalize it. > Also, it doesn't hurt to be verbose here and say this cmdline option > "Controls the WFE instruction trap behavior for KVM VMs" > I say this because there is a separate set of trap controls that allow > WFE or WFI to execute in EL0 (i.e. host userspace). Will do. >> + trap: set wfe instruction trap >> + >> + notrap: clear wfe instruction trap >> + >> + default: set wfe instruction trap only if multiple >> + tasks are running on the CPU > I would strongly prefer we not make any default behavior user-visible. > The default KVM behavior can (and will) change in the future. > Only the absence of an explicit trap / notrap policy should fall back to > KVM's default heuristics. Makes sense to me. Will do. >> -static inline void vcpu_clear_wfx_traps(struct kvm_vcpu *vcpu) >> +static inline void vcpu_clear_wfe_trap(struct kvm_vcpu *vcpu) >> { >> vcpu->arch.hcr_el2 &= ~HCR_TWE; >> +} >> + >> +static inline void vcpu_clear_wfi_trap(struct kvm_vcpu *vcpu) >> +{ >> if (atomic_read(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe.vlpi_count) || >> vcpu->kvm->arch.vgic.nassgireq) >> vcpu->arch.hcr_el2 &= ~HCR_TWI; >> @@ -119,12 +123,28 @@ static inline void vcpu_clear_wfx_traps(struct >> kvm_vcpu *vcpu) >> vcpu->arch.hcr_el2 |= HCR_TWI; >> } > This helper definitely does not do as it says on the tin. It ignores the > policy requested on the command line and conditionally *sets* TWI. If > the operator believes they know best and ask for a particular trap policy > KVM should uphold it unconditionally. Even if they've managed to shoot > themselves in the foot. Will do. I was only splitting up what the existing helper did here. >> @@ -423,6 +425,12 @@ void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) >> } >> +static bool kvm_should_clear_wfx_trap(enum kvm_wfx_trap_policy p) >> +{ >> + return (p == KVM_WFX_NOTRAP && kvm_vgic_global_state.has_gicv4) >> + || (p == KVM_WFX_NOTRAP_SINGLE_TASK && single_task_running()); >> +} > style nitpick: operators should always go on the preceding line for a > multi-line statement. Will do. >> void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) >> { >> struct kvm_s2_mmu *mmu; >> @@ -456,10 +464,15 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int >> cpu) >> if (kvm_arm_is_pvtime_enabled(&vcpu->arch)) >> kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu); >> - if (single_task_running()) >> - vcpu_clear_wfx_traps(vcpu); >> + if (kvm_should_clear_wfx_trap(kvm_wfi_trap_policy)) >> + vcpu_clear_wfi_trap(vcpu); >> else >> - vcpu_set_wfx_traps(vcpu); >> + vcpu_set_wfi_trap(vcpu); >> + >> + if (kvm_should_clear_wfx_trap(kvm_wfe_trap_policy)) >> + vcpu_clear_wfe_trap(vcpu); >> + else >> + vcpu_set_wfe_trap(vcpu); >> if (vcpu_has_ptrauth(vcpu)) >> vcpu_ptrauth_disable(vcpu); > I find all of the layering rather hard to follow; we don't need > accessors for doing simple bit manipulation. > Rough sketch: > static bool kvm_vcpu_should_clear_twi(struct kvm_vcpu *vcpu) > { > if (unlikely(kvm_wfi_trap != KVM_WFX_DEFAULT)) > return kvm_wfi_trap == KVM_WFX_NOTRAP; > return single_task_running() && > (atomic_read(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe.vlpi_count) || > vcpu->kvm->arch.vgic.nassgireq); > } > static bool kvm_vcpu_should_clear_twe(struct kvm_vcpu *vcpu) > { > if (unlikely(kvm_wfe_trap != KVM_WFX_DEFAULT)) > return kvm_wfe_trap == KVM_WFX_NOTRAP; > return single_task_running(); > } > static void kvm_vcpu_load_compute_hcr(struct kvm_vcpu *vcpu) > { > vcpu->arch.hcr_el2 |= HCR_TWE | HCR_TWI; > if (kvm_vcpu_should_clear_twe(vcpu)) > vcpu->arch.hcr_el2 &= ~HCR_TWE; > if (kvm_vcpu_should_clear_twi(vcpu)) > vcpu->arch.hcr_el2 &= ~HCR_TWI; > } Will do. > And if we really wanted to, the non-default trap configuration could be > moved to vcpu_reset_hcr() if we cared. Might as well. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel