Re: [PATCH v2 2/5] apic: add support for x2APIC mode

From: David Woodhouse <dwmw2@infradead.org>
To: Bui Quang Minh <minhquangbui99@gmail.com>, qemu-devel@nongnu.org
Cc: "Paolo Bonzini" <pbonzini@redhat.com>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"Eduardo Habkost" <eduardo@habkost.net>,
	"Michael S . Tsirkin" <mst@redhat.com>,
	"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
	"Igor Mammedov" <imammedo@redhat.com>,
	"Alex Bennée" <alex.bennee@linaro.org>
Subject: Re: [PATCH v2 2/5] apic: add support for x2APIC mode
Date: Mon, 27 Mar 2023 17:49:06 +0100	[thread overview]
Message-ID: <e47b58ef574bcf61259d7d3f0707a1f5ca808ff6.camel@infradead.org> (raw)
In-Reply-To: <445928d9-4cd3-978d-ce76-9cd01457b6f0@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2957 bytes --]

On Mon, 2023-03-27 at 23:35 +0700, Bui Quang Minh wrote:
> On 3/27/23 23:22, David Woodhouse wrote:
> > On Mon, 2023-03-27 at 22:45 +0700, Bui Quang Minh wrote:
> > > 
> > > > Maybe I'm misreading the patch, but to me it looks that
> > > > if (dest == 0xff) apic_get_broadcast_bitmask() bit applies even in
> > > > x2apic mode? So delivering to the APIC with physical ID 255 will be
> > > > misinterpreted as a broadcast?
> > > 
> > > In case dest == 0xff the second argument to apic_get_broadcast_bitmask
> > > is set to false which means this is xAPIC broadcast
> > 
> > Yeah, but it *isn't* xAPIC broadcast. It's X2APIC unicast to APIC#255.
> > 
> > I think you want (although you don't have 'dev') something like this:
> > 
> > 
> > static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
> >                                        uint32_t dest, uint8_t dest_mode)
> > {
> >      APICCommonState *apic_iter;
> >      int i;
> > 
> >      memset(deliver_bitmask, 0x00, max_apic_words * sizeof(uint32_t));
> > 
> >      /* x2APIC broadcast id for both physical and logical (cluster) mode */
> >      if (dest == 0xffffffff) {
> >          apic_get_broadcast_bitmask(deliver_bitmask, true);
> >          return;
> >      }
> > 
> >      if (dest_mode == 0) {
> >          apic_find_dest(deliver_bitmask, dest);
> >          /* Broadcast to xAPIC mode apics */
> > -        if (dest == 0xff) {
> > +        if (dest == 0xff && is_x2apic_mode(dev)) {
> >              apic_get_broadcast_bitmask(deliver_bitmask, false);
> >          }
> >      } else {
> > 
> 
> Hmm, the unicast case is handled in apic_find_dest function, the logic 
> inside the if (dest == 0xff) is for handling the broadcast case only.
> This is because when dest == 0xff, it can be both a x2APIC unicast and 
> xAPIC broadcast in case we have some CPUs that are in xAPIC and others 
> are in x2APIC.

Ah! Yes, I see it now.

Shouldn't apic_get_broadcast_bitmask(… true) add *all* APICs to the
mask, regardless of their mode? An APIC which is still in xAPIC mode
will only look at the low 8 bits and see 0xFF which it also interprets
as broadcast? Or is that not how real hardware behaves?

>  Do you think the code here is tricky and hard to read?

Well, I completely failed to read it... :)

I think changing the existing comment something like this might help...

-         /* Broadcast to xAPIC mode apics */
+         /* Any APIC in xAPIC mode will interpret 0xFF as broadcast */

Coupled with a comment on apic_get_delivery_bitmask() clarifying that
it depends on the mode of each APIC it considers — which is obvious
enough in retrospect now I read the code and you point it out to me,
but empirically, we have to concede that it wasn't obvious *enough* :)

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]