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

["Roger Pau Monné" <roger.pau@citrix.com>]

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.