[PATCH 0/3] Switch flat driver to use phys dst for ext ints

[PATCH 0/3] Switch flat driver to use phys dst for ext ints

[Matthew Barnes]

This patch series switches the apic_default APIC driver from using
logical flat destination mode for external interrupts, to using
physical destination mode for external interrupts.

This is followed up by two non-functional cleanup commits.

Matthew Barnes (3):
  x86/APIC: Switch flat driver to use phys dst for ext ints
  x86/APIC: Remove unnecessary logical flat functions
  x86/APIC: Refactor GENAPIC_FLAT -> GENAPIC_MIXED

 xen/arch/x86/genapic/default.c     |  2 +-
 xen/arch/x86/genapic/delivery.c    | 10 ----------
 xen/arch/x86/include/asm/genapic.h | 14 ++++++--------
 3 files changed, 7 insertions(+), 19 deletions(-)

-- 
2.46.0

[PATCH 1/3] x86/APIC: Switch flat driver to use phys dst for ext ints

[Matthew Barnes]

Resolves: https://gitlab.com/xen-project/xen/-/issues/194
Signed-off-by: Matthew Barnes <matthew.barnes@cloud.com>
---
 xen/arch/x86/include/asm/genapic.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/include/asm/genapic.h b/xen/arch/x86/include/asm/genapic.h
index a694371c6d16..d29bccc39b4b 100644
--- a/xen/arch/x86/include/asm/genapic.h
+++ b/xen/arch/x86/include/asm/genapic.h
@@ -45,21 +45,21 @@ void cf_check send_IPI_self_legacy(uint8_t vector);
 
 void cf_check init_apic_ldr_flat(void);
 unsigned int cf_check cpu_mask_to_apicid_flat(const cpumask_t *cpumask);
+const cpumask_t *cf_check vector_allocation_cpumask_phys(int cpu);
 void cf_check send_IPI_mask_flat(const cpumask_t *cpumask, int vector);
 const cpumask_t *cf_check vector_allocation_cpumask_flat(int cpu);
 #define GENAPIC_FLAT \
-	.int_delivery_mode = dest_LowestPrio, \
-	.int_dest_mode = 1 /* logical delivery */, \
+	.int_delivery_mode = dest_Fixed, \
+	.int_dest_mode = 0 /* physical delivery */, \
 	.init_apic_ldr = init_apic_ldr_flat, \
-	.vector_allocation_cpumask = vector_allocation_cpumask_flat, \
-	.cpu_mask_to_apicid = cpu_mask_to_apicid_flat, \
+	.vector_allocation_cpumask = vector_allocation_cpumask_phys, \
+	.cpu_mask_to_apicid = cpu_mask_to_apicid_phys, \
 	.send_IPI_mask = send_IPI_mask_flat, \
 	.send_IPI_self = send_IPI_self_legacy
 
 void cf_check init_apic_ldr_phys(void);
 unsigned int cf_check cpu_mask_to_apicid_phys(const cpumask_t *cpumask);
 void cf_check send_IPI_mask_phys(const cpumask_t *mask, int vector);
-const cpumask_t *cf_check vector_allocation_cpumask_phys(int cpu);
 #define GENAPIC_PHYS \
 	.int_delivery_mode = dest_Fixed, \
 	.int_dest_mode = 0 /* physical delivery */, \
-- 
2.46.0

Re: [PATCH 1/3] x86/APIC: Switch flat driver to use phys dst for ext ints

[Roger Pau Monné]

On Wed, Oct 02, 2024 at 04:17:24PM +0100, Matthew Barnes wrote:

The commit needs a log, doesn't need to be extremely long, but it's
important to note the reasoning behind using physical delivery for
external interrupts vs logial mode.

Take a look at the x2APIC one for inspiration, the motivation should
be similar.

Thanks, Roger.

Re: [PATCH 1/3] x86/APIC: Switch flat driver to use phys dst for ext ints

[Jan Beulich]

On 03.10.2024 12:51, Roger Pau Monné wrote:
> On Wed, Oct 02, 2024 at 04:17:24PM +0100, Matthew Barnes wrote:
> 
> The commit needs a log, doesn't need to be extremely long, but it's
> important to note the reasoning behind using physical delivery for
> external interrupts vs logial mode.

