Message ID | 20221114181632.3279119-1-seanjc@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp2292870wru; Mon, 14 Nov 2022 10:21:03 -0800 (PST) X-Google-Smtp-Source: AA0mqf4KFSgIWbdXAb4idwIrcLCJph0yxeSFCCsRYETm3A2rE/M+IBtCasUAceqtSbjAlG6rhV0s X-Received: by 2002:a17:90a:4e8a:b0:20a:75ae:8276 with SMTP id o10-20020a17090a4e8a00b0020a75ae8276mr15011529pjh.51.1668450063048; Mon, 14 Nov 2022 10:21:03 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1668450063; cv=none; d=google.com; s=arc-20160816; b=a2e3d0SMWcOCVdjoq5DuqkYotDXnPuPHF3jJSTgeRSSfcctWk3b6U2sYpj9dvmnrv+ dLfPxHbNwoQHyQQ3qQ8x3FAtac0vkjStztG3esXICMoPIgQZ1t34xMlRq/HHUbEw32oC C1ToPHxCjPck2S0YUKfj4ud7CjF9ErkzwB3IMnVfUWCwAJMsbu/S1soACyCD7J0xlN5r f+LF5kisNo9aW1c8rClZZfcggf6kHAjWzDQMvEJByf/eJVKx6sAzHlTa1up9KWlAyBP/ JLX3h9+cyZiDIvQi4rppkPoIIgEwhb17PH49OD45271D28fDIlWZSIrELu7uTpbESAS7 vcCg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:from:subject:message-id:mime-version:date :reply-to:dkim-signature; bh=WtE9LTLM6e/iDSPGZbavzj1dYFHgCb3oR3rJeTcOT+g=; b=WT1zGjSUfpsNJtb+SM3jZlD2ehynICChSx2lN4nvnOgOTS8m2xlpNzRZajIFlt9b04 rEPdEBda5WHa1uvDaEIIS5cbwfcO8yI0IM9ugS4AxhjdL3oulqeNklvD2tY6DdBZvaUj SljTP5guMqvlnGjfaWxk0zA6+Hgc/7uVf6WNPCKgjU14geBB6O+vAyGZBiCbpRQCoNNd 2HeoxXW5XMrSGAc9qnjklpvQwDQxYrfvqKIxWuc7zZSClfajX9RO8d3JLkMBUaLZAgdL L32lHbcmmaECZM2O6kFaodK89ucYJhIH8gjBeamFFJodK3pojmzQy2Lr4d+NRY6B3cnz HWqw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b=C+aQg2oY; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id 22-20020a17090a019600b00200e221e9a0si14309839pjc.149.2022.11.14.10.20.48; Mon, 14 Nov 2022 10:21:03 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b=C+aQg2oY; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236162AbiKNSTG (ORCPT <rfc822;zwp10758@gmail.com> + 99 others); Mon, 14 Nov 2022 13:19:06 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42604 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237507AbiKNSR5 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 14 Nov 2022 13:17:57 -0500 Received: from mail-pl1-x649.google.com (mail-pl1-x649.google.com [IPv6:2607:f8b0:4864:20::649]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9AB912936F for <linux-kernel@vger.kernel.org>; Mon, 14 Nov 2022 10:16:37 -0800 (PST) Received: by mail-pl1-x649.google.com with SMTP id c1-20020a170902d48100b0018723580343so9492998plg.15 for <linux-kernel@vger.kernel.org>; Mon, 14 Nov 2022 10:16:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:from:subject:message-id:mime-version:date:reply-to:from:to:cc :subject:date:message-id:reply-to; bh=WtE9LTLM6e/iDSPGZbavzj1dYFHgCb3oR3rJeTcOT+g=; b=C+aQg2oYQLkz9ZQE61nnXuMx0navRh+/cnw0YCS2ZX/meNvlvH6Be1MmSOBSShGO85 gmf+WlWAD7lCRWPz7fRII0HQ5gGlyIm+1y3iyiOKGWI1KBcWsNwCHoZQ3fFOmWreHM/O ahW3BEfVmV3/6/y8l5az53O3+CQT0ZfQtHwMZokgXzSutyUL4BUICwssgac2hSP8/njb PDCemJQMOteai8s6cpcR/oaQDmVlz2CaI8aai313l0sCk/ITNGZOA9snssz3mWulq35Q j8k3KcAKSto81EUZLhZs8UwQAD8wQSHVbBCzzDPKAiXjWhHcLkmVlG8JW680yuGNpVaq qI1A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:from:subject:message-id:mime-version:date:reply-to :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=WtE9LTLM6e/iDSPGZbavzj1dYFHgCb3oR3rJeTcOT+g=; b=OrpEF2YqCUpfoitUp2Cm8jxiqGusY2X+VpD5kIpNgUYeInzVbL0CisTtkmBQcISmJx QcSonRW1A+gk3AjeoRYzS/aNtbEQIRryZeEb5jc74/7bsWcXd2MYPPNcx2529TE9ZOB4 xKq1HgqTQAgjUAXYvb3m/2MutR0DU3iWlu4fVmAmouHwM9pGtVwQMCj4+XocDWn46uud e9IR1sPqvmpRdsGUyidsdord46jCoJHl8d577qKrhrAYSGS2ERCS8xuDcaSfl/XZqbcE NlD7hgSSdWUhC528zqdaHrs2zotXUTUk4O42i8f5/VqKx79vmXc//WoZOC4HrOd9IRx1 BtQg== X-Gm-Message-State: ANoB5pkj9u1SdG8+M6ZvsRIcQ85iwNMLts3sBml9IvX1c52fceXCMIjG hTQ3XmT/WbqzkhTPdk7Xfls7Mpmcvro= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:a17:90a:1b23:b0:218:725:c820 with SMTP id q32-20020a17090a1b2300b002180725c820mr14862954pjq.170.1668449797098; Mon, 14 Nov 2022 10:16:37 -0800 (PST) Reply-To: Sean Christopherson <seanjc@google.com> Date: Mon, 14 Nov 2022 18:16:32 +0000 Mime-Version: 1.0 X-Mailer: git-send-email 2.38.1.431.g37b22c650d-goog Message-ID: <20221114181632.3279119-1-seanjc@google.com> Subject: [PATCH] KVM: x86/xen: Make number of event channels defines less magical From: Sean Christopherson <seanjc@google.com> To: Sean Christopherson <seanjc@google.com>, Paolo Bonzini <pbonzini@redhat.com> Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, David Woodhouse <dwmw2@infradead.org>, Paul Durrant <paul@xen.org> Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1749496693455748049?= X-GMAIL-MSGID: =?utf-8?q?1749496693455748049?= |
Series |
KVM: x86/xen: Make number of event channels defines less magical
|
|
Commit Message
Sean Christopherson
Nov. 14, 2022, 6:16 p.m. UTC
Use BITS_PER_BYTE and sizeof_field() to compute the number of Xen event
channels. The compat version at least uses sizeof_field(), but the
regular version open codes sizeof_field(), BITS_PER_BYTE, and combines
literals in the process, which makes it far too difficult to understand
relatively straightforward code.
No functional change intended.
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Paul Durrant <paul@xen.org>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/xen.h | 8 +++++---
include/xen/interface/event_channel.h | 6 ++++--
2 files changed, 9 insertions(+), 5 deletions(-)
base-commit: d663b8a285986072428a6a145e5994bc275df994
Comments
On Mon, 2022-11-14 at 18:16 +0000, Sean Christopherson wrote: > Use BITS_PER_BYTE and sizeof_field() to compute the number of Xen event > channels. The compat version at least uses sizeof_field(), but the > regular version open codes sizeof_field(), BITS_PER_BYTE, and combines > literals in the process, which makes it far too difficult to understand > relatively straightforward code. > > No functional change intended. Slightly dubious about changing the regular one, since that's just imported directly from Xen public header files.
On Mon, Nov 14, 2022, David Woodhouse wrote: > On Mon, 2022-11-14 at 18:16 +0000, Sean Christopherson wrote: > > Use BITS_PER_BYTE and sizeof_field() to compute the number of Xen event > > channels. The compat version at least uses sizeof_field(), but the > > regular version open codes sizeof_field(), BITS_PER_BYTE, and combines > > literals in the process, which makes it far too difficult to understand > > relatively straightforward code. > > > > No functional change intended. > > Slightly dubious about changing the regular one, since that's just > imported directly from Xen public header files. Ugh. I worried that might be the case. An alternative approach to help document things from a KVM perspective would be something like: diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c index 93c628d3e3a9..7769f3b98af0 100644 --- a/arch/x86/kvm/xen.c +++ b/arch/x86/kvm/xen.c @@ -1300,6 +1300,9 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu) static inline int max_evtchn_port(struct kvm *kvm) { + BUILD_BUG_ON(EVTCHN_2L_NR_CHANNELS != + (sizeof_field(struct shared_info, evtchn_pending) * BITS_PER_BYTE)); + if (IS_ENABLED(CONFIG_64BIT) && kvm->arch.xen.long_mode) return EVTCHN_2L_NR_CHANNELS; else
On Mon, 2022-11-14 at 19:39 +0000, Sean Christopherson wrote: > Ugh. I worried that might be the case. An alternative approach to help document > things from a KVM perspective would be something like: > > diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c > index 93c628d3e3a9..7769f3b98af0 100644 > --- a/arch/x86/kvm/xen.c > +++ b/arch/x86/kvm/xen.c > @@ -1300,6 +1300,9 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu) > > static inline int max_evtchn_port(struct kvm *kvm) > { > + BUILD_BUG_ON(EVTCHN_2L_NR_CHANNELS != > + (sizeof_field(struct shared_info, evtchn_pending) * BITS_PER_BYTE)); > + > if (IS_ENABLED(CONFIG_64BIT) && kvm->arch.xen.long_mode) > return EVTCHN_2L_NR_CHANNELS; > else Not really sure I see the point of that. There are two main reasons for that kind of BUILD_BUG_ON(). I've added a few of them asserting that the size of the structure and its compat variant are identical, and thus documenting *why* the code lacks compat handling. For example... /* * Next, write the new runstate. This is in the *same* place * for 32-bit and 64-bit guests, asserted here for paranoia. */ BUILD_BUG_ON(offsetof(struct vcpu_runstate_info, state) != offsetof(struct compat_vcpu_runstate_info, state)); The second reason is to prevent accidental screwups where our local definition of a structure varies from the official ABI. Like these: /* Paranoia checks on the 32-bit struct layout */ BUILD_BUG_ON(offsetof(struct compat_shared_info, wc) != 0x900); BUILD_BUG_ON(offsetof(struct compat_shared_info, arch.wc_sec_hi) != 0x924); BUILD_BUG_ON(offsetof(struct pvclock_vcpu_time_info, version) != 0); I don't really see the above fulfilling either of those use cases. Given that the definition of the evtchn_pending field is: xen_ulong_t evtchn_pending[sizeof(xen_ulong_t) * 8]; It's fairly tautological that the number of event channels supported is BITS_PER_ULONG * BITS_PER_ULONG. Which is sizeof(xen_ulong_t)² * 64 as defined in the official Xen headers. I don't know that we really need to add our own sanity check on the headers we imported from Xen. It doesn't seem to add much. No objection to cleaning up the COMPAT one though, as in your original patch.
EVTCHN_2L_NR_CHANNELSOn Mon, Nov 14, 2022, David Woodhouse wrote: > On Mon, 2022-11-14 at 19:39 +0000, Sean Christopherson wrote: > > Ugh. I worried that might be the case. An alternative approach to help document > > things from a KVM perspective would be something like: > > > > diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c > > index 93c628d3e3a9..7769f3b98af0 100644 > > --- a/arch/x86/kvm/xen.c > > +++ b/arch/x86/kvm/xen.c > > @@ -1300,6 +1300,9 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu) > > > > static inline int max_evtchn_port(struct kvm *kvm) > > { > > + BUILD_BUG_ON(EVTCHN_2L_NR_CHANNELS != > > + (sizeof_field(struct shared_info, evtchn_pending) * BITS_PER_BYTE)); > > + > > if (IS_ENABLED(CONFIG_64BIT) && kvm->arch.xen.long_mode) > > return EVTCHN_2L_NR_CHANNELS; > > else > > Not really sure I see the point of that. > > There are two main reasons for that kind of BUILD_BUG_ON(). I've added > a few of them asserting that the size of the structure and its compat > variant are identical, and thus documenting *why* the code lacks compat > handling. For example... > > /* > * Next, write the new runstate. This is in the *same* place > * for 32-bit and 64-bit guests, asserted here for paranoia. > */ > BUILD_BUG_ON(offsetof(struct vcpu_runstate_info, state) != > offsetof(struct compat_vcpu_runstate_info, state)); > > The second reason is to prevent accidental screwups where our local > definition of a structure varies from the official ABI. Like these: > > /* Paranoia checks on the 32-bit struct layout */ > BUILD_BUG_ON(offsetof(struct compat_shared_info, wc) != 0x900); > BUILD_BUG_ON(offsetof(struct compat_shared_info, arch.wc_sec_hi) != 0x924); > BUILD_BUG_ON(offsetof(struct pvclock_vcpu_time_info, version) != 0); > > I don't really see the above fulfilling either of those use cases. > > Given that the definition of the evtchn_pending field is: > > xen_ulong_t evtchn_pending[sizeof(xen_ulong_t) * 8]; > > It's fairly tautological that the number of event channels supported is > BITS_PER_ULONG * BITS_PER_ULONG. Which is sizeof(xen_ulong_t)² * 64 as > defined in the official Xen headers. > > I don't know that we really need to add our own sanity check on the > headers we imported from Xen. It doesn't seem to add much. The goal isn't to add a sanity check, it's to document what EVTCHN_2L_NR_CHANNELS actually represents. My frustration with sizeof(xen_ulong_t) * sizeof(xen_ulong_t) * 64 is that there's nothing there that connects it back to evtchn_pending or evtchn_mask. E.g. ideally the code would be something like #define COMPAT_EVTCHN_2L_NR_CHANNELS 256 #ifdef CONFIG_X86_64 #define EVTCHN_2L_NR_CHANNELS 512 #else #define EVTCHN_2L_NR_CHANNELS COMPAT_EVTCHN_2L_NR_CHANNELS DECLARE_BITMAP(evtchn_pending, EVTCHN_2L_NR_CHANNELS); DECLARE_BITMAP(evtchn_mask, EVTCHN_2L_NR_CHANNELS); which is much more self-documenting and doesn't require the reader to do math to grok the basics. Anyways, we can drop this patch, it was written mostly out of frustration with how long it took me to understand what is actually a very simple concept that's written in an unnecessarily obscure way.
On Tue, 2022-11-22 at 20:31 +0000, Sean Christopherson wrote: > EVTCHN_2L_NR_CHANNELSOn Mon, Nov 14, 2022, David Woodhouse wrote: > > On Mon, 2022-11-14 at 19:39 +0000, Sean Christopherson wrote: > > > Ugh. I worried that might be the case. An alternative approach to help document > > > things from a KVM perspective would be something like: > > > > > > diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c > > > index 93c628d3e3a9..7769f3b98af0 100644 > > > --- a/arch/x86/kvm/xen.c > > > +++ b/arch/x86/kvm/xen.c > > > @@ -1300,6 +1300,9 @@ int kvm_xen_hypercall(struct kvm_vcpu *vcpu) > > > > > > static inline int max_evtchn_port(struct kvm *kvm) > > > { > > > + BUILD_BUG_ON(EVTCHN_2L_NR_CHANNELS != > > > + (sizeof_field(struct shared_info, evtchn_pending) * BITS_PER_BYTE)); > > > + > > > if (IS_ENABLED(CONFIG_64BIT) && kvm->arch.xen.long_mode) > > > return EVTCHN_2L_NR_CHANNELS; > > > else > > > > Not really sure I see the point of that. > > > > There are two main reasons for that kind of BUILD_BUG_ON(). I've added > > a few of them asserting that the size of the structure and its compat > > variant are identical, and thus documenting *why* the code lacks compat > > handling. For example... > > > > /* > > * Next, write the new runstate. This is in the *same* place > > * for 32-bit and 64-bit guests, asserted here for paranoia. > > */ > > BUILD_BUG_ON(offsetof(struct vcpu_runstate_info, state) != > > offsetof(struct compat_vcpu_runstate_info, state)); > > > > The second reason is to prevent accidental screwups where our local > > definition of a structure varies from the official ABI. Like these: > > > > /* Paranoia checks on the 32-bit struct layout */ > > BUILD_BUG_ON(offsetof(struct compat_shared_info, wc) != 0x900); > > BUILD_BUG_ON(offsetof(struct compat_shared_info, arch.wc_sec_hi) != 0x924); > > BUILD_BUG_ON(offsetof(struct pvclock_vcpu_time_info, version) != 0); > > > > I don't really see the above fulfilling either of those use cases. > > > > Given that the definition of the evtchn_pending field is: > > > > xen_ulong_t evtchn_pending[sizeof(xen_ulong_t) * 8]; > > > > It's fairly tautological that the number of event channels supported is > > BITS_PER_ULONG * BITS_PER_ULONG. Which is sizeof(xen_ulong_t)² * 64 as > > defined in the official Xen headers. > > > > I don't know that we really need to add our own sanity check on the > > headers we imported from Xen. It doesn't seem to add much. > > The goal isn't to add a sanity check, it's to document what EVTCHN_2L_NR_CHANNELS > actually represents. My frustration with > > sizeof(xen_ulong_t) * sizeof(xen_ulong_t) * 64 > > is that there's nothing there that connects it back to evtchn_pending or evtchn_mask. Heh, welcome to Xen code :) We could submit patches to Xen which make that clearer. > E.g. ideally the code would be something like > > #define COMPAT_EVTCHN_2L_NR_CHANNELS 256 > > #ifdef CONFIG_X86_64 > #define EVTCHN_2L_NR_CHANNELS 512 > #else > #define EVTCHN_2L_NR_CHANNELS COMPAT_EVTCHN_2L_NR_CHANNELS > > > DECLARE_BITMAP(evtchn_pending, EVTCHN_2L_NR_CHANNELS); > DECLARE_BITMAP(evtchn_mask, EVTCHN_2L_NR_CHANNELS); > > which is much more self-documenting and doesn't require the reader to do math to > grok the basics. For the *compat* case that's entirely within arch/x86/kvm/xen.h so we really *can* flip it to '#define COMPAT_EVTCHN_2L_NR_CHANNELS 1024' and then use DECLARE_BITMAP in the struct itself. And I believe that I have enough BUILD_BUG_ON() sanity checks that if you try that with 256 channels it would kick you in the teeth for it. The fact that the official definition is an array of uint32_t is irrelevant in our code, as we always cast to (unsigned long *) when accessing the bitmaps anyway. DECLARE_BITMAP() is fine. > Anyways, we can drop this patch, it was written mostly out of frustration with > how long it took me to understand what is actually a very simple concept that's > written in an unnecessarily obscure way. Your experience is valid. This should be as understandable as possible for people who aren't intimately familiar with the Xen ABIs, and I certainly can't have an opinion on that. How's something like this? I did start typing that comment in the max_evtchn_port() function in xen.c but moved it over. Still not utterly convinced, as it's still somewhat circular — we now define NR_CHANNELS as (32*32) with a big comment explaining *why* that is, and the reason is basically "because that's the number of bits in an array of uint32_t[32]". Which might perhaps have been expressed in C instead of prose, as #define COMPAT_EVTCHN_2L_NR_CHANNELS (BITS_PER_BYTE * \ sizeof_field(struct compat_shared_info,evtchn_pending)) Signed-off-by-if-you-want-it: David Woodhouse <dwmw@amazon.co.uk> diff --git a/arch/x86/kvm/xen.h b/arch/x86/kvm/xen.h index 8503d2c6891e..96735c8ad03f 100644 --- a/arch/x86/kvm/xen.h +++ b/arch/x86/kvm/xen.h @@ -190,17 +190,24 @@ struct compat_arch_shared_info { uint32_t wc_sec_hi; }; +/* + * EVTCHN_2L_NR_CHANNELS is the number of event channels supported, + * corresponding to bits in the shinfo->evtchn_pending (and _mask) + * bitmaps. Those bitmaps are an array of + * unsigned long evtchn_pending[BITS_PER_LONG]; + * giving 1024 (32*32) or 4096 (64*64) event channels for 32-bit and + * 64-bit guests respectively. + */ +#define COMPAT_EVTCHN_2L_NR_CHANNELS (32 * 32) + struct compat_shared_info { struct compat_vcpu_info vcpu_info[MAX_VIRT_CPUS]; - uint32_t evtchn_pending[32]; - uint32_t evtchn_mask[32]; + DECLARE_BITMAP(evtchn_pending, COMPAT_EVTCHN_2L_NR_CHANNELS); + DECLARE_BITMAP(evtchn_mask, COMPAT_EVTCHN_2L_NR_CHANNELS); struct pvclock_wall_clock wc; struct compat_arch_shared_info arch; }; -#define COMPAT_EVTCHN_2L_NR_CHANNELS (8 * \ - sizeof_field(struct compat_shared_info, \ - evtchn_pending)) struct compat_vcpu_runstate_info { int state; uint64_t state_entry_time;
On Tue, Nov 22, 2022, David Woodhouse wrote: > On Tue, 2022-11-22 at 20:31 +0000, Sean Christopherson wrote: > How's something like this? I did start typing that comment in the > max_evtchn_port() function in xen.c but moved it over. > > Still not utterly convinced, as it's still somewhat circular — we now > define NR_CHANNELS as (32*32) with a big comment explaining *why* that > is, and the reason is basically "because that's the number of bits in > an array of uint32_t[32]". Agreed, probably not an improvement across the board. Consistency with how the non-compat code declares the fields is also valuable, so unless someone changes upstream Xen code, let's just leave things as-is.
diff --git a/arch/x86/kvm/xen.h b/arch/x86/kvm/xen.h index 532a535a9e99..c771fbed7058 100644 --- a/arch/x86/kvm/xen.h +++ b/arch/x86/kvm/xen.h @@ -9,6 +9,8 @@ #ifndef __ARCH_X86_KVM_XEN_H__ #define __ARCH_X86_KVM_XEN_H__ +#include <linux/bits.h> + #ifdef CONFIG_KVM_XEN #include <linux/jump_label_ratelimit.h> @@ -198,9 +200,9 @@ struct compat_shared_info { struct compat_arch_shared_info arch; }; -#define COMPAT_EVTCHN_2L_NR_CHANNELS (8 * \ - sizeof_field(struct compat_shared_info, \ - evtchn_pending)) +#define COMPAT_EVTCHN_2L_NR_CHANNELS \ + (sizeof_field(struct compat_shared_info, evtchn_pending) * BITS_PER_BYTE) + struct compat_vcpu_runstate_info { int state; uint64_t state_entry_time; diff --git a/include/xen/interface/event_channel.h b/include/xen/interface/event_channel.h index 5f8da466e8a9..c7b97c206226 100644 --- a/include/xen/interface/event_channel.h +++ b/include/xen/interface/event_channel.h @@ -10,6 +10,8 @@ #ifndef __XEN_PUBLIC_EVENT_CHANNEL_H__ #define __XEN_PUBLIC_EVENT_CHANNEL_H__ +#include <linux/bits.h> + #include <xen/interface/xen.h> typedef uint32_t evtchn_port_t; @@ -244,8 +246,8 @@ DEFINE_GUEST_HANDLE_STRUCT(evtchn_op); /* * 2-level ABI */ - -#define EVTCHN_2L_NR_CHANNELS (sizeof(xen_ulong_t) * sizeof(xen_ulong_t) * 64) +#define EVTCHN_2L_NR_CHANNELS \ + (sizeof_field(struct shared_info, evtchn_pending) * BITS_PER_BYTE) /* * FIFO ABI