Date:   Tue, 22 Nov 2022 23:48:51 +0000
From:   David Woodhouse <dwmw2@...radead.org>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Paul Durrant <paul@....org>
Subject: Re: [PATCH] KVM: x86/xen: Make number of event channels defines
 less magical

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@...zon.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;


Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)

Powered by blists - more mailing lists