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.bar...@cloud.com>
Reviewed-by: Roger Pau Monné <roger....@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.
