Re: [PATCH v2 2/5] apic: add support for x2APIC mode
On 4/3/23 23:38, Bui Quang Minh wrote: On 4/3/23 17:27, David Woodhouse wrote: On Wed, 2023-03-29 at 22:30 +0700, Bui Quang Minh wrote: I do some more testing on my hardware, your point is correct when dest == 0x, the interrupt is delivered to all APICs regardless of their mode. To be precisely, it only broadcasts to CPUs in xAPIC mode if the IPI destination mode is physical. In case the destination mode is logical, flat model/cluster model rule applies to determine if the xAPIC CPUs accept the IPI. Wow, this is so complicated :) So even if you send to *all* of the first 8 CPUs in a cluster (e.g. cluster 0x0001 giving a destination 0x000100FF, a CPU in xAPIC mode doesn't see that as a broadcast because it's logical mode? I mean if the destination is 0x, the xAPIC CPU will see destination as 0xff. 0xff is broadcast in physical destination mode only, in logical destination, it may not be a broadcast. It may depend on the whether it is flat model or cluster model in logical destination mode. In flat model, 8 bits are used as mask, so in theory, this model can only support 8 CPUs, each CPU reserves its own bit by setting the upper 8 bits of APIC LDR register. In Intel SDM, it is said that 0xff can be interpreted as a broadcast, this is true in normal case, but I think if the CPU its APIC LDR to 0, the IPI should not deliver to this CPU. This also matches with the current flat model of logical destination mode implementation of userspace APIC in Qemu before my series. However, I see this seems like a corner case, I didn't test it on real hardware. With cluster model, when writing the above paragraphs, I think that 0xff will be delivered to all APICs (mask = 0xf) of cluster 15 (0xf). However, reading the SDM more carefully, I see that the there are only 15 clusters with address from 0 to 14 so there is no cluster with address 15. 0xff is interpreted as broadcast to all APICs in all clusters too. In conclusion, IPI with destination 0x can be a broadcast to all xAPIC CPUs too if we just ignore the corner case in flat model of logical destination mode (we may need to test more) I do some tests on my machine with custom Linux kernel module (I can't use kvm-unit-tests because the display on my laptop is on embedded display port not serial port so I don't know how to get the output). I find out that - In flat model, if the CPU intentionally set its 8 bit address to 0 in APIC_LDR, the 0xff logical IPI does not deliver to this CPU. - In cluster model, the 4 higher bits used as cluster address, if these 4 bits is equal to 0xf, the IPI is broadcasted to all cluster. The 4 lower bits are used to address APICs in the cluster. If the CPU intentionally set these 4 lower bits to 0 in APIC_LDR, same as the flat model, this CPU does not receive the logical IPI. For example, the CPUs set its address to 0x20 (cluster 2, address 0 in cluster), the logical IPI with destination == 0xff does deliver to cluster 2 but does not deliver to this CPU. So I choose to stick with the current implementation, 0x in IPI is seen as 0xff IPI in xAPIC CPUs. This IPI if in physical mode is a broadcast but not in logical mode.
Re: [PATCH v2 2/5] apic: add support for x2APIC mode
On 4/3/23 17:27, David Woodhouse wrote: On Wed, 2023-03-29 at 22:30 +0700, Bui Quang Minh wrote: I do some more testing on my hardware, your point is correct when dest == 0x, the interrupt is delivered to all APICs regardless of their mode. To be precisely, it only broadcasts to CPUs in xAPIC mode if the IPI destination mode is physical. In case the destination mode is logical, flat model/cluster model rule applies to determine if the xAPIC CPUs accept the IPI. Wow, this is so complicated :) So even if you send to *all* of the first 8 CPUs in a cluster (e.g. cluster 0x0001 giving a destination 0x000100FF, a CPU in xAPIC mode doesn't see that as a broadcast because it's logical mode? I mean if the destination is 0x, the xAPIC CPU will see destination as 0xff. 0xff is broadcast in physical destination mode only, in logical destination, it may not be a broadcast. It may depend on the whether it is flat model or cluster model in logical destination mode. In flat model, 8 bits are used as mask, so in theory, this model can only support 8 CPUs, each CPU reserves its own bit by setting the upper 8 bits of APIC LDR register. In Intel SDM, it is said that 0xff can be interpreted as a broadcast, this is true in normal case, but I think if the CPU its APIC LDR to 0, the IPI should not deliver to this CPU. This also matches with the current flat model of logical destination mode implementation of userspace APIC in Qemu before my series. However, I see this seems like a corner case, I didn't test it on real hardware. With cluster model, when writing the above paragraphs, I think that 0xff will be delivered to all APICs (mask = 0xf) of cluster 15 (0xf). However, reading the SDM more carefully, I see that the there are only 15 clusters with address from 0 to 14 so there is no cluster with address 15. 0xff is interpreted as broadcast to all APICs in all clusters too. In conclusion, IPI with destination 0x can be a broadcast to all xAPIC CPUs too if we just ignore the corner case in flat model of logical destination mode (we may need to test more) I would have assumed that a CPU in xAPIC mode would have looked at the low byte and interpreted it as xAPIC logical mode, with the cluster in the high nybble and the 4-bit mask in the low nybble? Yes, this is the behavior in cluster model of logical destination mode (I try to stick with Intel SDM Section 10.6.2.2 Volume 3A when using those terminologies)
Re: [PATCH v2 2/5] apic: add support for x2APIC mode
On 3/30/23 15:28, Igor Mammedov wrote:
On Wed, 29 Mar 2023 22:30:44 +0700
Bui Quang Minh wrote:
On 3/29/23 21:53, Bui Quang Minh wrote:
On 3/28/23 22:58, Bui Quang Minh wrote:
On 3/27/23 23:49, David Woodhouse wrote:
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 == 0x) {
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?
This is interesting. Your point looks reasonable to me but I don't
know how to verify it, I'm trying to write kernel module to test it
but there are just too many things running on Linux that uses
interrupt so the system hangs.
This raises another question: when dest == 0x102 in IPI, does the
xAPIC mode CPU with APIC ID 0x2 accept the IPI? I can't see this
stated clearly in the Intel SDM.
I do some more testing on my hardware, your point is correct when dest
== 0x, the interrupt is delivered to all APICs regardless of
their mode.
To be precisely, it only broadcasts to CPUs in xAPIC mode if the IPI
destination mode is physical. In case the destination mode is logical,
flat model/cluster model rule applies to determine if the xAPIC CPUs
accept the IPI. Wow, this is so complicated :)
It would be nice if you could update apic kvm unit test with your
findings if it doesn't test those variants yet.
And when dest == 0x102 in IPI, xAPIC mode CPU with APIC ID
0x2 also accepts that IPI.
KVM does not do the same way as the real hardware in these cases, if the
destination of IPI is 0x, IPI is broadcasted to x2APIC CPUs but
not xAPIC CPUs. The same with IPI has destination 0x102 does not deliver
to xAPIC CPU with APIC ID 0x2. This is the intended behavior as I see
some comments mentioning it.
diff --git a/x86/apic.c b/x86/apic.c
index 20c3a1a..8c91d27 100644
--- a/x86/apic.c
+++ b/x86/apic.c
@@ -679,7 +679,7 @@ static void set_ldr(void *__ldr)
apic_write(APIC_LDR, ldr << 24);
}
-static int test_fixed_ipi(u32 dest_mode, u8 dest, u8 vector,
+static int test_fixed_ipi(u32 dest_mode, u32 dest, u8 vector,
int nr_ipis_expected, const char *mode_name)
{
u64 start = rdtsc();
@@ -913,6 +913,38 @@ static void test_aliased_xapic_physical_ipi(void)
report(!f, "IPI to aliased xAPIC physical IDs");
}
+static void reset_apic_cpu(void *arg)
+{
+ u8 *id = (u8 *)arg;
+ reset_apic();
+ *id = apic_id();
+}
+
+static void test_physical_ipi_with_x2apic_id(void)
+{
+ u8 vector = 0xf1;
+ int f = 0;
+ u8 apic_id_cpu1;
+
+ if (cpu_count() < 2)
+ return;
+
+ if (!is_x2apic_enabled())
+ return;
+
+ on_cpu(1, reset_apic_cpu, &apic_id_cpu1);
+ handle_irq(vector, handle_ipi);
+
+ /*
+* CPU1 is in xAPIC so it accepts the IPI because the (destination &
0xff)
+* matches its APIC ID.
+*/
+ f += test_fixed_ipi(APIC_DEST_PHYSICAL, apic_id_cpu1 | 0x100, vector,
1, "physical");
+ f += test_fixed_ipi(APIC_DEST_PHYSICAL, 0x, vector,
cpu_count(), "physical");
+
+ report(!f, "IPI with x2apic id to xapic CPU");
+}
+
typedef void (*apic_t
Re: [PATCH v2 2/5] apic: add support for x2APIC mode
On Wed, 2023-03-29 at 22:30 +0700, Bui Quang Minh wrote: > > > > > I do some more testing on my hardware, your point is correct when dest > > == 0x, the interrupt is delivered to all APICs regardless of > > their mode. > > To be precisely, it only broadcasts to CPUs in xAPIC mode if the IPI > destination mode is physical. In case the destination mode is logical, > flat model/cluster model rule applies to determine if the xAPIC CPUs > accept the IPI. Wow, this is so complicated :) So even if you send to *all* of the first 8 CPUs in a cluster (e.g. cluster 0x0001 giving a destination 0x000100FF, a CPU in xAPIC mode doesn't see that as a broadcast because it's logical mode? I would have assumed that a CPU in xAPIC mode would have looked at the low byte and interpreted it as xAPIC logical mode, with the cluster in the high nybble and the 4-bit mask in the low nybble? smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH v2 2/5] apic: add support for x2APIC mode
On Wed, 29 Mar 2023 22:30:44 +0700
Bui Quang Minh wrote:
> On 3/29/23 21:53, Bui Quang Minh wrote:
> > On 3/28/23 22:58, Bui Quang Minh wrote:
> >> On 3/27/23 23:49, David Woodhouse wrote:
> >>> 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 == 0x) {
> > 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?
> >>
> >> This is interesting. Your point looks reasonable to me but I don't
> >> know how to verify it, I'm trying to write kernel module to test it
> >> but there are just too many things running on Linux that uses
> >> interrupt so the system hangs.
> >>
> >> This raises another question: when dest == 0x102 in IPI, does the
> >> xAPIC mode CPU with APIC ID 0x2 accept the IPI? I can't see this
> >> stated clearly in the Intel SDM.
> >
> > I do some more testing on my hardware, your point is correct when dest
> > == 0x, the interrupt is delivered to all APICs regardless of
> > their mode.
>
> To be precisely, it only broadcasts to CPUs in xAPIC mode if the IPI
> destination mode is physical. In case the destination mode is logical,
> flat model/cluster model rule applies to determine if the xAPIC CPUs
> accept the IPI. Wow, this is so complicated :)
It would be nice if you could update apic kvm unit test with your
findings if it doesn't test those variants yet.
>
>
> > And when dest == 0x102 in IPI, xAPIC mode CPU with APIC ID
> > 0x2 also accepts that IPI.
>
Re: [PATCH v2 2/5] apic: add support for x2APIC mode
On 3/29/23 21:53, Bui Quang Minh wrote:
On 3/28/23 22:58, Bui Quang Minh wrote:
On 3/27/23 23:49, David Woodhouse wrote:
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 == 0x) {
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?
This is interesting. Your point looks reasonable to me but I don't
know how to verify it, I'm trying to write kernel module to test it
but there are just too many things running on Linux that uses
interrupt so the system hangs.
This raises another question: when dest == 0x102 in IPI, does the
xAPIC mode CPU with APIC ID 0x2 accept the IPI? I can't see this
stated clearly in the Intel SDM.
I do some more testing on my hardware, your point is correct when dest
== 0x, the interrupt is delivered to all APICs regardless of
their mode.
To be precisely, it only broadcasts to CPUs in xAPIC mode if the IPI
destination mode is physical. In case the destination mode is logical,
flat model/cluster model rule applies to determine if the xAPIC CPUs
accept the IPI. Wow, this is so complicated :)
And when dest == 0x102 in IPI, xAPIC mode CPU with APIC ID
0x2 also accepts that IPI.
Re: [PATCH v2 2/5] apic: add support for x2APIC mode
On 3/28/23 22:58, Bui Quang Minh wrote:
On 3/27/23 23:49, David Woodhouse wrote:
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 == 0x) {
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?
This is interesting. Your point looks reasonable to me but I don't know
how to verify it, I'm trying to write kernel module to test it but there
are just too many things running on Linux that uses interrupt so the
system hangs.
This raises another question: when dest == 0x102 in IPI, does the xAPIC
mode CPU with APIC ID 0x2 accept the IPI? I can't see this stated
clearly in the Intel SDM.
I do some more testing on my hardware, your point is correct when dest
== 0x, the interrupt is delivered to all APICs regardless of
their mode. And when dest == 0x102 in IPI, xAPIC mode CPU with APIC ID
0x2 also accepts that IPI.
Re: [PATCH v2 2/5] apic: add support for x2APIC mode
On 3/27/23 23:49, David Woodhouse wrote:
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 == 0x) {
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?
This is interesting. Your point looks reasonable to me but I don't know
how to verify it, I'm trying to write kernel module to test it but there
are just too many things running on Linux that uses interrupt so the
system hangs.
This raises another question: when dest == 0x102 in IPI, does the xAPIC
mode CPU with APIC ID 0x2 accept the IPI? I can't see this stated
clearly in the Intel SDM.
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* :)
Re: [PATCH v2 2/5] apic: add support for x2APIC mode
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 == 0x) {
> > 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* :)
smime.p7s
Description: S/MIME cryptographic signature
Re: [PATCH v2 2/5] apic: add support for x2APIC mode
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 == 0x) {
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. Do you think the code here is tricky and hard to read?
Re: [PATCH v2 2/5] apic: add support for x2APIC mode
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 == 0x) {
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 {
smime.p7s
Description: S/MIME cryptographic signature
Re: [PATCH v2 2/5] apic: add support for x2APIC mode
On 3/27/23 22:37, David Woodhouse wrote:
On Mon, 2023-03-27 at 22:33 +0700, Bui Quang Minh wrote:
+ memset(deliver_bitmask, 0x00, max_apic_words * sizeof(uint32_t));
+
+ /* x2APIC broadcast id for both physical and logical (cluster) mode */
+ if (dest == 0x) {
+ apic_get_broadcast_bitmask(deliver_bitmask, true);
+ return;
+ }
+
if (dest_mode == 0) {
Might be nice to have a constant for DEST_MODE_PHYS vs.
DEST_MODE_LOGICAL to make this clearer?
I'll fix it in the next version.
+ apic_find_dest(deliver_bitmask, dest);
+ /* Broadcast to xAPIC mode apics */
if (dest == 0xff) {
- memset(deliver_bitmask, 0xff, MAX_APIC_WORDS * sizeof(uint32_t));
- } else {
- int idx = apic_find_dest(dest);
- memset(deliver_bitmask, 0x00, MAX_APIC_WORDS * sizeof(uint32_t));
- if (idx >= 0)
- apic_set_bit(deliver_bitmask, idx);
+ apic_get_broadcast_bitmask(deliver_bitmask, false);
Hrm... aren't you still interpreting destination 0x00FF as
broadcast even for X2APIC mode? Or am I misreading this?
In case the destination is 0xFF, the IPI will be delivered to CPU has
APIC ID 0xFF if it is in x2APIC mode, and it will be delivered to all
CPUs that are in xAPIC mode. In case the destination is 0x, the
IPI is delivered to all CPUs that are in x2APIC mode. I've created
apic_get_broadcast_bitmask function and changed the apic_find_dest to
implement that logic.
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
static void apic_get_broadcast_bitmask(uint32_t *deliver_bitmask,
bool is_x2apic_broadcast)
{
int i;
APICCommonState *apic_iter;
for (i = 0; i < max_apics; i++) {
apic_iter = local_apics[i];
if (apic_iter) {
bool apic_in_x2apic = is_x2apic_mode(&apic_iter->parent_obj);
if (is_x2apic_broadcast && apic_in_x2apic) {
apic_set_bit(deliver_bitmask, i);
} else if (!is_x2apic_broadcast && !apic_in_x2apic) {
apic_set_bit(deliver_bitmask, i);
}
}
}
}
In apic_get_broadcast_bitmask, because is_x2apic_broadcast == false, the
delivery bit set only if that apic_in_x2apic == false (that CPU is in
xAPIC mode)
Re: [PATCH v2 2/5] apic: add support for x2APIC mode
On Mon, 2023-03-27 at 22:33 +0700, Bui Quang Minh wrote:
>
> > > + memset(deliver_bitmask, 0x00, max_apic_words * sizeof(uint32_t));
> > > +
> > > + /* x2APIC broadcast id for both physical and logical (cluster) mode
> > > */
> > > + if (dest == 0x) {
> > > + apic_get_broadcast_bitmask(deliver_bitmask, true);
> > > + return;
> > > + }
> > > +
> > > if (dest_mode == 0) {
> >
> > Might be nice to have a constant for DEST_MODE_PHYS vs.
> > DEST_MODE_LOGICAL to make this clearer?
>
> I'll fix it in the next version.
>
> > > + apic_find_dest(deliver_bitmask, dest);
> > > + /* Broadcast to xAPIC mode apics */
> > > if (dest == 0xff) {
> > > - memset(deliver_bitmask, 0xff, MAX_APIC_WORDS *
> > > sizeof(uint32_t));
> > > - } else {
> > > - int idx = apic_find_dest(dest);
> > > - memset(deliver_bitmask, 0x00, MAX_APIC_WORDS *
> > > sizeof(uint32_t));
> > > - if (idx >= 0)
> > > - apic_set_bit(deliver_bitmask, idx);
> > > + apic_get_broadcast_bitmask(deliver_bitmask, false);
> >
> >
> > Hrm... aren't you still interpreting destination 0x00FF as
> > broadcast even for X2APIC mode? Or am I misreading this?
>
> In case the destination is 0xFF, the IPI will be delivered to CPU has
> APIC ID 0xFF if it is in x2APIC mode, and it will be delivered to all
> CPUs that are in xAPIC mode. In case the destination is 0x, the
> IPI is delivered to all CPUs that are in x2APIC mode. I've created
> apic_get_broadcast_bitmask function and changed the apic_find_dest to
> implement that logic.
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?
smime.p7s
Description: S/MIME cryptographic signature
Re: [PATCH v2 2/5] apic: add support for x2APIC mode
On 3/27/23 18:04, David Woodhouse wrote:
On Sun, 2023-03-26 at 12:20 +0700, Bui Quang Minh wrote:
This commit extends the APIC ID to 32-bit long and remove the 255 max APIC
ID limit in userspace APIC. The array that manages local APICs is now
dynamically allocated based on the max APIC ID of created x86 machine.
Also, new x2APIC IPI destination determination scheme, self IPI and x2APIC
mode register access are supported.
Signed-off-by: Bui Quang Minh
---
hw/i386/x86.c | 8 +-
hw/intc/apic.c | 229 +++-
hw/intc/apic_common.c | 8 +-
include/hw/i386/apic.h | 3 +-
include/hw/i386/apic_internal.h | 2 +-
5 files changed, 184 insertions(+), 66 deletions(-)
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index a88a126123..fa9b15190d 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -132,11 +132,11 @@ void x86_cpus_init(X86MachineState *x86ms, int
default_cpu_version)
* Can we support APIC ID 255 or higher?
*
* Under Xen: yes.
- * With userspace emulated lapic: no
+ * With userspace emulated lapic: yes.
Are you making this unconditional? It shall not be possible to emulate
a CPU *without* X2APIC?
You are right, this should report error when APIC ID is higher than 255
and x2APIC is not supported by the CPU.
* With KVM's in-kernel lapic: only if X2APIC API is enabled.
*/
if (x86ms->apic_id_limit > 255 && !xen_enabled() &&
- (!kvm_irqchip_in_kernel() || !kvm_enable_x2apic())) {
+ kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
error_report("current -smp configuration requires kernel "
"irqchip and X2APIC API support.");
exit(EXIT_FAILURE);
...
@@ -276,16 +288,17 @@ static void apic_bus_deliver(const uint32_t
*deliver_bitmask,
apic_set_irq(apic_iter, vector_num, trigger_mode) );
}
-void apic_deliver_irq(uint8_t dest, uint8_t dest_mode, uint8_t delivery_mode,
+void apic_deliver_irq(uint32_t dest, uint8_t dest_mode, uint8_t delivery_mode,
uint8_t vector_num, uint8_t trigger_mode)
We can make this 'static' while we're here. It isn't actually called
from anywhere else, is it?
I'll fix it in the next version.
static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
- uint8_t dest, uint8_t dest_mode)
+ 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 == 0x) {
+ apic_get_broadcast_bitmask(deliver_bitmask, true);
+ return;
+ }
+
if (dest_mode == 0) {
Might be nice to have a constant for DEST_MODE_PHYS vs.
DEST_MODE_LOGICAL to make this clearer?
I'll fix it in the next version.
+ apic_find_dest(deliver_bitmask, dest);
+ /* Broadcast to xAPIC mode apics */
if (dest == 0xff) {
- memset(deliver_bitmask, 0xff, MAX_APIC_WORDS * sizeof(uint32_t));
- } else {
- int idx = apic_find_dest(dest);
- memset(deliver_bitmask, 0x00, MAX_APIC_WORDS * sizeof(uint32_t));
- if (idx >= 0)
- apic_set_bit(deliver_bitmask, idx);
+ apic_get_broadcast_bitmask(deliver_bitmask, false);
Hrm... aren't you still interpreting destination 0x00FF as
broadcast even for X2APIC mode? Or am I misreading this?
In case the destination is 0xFF, the IPI will be delivered to CPU has
APIC ID 0xFF if it is in x2APIC mode, and it will be delivered to all
CPUs that are in xAPIC mode. In case the destination is 0x, the
IPI is delivered to all CPUs that are in x2APIC mode. I've created
apic_get_broadcast_bitmask function and changed the apic_find_dest to
implement that logic.
@@ -366,7 +370,7 @@ static const VMStateDescription vmstate_apic_common = {
VMSTATE_UINT8(arb_id, APICCommonState),
VMSTATE_UINT8(tpr, APICCommonState),
VMSTATE_UINT32(spurious_vec, APICCommonState),
- VMSTATE_UINT8(log_dest, APICCommonState),
+ VMSTATE_UINT32(log_dest, APICCommonState),
VMSTATE_UINT8(dest_mode, APICCommonState),
VMSTATE_UINT32_ARRAY(isr, APICCommonState, 8),
VMSTATE_UINT32_ARRAY(tmr, APICCommonState, 8),
Hm, doesn't this need to be added in a separate subsection, much as
ide_drive/pio_state in the example in docs/devel/migration.rst? Or did
I *not* need to do that in commit ecb0e98b4 (unrelated to x2apic, but
similar addition of state)?
Can you confirm that you've tested the behaviour when migrating back
from this to an older QEMU, both for a guest *with* X2APIC enabled
(which should fail gracefully), and a guest without X2APIC (which
should work).
Oh, thank you for pointing out, I actually
Re: [PATCH v2 2/5] apic: add support for x2APIC mode
On Sun, 2023-03-26 at 12:20 +0700, Bui Quang Minh wrote:
> This commit extends the APIC ID to 32-bit long and remove the 255 max APIC
> ID limit in userspace APIC. The array that manages local APICs is now
> dynamically allocated based on the max APIC ID of created x86 machine.
> Also, new x2APIC IPI destination determination scheme, self IPI and x2APIC
> mode register access are supported.
>
> Signed-off-by: Bui Quang Minh
> ---
> hw/i386/x86.c | 8 +-
> hw/intc/apic.c | 229 +++-
> hw/intc/apic_common.c | 8 +-
> include/hw/i386/apic.h | 3 +-
> include/hw/i386/apic_internal.h | 2 +-
> 5 files changed, 184 insertions(+), 66 deletions(-)
>
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index a88a126123..fa9b15190d 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -132,11 +132,11 @@ void x86_cpus_init(X86MachineState *x86ms, int
> default_cpu_version)
> * Can we support APIC ID 255 or higher?
> *
> * Under Xen: yes.
> - * With userspace emulated lapic: no
> + * With userspace emulated lapic: yes.
Are you making this unconditional? It shall not be possible to emulate
a CPU *without* X2APIC?
> * With KVM's in-kernel lapic: only if X2APIC API is enabled.
> */
> if (x86ms->apic_id_limit > 255 && !xen_enabled() &&
> - (!kvm_irqchip_in_kernel() || !kvm_enable_x2apic())) {
> + kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
> error_report("current -smp configuration requires kernel "
> "irqchip and X2APIC API support.");
> exit(EXIT_FAILURE);
...
> @@ -276,16 +288,17 @@ static void apic_bus_deliver(const uint32_t
> *deliver_bitmask,
> apic_set_irq(apic_iter, vector_num, trigger_mode) );
> }
>
> -void apic_deliver_irq(uint8_t dest, uint8_t dest_mode, uint8_t delivery_mode,
> +void apic_deliver_irq(uint32_t dest, uint8_t dest_mode, uint8_t
> delivery_mode,
> uint8_t vector_num, uint8_t trigger_mode)
We can make this 'static' while we're here. It isn't actually called
from anywhere else, is it?
> {
> - uint32_t deliver_bitmask[MAX_APIC_WORDS];
> + uint32_t *deliver_bitmask = g_malloc(max_apic_words * sizeof(uint32_t));
>
> trace_apic_deliver_irq(dest, dest_mode, delivery_mode, vector_num,
> trigger_mode);
>
> apic_get_delivery_bitmask(deliver_bitmask, dest, dest_mode);
> apic_bus_deliver(deliver_bitmask, delivery_mode, vector_num,
> trigger_mode);
> + g_free(deliver_bitmask);
> }
>
> bool is_x2apic_mode(DeviceState *dev)
...
>
> static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
> - uint8_t dest, uint8_t dest_mode)
> + 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 == 0x) {
> + apic_get_broadcast_bitmask(deliver_bitmask, true);
> + return;
> + }
> +
> if (dest_mode == 0) {
Might be nice to have a constant for DEST_MODE_PHYS vs.
DEST_MODE_LOGICAL to make this clearer?
> + apic_find_dest(deliver_bitmask, dest);
> + /* Broadcast to xAPIC mode apics */
> if (dest == 0xff) {
> - memset(deliver_bitmask, 0xff, MAX_APIC_WORDS * sizeof(uint32_t));
> - } else {
> - int idx = apic_find_dest(dest);
> - memset(deliver_bitmask, 0x00, MAX_APIC_WORDS * sizeof(uint32_t));
> - if (idx >= 0)
> - apic_set_bit(deliver_bitmask, idx);
> + apic_get_broadcast_bitmask(deliver_bitmask, false);
Hrm... aren't you still interpreting destination 0x00FF as
broadcast even for X2APIC mode? Or am I misreading this?
> }
> } else {
> /* XXX: cluster mode */
>
...
> @@ -366,7 +370,7 @@ static const VMStateDescription vmstate_apic_common = {
> VMSTATE_UINT8(arb_id, APICCommonState),
> VMSTATE_UINT8(tpr, APICCommonState),
> VMSTATE_UINT32(spurious_vec, APICCommonState),
> - VMSTATE_UINT8(log_dest, APICCommonState),
> + VMSTATE_UINT32(log_dest, APICCommonState),
> VMSTATE_UINT8(dest_mode, APICCommonState),
> VMSTATE_UINT32_ARRAY(isr, APICCommonState, 8),
> VMSTATE_UINT32_ARRAY(tmr, APICCommonState, 8),
Hm, doesn't this need to be added in a separate subsection, much as
ide_drive/pio_state in the example in docs/devel/migration.rst? Or did
I *not* need to do that in commit ecb0e98b4 (unrelated to x2apic, but
similar addition of state)?
Can you confirm that you've tested the behaviour when migrating back
from this to an older QEMU, both for a guest *with* X2APIC enabled
(which should fail gracefully), and a guest wi
