xen/arch/x86/genapic/bigsmp.c | 8 +++++++- xen/arch/x86/genapic/default.c | 8 +++++++- xen/arch/x86/genapic/delivery.c | 10 ---------- xen/arch/x86/include/asm/genapic.h | 20 +------------------- 4 files changed, 15 insertions(+), 31 deletions(-)
External interrupts via logical delivery mode in xAPIC do not benefit
from targeting multiple CPUs and instead simply bloat up the vector
space.
However the xAPIC flat driver currently uses logical delivery for
external interrupts.
This patch switches the xAPIC flat driver to use physical destination
mode for external interrupts, instead of logical destination mode.
This patch also applies the following non-functional changes:
- Remove now unused logical flat functions
- Expand GENAPIC_FLAT and GENAPIC_PHYS macros, and delete them.
Resolves: https://gitlab.com/xen-project/xen/-/issues/194
Signed-off-by: Matthew Barnes <matthew.barnes@cloud.com>
---
Changes in v2:
- Reword commit message
- Expand and delete GENAPIC_* macros
- Bundle patch series into one patch
---
xen/arch/x86/genapic/bigsmp.c | 8 +++++++-
xen/arch/x86/genapic/default.c | 8 +++++++-
xen/arch/x86/genapic/delivery.c | 10 ----------
xen/arch/x86/include/asm/genapic.h | 20 +-------------------
4 files changed, 15 insertions(+), 31 deletions(-)
diff --git a/xen/arch/x86/genapic/bigsmp.c b/xen/arch/x86/genapic/bigsmp.c
index e97e4453a033..b2e721845275 100644
--- a/xen/arch/x86/genapic/bigsmp.c
+++ b/xen/arch/x86/genapic/bigsmp.c
@@ -46,5 +46,11 @@ static int __init cf_check probe_bigsmp(void)
const struct genapic __initconst_cf_clobber apic_bigsmp = {
APIC_INIT("bigsmp", probe_bigsmp),
- GENAPIC_PHYS
+ .int_delivery_mode = dest_Fixed,
+ .int_dest_mode = 0, /* physical delivery */
+ .init_apic_ldr = init_apic_ldr_phys,
+ .vector_allocation_cpumask = vector_allocation_cpumask_phys,
+ .cpu_mask_to_apicid = cpu_mask_to_apicid_phys,
+ .send_IPI_mask = send_IPI_mask_phys,
+ .send_IPI_self = send_IPI_self_legacy
};
diff --git a/xen/arch/x86/genapic/default.c b/xen/arch/x86/genapic/default.c
index a968836a1878..59c79afdb8fa 100644
--- a/xen/arch/x86/genapic/default.c
+++ b/xen/arch/x86/genapic/default.c
@@ -16,5 +16,11 @@
/* should be called last. */
const struct genapic __initconst_cf_clobber apic_default = {
APIC_INIT("default", NULL),
- GENAPIC_FLAT
+ .int_delivery_mode = dest_Fixed,
+ .int_dest_mode = 0, /* physical delivery */
+ .init_apic_ldr = init_apic_ldr_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
};
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 a694371c6d16..2f46f173c1d4 100644
--- a/xen/arch/x86/include/asm/genapic.h
+++ b/xen/arch/x86/include/asm/genapic.h
@@ -44,29 +44,11 @@ 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_LowestPrio, \
- .int_dest_mode = 1 /* logical delivery */, \
- .init_apic_ldr = init_apic_ldr_flat, \
- .vector_allocation_cpumask = vector_allocation_cpumask_flat, \
- .cpu_mask_to_apicid = cpu_mask_to_apicid_flat, \
- .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 */, \
- .init_apic_ldr = init_apic_ldr_phys, \
- .vector_allocation_cpumask = vector_allocation_cpumask_phys, \
- .cpu_mask_to_apicid = cpu_mask_to_apicid_phys, \
- .send_IPI_mask = send_IPI_mask_phys, \
- .send_IPI_self = send_IPI_self_legacy
#endif
--
2.46.0On 07.10.2024 16:34, Matthew Barnes wrote: > --- a/xen/arch/x86/include/asm/genapic.h > +++ b/xen/arch/x86/include/asm/genapic.h > @@ -44,29 +44,11 @@ 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); Why does this decl move to between two flat decls, rather than ... > 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 */, \ > - .init_apic_ldr = init_apic_ldr_flat, \ > - .vector_allocation_cpumask = vector_allocation_cpumask_flat, \ > - .cpu_mask_to_apicid = cpu_mask_to_apicid_flat, \ > - .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); ... staying where it was, next to other phys ones? I can certainly adjust this back while committing ... Jan
On Tue, Oct 08, 2024 at 03:47:41PM +0200, Jan Beulich wrote: > On 07.10.2024 16:34, Matthew Barnes wrote: > > --- a/xen/arch/x86/include/asm/genapic.h > > +++ b/xen/arch/x86/include/asm/genapic.h > > @@ -44,29 +44,11 @@ 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); > > Why does this decl move to between two flat decls, rather than ... > > > 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 */, \ > > - .init_apic_ldr = init_apic_ldr_flat, \ > > - .vector_allocation_cpumask = vector_allocation_cpumask_flat, \ > > - .cpu_mask_to_apicid = cpu_mask_to_apicid_flat, \ > > - .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); > > ... staying where it was, next to other phys ones? My bad, it's a remnant from an older version of this patch. There's no reason for the declaration to move. - Matt
On Mon, Oct 07, 2024 at 03:34:43PM +0100, Matthew Barnes wrote:
> External interrupts via logical delivery mode in xAPIC do not benefit
> from targeting multiple CPUs and instead simply bloat up the vector
> space.
>
> However the xAPIC flat driver currently uses logical delivery for
> external interrupts.
>
> This patch switches the xAPIC flat driver to use physical destination
> mode for external interrupts, instead of logical destination mode.
>
> This patch also applies the following non-functional changes:
> - Remove now unused logical flat functions
> - Expand GENAPIC_FLAT and GENAPIC_PHYS macros, and delete them.
>
> Resolves: https://gitlab.com/xen-project/xen/-/issues/194
> Signed-off-by: Matthew Barnes <matthew.barnes@cloud.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Just some nits below (and suggestions for future work).
> ---
> Changes in v2:
> - Reword commit message
> - Expand and delete GENAPIC_* macros
> - Bundle patch series into one patch
> ---
> xen/arch/x86/genapic/bigsmp.c | 8 +++++++-
> xen/arch/x86/genapic/default.c | 8 +++++++-
The organization of giles herre is very bad. bigsmp.c and default.c are
almost empty files, and delivery.c is almost empty also. IT should
all be unified into a single xapic.c file, and rename the genapic folder
to plain apic IMO. Not for you to do here, just realized while
looking at the changes.
> xen/arch/x86/genapic/delivery.c | 10 ----------
> xen/arch/x86/include/asm/genapic.h | 20 +-------------------
> 4 files changed, 15 insertions(+), 31 deletions(-)
>
> diff --git a/xen/arch/x86/genapic/bigsmp.c b/xen/arch/x86/genapic/bigsmp.c
> index e97e4453a033..b2e721845275 100644
> --- a/xen/arch/x86/genapic/bigsmp.c
> +++ b/xen/arch/x86/genapic/bigsmp.c
> @@ -46,5 +46,11 @@ static int __init cf_check probe_bigsmp(void)
>
> const struct genapic __initconst_cf_clobber apic_bigsmp = {
> APIC_INIT("bigsmp", probe_bigsmp),
> - GENAPIC_PHYS
> + .int_delivery_mode = dest_Fixed,
> + .int_dest_mode = 0, /* physical delivery */
> + .init_apic_ldr = init_apic_ldr_phys,
> + .vector_allocation_cpumask = vector_allocation_cpumask_phys,
> + .cpu_mask_to_apicid = cpu_mask_to_apicid_phys,
> + .send_IPI_mask = send_IPI_mask_phys,
> + .send_IPI_self = send_IPI_self_legacy
> };
> diff --git a/xen/arch/x86/genapic/default.c b/xen/arch/x86/genapic/default.c
> index a968836a1878..59c79afdb8fa 100644
> --- a/xen/arch/x86/genapic/default.c
> +++ b/xen/arch/x86/genapic/default.c
> @@ -16,5 +16,11 @@
> /* should be called last. */
> const struct genapic __initconst_cf_clobber apic_default = {
> APIC_INIT("default", NULL),
> - GENAPIC_FLAT
> + .int_delivery_mode = dest_Fixed,
> + .int_dest_mode = 0, /* physical delivery */
> + .init_apic_ldr = init_apic_ldr_flat,
> + .vector_allocation_cpumask = vector_allocation_cpumask_phys,
> + .cpu_mask_to_apicid = cpu_mask_to_apicid_phys,
After this change all APIC drivers use the same values for the
int_delivery_mode, int_dest_mode, vector_allocation_cpumask and
cpu_mask_to_apicid fields, at which point we can remove the hooks and
adjust the code. For example vector_allocation_cpumask_phys() should
be renamed to vector_allocation_cpumask(), and the
vector_allocation_cpumask removed.
Can be done in a followup commit.
As a small nit, it would be good if the remaining fields (used for IPI
sending): init_apic_ldr, send_IPI_{mask,self} are grouped together. I
would move the initialization of the init_apic_ldr field just above
send_IPI_mask.
Thanks, Roger.
© 2016 - 2024 Red Hat, Inc.