Furthermore I question that the naming can remain as is - the driver
is no longer uniformly "flat" then.

Jan

Re: [PATCH 1/3] x86/APIC: Switch flat driver to use phys dst for ext ints

[Roger Pau Monné]

On Fri, Oct 04, 2024 at 08:48:05AM +0200, Jan Beulich wrote:
> On 03.10.2024 12:51, Roger Pau Monné wrote:
> > On Wed, Oct 02, 2024 at 04:17:24PM +0100, Matthew Barnes wrote:
> > 
> > The commit needs a log, doesn't need to be extremely long, but it's
> > important to note the reasoning behind using physical delivery for
> > external interrupts vs logial mode.
> 
> Furthermore I question that the naming can remain as is - the driver
> is no longer uniformly "flat" then.

Yeah, that's done in a later patch.  I wouldn't mind if it was all
folded into the same patch TBH.

Thanks, Roger.

[PATCH 2/3] x86/APIC: Remove unnecessary logical flat functions

[Matthew Barnes]

Now that GENAPIC_FLAT uses physical destination for external interrupts,
these functions implementing logical flat for external interrupts are no
longer used.

No functional change.

Signed-off-by: Matthew Barnes <matthew.barnes@cloud.com>
---
 xen/arch/x86/genapic/delivery.c    | 10 ----------
 xen/arch/x86/include/asm/genapic.h |  2 --
 2 files changed, 12 deletions(-)

diff --git a/xen/arch/x86/genapic/delivery.c b/xen/arch/x86/genapic/delivery.c
index d1f99bf6834a..3def78f380d3 100644
--- a/xen/arch/x86/genapic/delivery.c
+++ b/xen/arch/x86/genapic/delivery.c
@@ -19,16 +19,6 @@ void cf_check init_apic_ldr_flat(void)
 	apic_write(APIC_LDR, val);
 }
 
-const cpumask_t *cf_check vector_allocation_cpumask_flat(int cpu)
-{
-	return &cpu_online_map;
-} 
-
-unsigned int cf_check cpu_mask_to_apicid_flat(const cpumask_t *cpumask)
-{
-	return cpumask_bits(cpumask)[0]&0xFF;
-}
-
 /*
  * PHYSICAL DELIVERY MODE (unicast to physical APIC IDs).
  */
diff --git a/xen/arch/x86/include/asm/genapic.h b/xen/arch/x86/include/asm/genapic.h
index d29bccc39b4b..91fdeaca47b3 100644
--- a/xen/arch/x86/include/asm/genapic.h
+++ b/xen/arch/x86/include/asm/genapic.h
@@ -44,10 +44,8 @@ extern const struct genapic apic_bigsmp;
 void cf_check send_IPI_self_legacy(uint8_t vector);
 
 void cf_check init_apic_ldr_flat(void);
-unsigned int cf_check cpu_mask_to_apicid_flat(const cpumask_t *cpumask);
 const cpumask_t *cf_check vector_allocation_cpumask_phys(int cpu);
 void cf_check send_IPI_mask_flat(const cpumask_t *cpumask, int vector);
-const cpumask_t *cf_check vector_allocation_cpumask_flat(int cpu);
 #define GENAPIC_FLAT \
 	.int_delivery_mode = dest_Fixed, \
 	.int_dest_mode = 0 /* physical delivery */, \
-- 
2.46.0

Re: [PATCH 2/3] x86/APIC: Remove unnecessary logical flat functions

[Roger Pau Monné]

On Wed, Oct 02, 2024 at 04:17:25PM +0100, Matthew Barnes wrote:
> Now that GENAPIC_FLAT uses physical destination for external interrupts,
> these functions implementing logical flat for external interrupts are no
> longer used.

I think it's fine to merge this with the previous commit, so that the
functions are removed at the point there are no longer used.

Thanks, Roger.

Re: [PATCH 2/3] x86/APIC: Remove unnecessary logical flat functions

[Jan Beulich]

On 03.10.2024 12:51, Roger Pau Monné wrote:
> On Wed, Oct 02, 2024 at 04:17:25PM +0100, Matthew Barnes wrote:
>> Now that GENAPIC_FLAT uses physical destination for external interrupts,
>> these functions implementing logical flat for external interrupts are no
>> longer used.
> 
> I think it's fine to merge this with the previous commit, so that the
> functions are removed at the point there are no longer used.

Indeed - no reason to even transiently introduce new Misra violations.

Jan

[PATCH 3/3] x86/APIC: Refactor GENAPIC_FLAT -> GENAPIC_MIXED

[Matthew Barnes]

The macro name is changed to somewhat mirror x2apic driver code,
as logical flat is no longer used for external interrupts for this
mode.

No functional change.

Signed-off-by: Matthew Barnes <matthew.barnes@cloud.com>
---
 xen/arch/x86/genapic/default.c     | 2 +-
 xen/arch/x86/include/asm/genapic.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/genapic/default.c b/xen/arch/x86/genapic/default.c
index a968836a1878..3f0d57ea5e25 100644
--- a/xen/arch/x86/genapic/default.c
+++ b/xen/arch/x86/genapic/default.c
@@ -16,5 +16,5 @@
 /* should be called last. */
 const struct genapic __initconst_cf_clobber apic_default = {
 	APIC_INIT("default", NULL),
-	GENAPIC_FLAT
+	GENAPIC_MIXED
 };
diff --git a/xen/arch/x86/include/asm/genapic.h b/xen/arch/x86/include/asm/genapic.h
index 91fdeaca47b3..fcb3eb04db7b 100644
--- a/xen/arch/x86/include/asm/genapic.h
+++ b/xen/arch/x86/include/asm/genapic.h
@@ -46,7 +46,7 @@ void cf_check send_IPI_self_legacy(uint8_t vector);
 void cf_check init_apic_ldr_flat(void);
 const cpumask_t *cf_check vector_allocation_cpumask_phys(int cpu);
 void cf_check send_IPI_mask_flat(const cpumask_t *cpumask, int vector);
-#define GENAPIC_FLAT \
+#define GENAPIC_MIXED \
 	.int_delivery_mode = dest_Fixed, \
 	.int_dest_mode = 0 /* physical delivery */, \
 	.init_apic_ldr = init_apic_ldr_flat, \
-- 
2.46.0

Re: [PATCH 0/3] Switch flat driver to use phys dst for ext ints

[Andrew Cooper]

On 02/10/2024 4:17 pm, Matthew Barnes wrote:
> This patch series switches the apic_default APIC driver from using
> logical flat destination mode for external interrupts, to using
> physical destination mode for external interrupts.
>
> This is followed up by two non-functional cleanup commits.
>
> Matthew Barnes (3):
>   x86/APIC: Switch flat driver to use phys dst for ext ints
>   x86/APIC: Remove unnecessary logical flat functions
>   x86/APIC: Refactor GENAPIC_FLAT -> GENAPIC_MIXED

Patches 1 and 2 look fine.

For patch 3, can't we just delete the macro and expand it in it's single
location?

It's a bigger patch, but a better improvement in genapic.h

~Andrew

Re: [PATCH 0/3] Switch flat driver to use phys dst for ext ints

[Roger Pau Monné]

On Wed, Oct 02, 2024 at 06:45:51PM +0100, Andrew Cooper wrote:
> On 02/10/2024 4:17 pm, Matthew Barnes wrote:
> > This patch series switches the apic_default APIC driver from using
> > logical flat destination mode for external interrupts, to using
> > physical destination mode for external interrupts.
> >
> > This is followed up by two non-functional cleanup commits.
> >
> > Matthew Barnes (3):
> >   x86/APIC: Switch flat driver to use phys dst for ext ints
> >   x86/APIC: Remove unnecessary logical flat functions
> >   x86/APIC: Refactor GENAPIC_FLAT -> GENAPIC_MIXED
> 
> Patches 1 and 2 look fine.
> 
> For patch 3, can't we just delete the macro and expand it in it's single
> location?
> 
> It's a bigger patch, but a better improvement in genapic.h

I agree, but if we go that route we might as well do the conversion of
GENAPIC_PHYS in the same patch, and deal with both at the same time
(GENAPIC_PHYS is also used in a single place).

Thanks, Roger.