| Message ID | 20250306145733.99927-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/pci: reduce PCI accesses | expand |
On 06/03/2025 2:57 pm, Roger Pau Monne wrote: > All local APIC drivers use physical destination and fixed delivery modes, > remove the fields from the genapic struct and simplify the logic. > > No functional change intended. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > xen/arch/x86/genapic/bigsmp.c | 2 -- > xen/arch/x86/genapic/default.c | 2 -- > xen/arch/x86/genapic/x2apic.c | 4 ---- > xen/arch/x86/include/asm/genapic.h | 5 ----- > xen/arch/x86/io_apic.c | 16 ++++++++-------- > xen/arch/x86/msi.c | 11 +++-------- > 6 files changed, 11 insertions(+), 29 deletions(-) Definitely going in a good direction. Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
On 06.03.2025 15:57, Roger Pau Monne wrote: > --- a/xen/arch/x86/genapic/bigsmp.c > +++ b/xen/arch/x86/genapic/bigsmp.c > @@ -46,8 +46,6 @@ static int __init cf_check probe_bigsmp(void) > > const struct genapic __initconst_cf_clobber apic_bigsmp = { > APIC_INIT("bigsmp", probe_bigsmp), > - .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, > --- a/xen/arch/x86/genapic/default.c > +++ b/xen/arch/x86/genapic/default.c > @@ -16,8 +16,6 @@ > /* should be called last. */ > const struct genapic __initconst_cf_clobber apic_default = { > APIC_INIT("default", NULL), > - .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, > --- a/xen/arch/x86/genapic/x2apic.c > +++ b/xen/arch/x86/genapic/x2apic.c > @@ -140,8 +140,6 @@ static void cf_check send_IPI_mask_x2apic_cluster( > > static const struct genapic __initconst_cf_clobber apic_x2apic_phys = { > APIC_INIT("x2apic_phys", NULL), > - .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, > @@ -163,8 +161,6 @@ static const struct genapic __initconst_cf_clobber apic_x2apic_mixed = { > * The following fields are exclusively used by external interrupts and > * hence are set to use Physical destination mode handlers. > */ > - .int_delivery_mode = dest_Fixed, > - .int_dest_mode = 0 /* physical delivery */, > .vector_allocation_cpumask = vector_allocation_cpumask_phys, > .cpu_mask_to_apicid = cpu_mask_to_apicid_phys, Like we had it everywhere above, ... > --- a/xen/arch/x86/io_apic.c > +++ b/xen/arch/x86/io_apic.c > @@ -1080,8 +1080,8 @@ static void __init setup_IO_APIC_irqs(void) > */ > memset(&entry,0,sizeof(entry)); > > - entry.delivery_mode = INT_DELIVERY_MODE; > - entry.dest_mode = INT_DEST_MODE; > + entry.delivery_mode = dest_Fixed; > + entry.dest_mode = 0; ... here and below these zeros would better gain a comment, or be expressed as e.g. (untested) MASK_EXTR(APIC_DEST_PHYSICAL, APIC_DEST_MASK). Jan
On Thu, Mar 06, 2025 at 04:33:37PM +0100, Jan Beulich wrote: > On 06.03.2025 15:57, Roger Pau Monne wrote: > > --- a/xen/arch/x86/genapic/bigsmp.c > > +++ b/xen/arch/x86/genapic/bigsmp.c > > @@ -46,8 +46,6 @@ static int __init cf_check probe_bigsmp(void) > > > > const struct genapic __initconst_cf_clobber apic_bigsmp = { > > APIC_INIT("bigsmp", probe_bigsmp), > > - .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, > > --- a/xen/arch/x86/genapic/default.c > > +++ b/xen/arch/x86/genapic/default.c > > @@ -16,8 +16,6 @@ > > /* should be called last. */ > > const struct genapic __initconst_cf_clobber apic_default = { > > APIC_INIT("default", NULL), > > - .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, > > --- a/xen/arch/x86/genapic/x2apic.c > > +++ b/xen/arch/x86/genapic/x2apic.c > > @@ -140,8 +140,6 @@ static void cf_check send_IPI_mask_x2apic_cluster( > > > > static const struct genapic __initconst_cf_clobber apic_x2apic_phys = { > > APIC_INIT("x2apic_phys", NULL), > > - .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, > > @@ -163,8 +161,6 @@ static const struct genapic __initconst_cf_clobber apic_x2apic_mixed = { > > * The following fields are exclusively used by external interrupts and > > * hence are set to use Physical destination mode handlers. > > */ > > - .int_delivery_mode = dest_Fixed, > > - .int_dest_mode = 0 /* physical delivery */, > > .vector_allocation_cpumask = vector_allocation_cpumask_phys, > > .cpu_mask_to_apicid = cpu_mask_to_apicid_phys, > > Like we had it everywhere above, ... > > > --- a/xen/arch/x86/io_apic.c > > +++ b/xen/arch/x86/io_apic.c > > @@ -1080,8 +1080,8 @@ static void __init setup_IO_APIC_irqs(void) > > */ > > memset(&entry,0,sizeof(entry)); > > > > - entry.delivery_mode = INT_DELIVERY_MODE; > > - entry.dest_mode = INT_DEST_MODE; > > + entry.delivery_mode = dest_Fixed; > > + entry.dest_mode = 0; > > ... here and below these zeros would better gain a comment, or be expressed > as e.g. (untested) MASK_EXTR(APIC_DEST_PHYSICAL, APIC_DEST_MASK). I've started adding those comments, but then I got the impression they where a bit redundant, many of the setting of the fields didn't have a matching comment. I was even tempted to just not setting the field at all, seeing as the structure is zeroed. Also this is the IO-APIC RTE, so it feels a bit out of place to use the local APIC defines? I will add a comment if you are fine with it. Thanks, Roger.
On 06.03.2025 16:54, Roger Pau Monné wrote: > On Thu, Mar 06, 2025 at 04:33:37PM +0100, Jan Beulich wrote: >> On 06.03.2025 15:57, Roger Pau Monne wrote: >>> --- a/xen/arch/x86/genapic/bigsmp.c >>> +++ b/xen/arch/x86/genapic/bigsmp.c >>> @@ -46,8 +46,6 @@ static int __init cf_check probe_bigsmp(void) >>> >>> const struct genapic __initconst_cf_clobber apic_bigsmp = { >>> APIC_INIT("bigsmp", probe_bigsmp), >>> - .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, >>> --- a/xen/arch/x86/genapic/default.c >>> +++ b/xen/arch/x86/genapic/default.c >>> @@ -16,8 +16,6 @@ >>> /* should be called last. */ >>> const struct genapic __initconst_cf_clobber apic_default = { >>> APIC_INIT("default", NULL), >>> - .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, >>> --- a/xen/arch/x86/genapic/x2apic.c >>> +++ b/xen/arch/x86/genapic/x2apic.c >>> @@ -140,8 +140,6 @@ static void cf_check send_IPI_mask_x2apic_cluster( >>> >>> static const struct genapic __initconst_cf_clobber apic_x2apic_phys = { >>> APIC_INIT("x2apic_phys", NULL), >>> - .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, >>> @@ -163,8 +161,6 @@ static const struct genapic __initconst_cf_clobber apic_x2apic_mixed = { >>> * The following fields are exclusively used by external interrupts and >>> * hence are set to use Physical destination mode handlers. >>> */ >>> - .int_delivery_mode = dest_Fixed, >>> - .int_dest_mode = 0 /* physical delivery */, >>> .vector_allocation_cpumask = vector_allocation_cpumask_phys, >>> .cpu_mask_to_apicid = cpu_mask_to_apicid_phys, >> >> Like we had it everywhere above, ... >> >>> --- a/xen/arch/x86/io_apic.c >>> +++ b/xen/arch/x86/io_apic.c >>> @@ -1080,8 +1080,8 @@ static void __init setup_IO_APIC_irqs(void) >>> */ >>> memset(&entry,0,sizeof(entry)); >>> >>> - entry.delivery_mode = INT_DELIVERY_MODE; >>> - entry.dest_mode = INT_DEST_MODE; >>> + entry.delivery_mode = dest_Fixed; >>> + entry.dest_mode = 0; >> >> ... here and below these zeros would better gain a comment, or be expressed >> as e.g. (untested) MASK_EXTR(APIC_DEST_PHYSICAL, APIC_DEST_MASK). > > I've started adding those comments, but then I got the impression they > where a bit redundant, many of the setting of the fields didn't have a > matching comment. I was even tempted to just not setting the field at > all, seeing as the structure is zeroed. > > Also this is the IO-APIC RTE, so it feels a bit out of place to use > the local APIC defines? Maybe. There is a certain level of correlation, but yes, it may end up being confusing. > I will add a comment if you are fine with it. I am. Jan
diff --git a/xen/arch/x86/genapic/bigsmp.c b/xen/arch/x86/genapic/bigsmp.c index b2e721845275..ddb3a0b5d727 100644 --- a/xen/arch/x86/genapic/bigsmp.c +++ b/xen/arch/x86/genapic/bigsmp.c @@ -46,8 +46,6 @@ static int __init cf_check probe_bigsmp(void) const struct genapic __initconst_cf_clobber apic_bigsmp = { APIC_INIT("bigsmp", probe_bigsmp), - .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, diff --git a/xen/arch/x86/genapic/default.c b/xen/arch/x86/genapic/default.c index 59c79afdb8fa..16e1875f6378 100644 --- a/xen/arch/x86/genapic/default.c +++ b/xen/arch/x86/genapic/default.c @@ -16,8 +16,6 @@ /* should be called last. */ const struct genapic __initconst_cf_clobber apic_default = { APIC_INIT("default", NULL), - .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, diff --git a/xen/arch/x86/genapic/x2apic.c b/xen/arch/x86/genapic/x2apic.c index c277f4f79b0a..74a6d808ac30 100644 --- a/xen/arch/x86/genapic/x2apic.c +++ b/xen/arch/x86/genapic/x2apic.c @@ -140,8 +140,6 @@ static void cf_check send_IPI_mask_x2apic_cluster( static const struct genapic __initconst_cf_clobber apic_x2apic_phys = { APIC_INIT("x2apic_phys", NULL), - .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, @@ -163,8 +161,6 @@ static const struct genapic __initconst_cf_clobber apic_x2apic_mixed = { * The following fields are exclusively used by external interrupts and * hence are set to use Physical destination mode handlers. */ - .int_delivery_mode = dest_Fixed, - .int_dest_mode = 0 /* physical delivery */, .vector_allocation_cpumask = vector_allocation_cpumask_phys, .cpu_mask_to_apicid = cpu_mask_to_apicid_phys, diff --git a/xen/arch/x86/include/asm/genapic.h b/xen/arch/x86/include/asm/genapic.h index cf36d48f3b07..04d3f1de7a1f 100644 --- a/xen/arch/x86/include/asm/genapic.h +++ b/xen/arch/x86/include/asm/genapic.h @@ -23,9 +23,6 @@ struct genapic { const char *name; int (*probe)(void); - /* Interrupt delivery parameters ('physical' vs. 'logical flat'). */ - int int_delivery_mode; - int int_dest_mode; void (*init_apic_ldr)(void); const cpumask_t *(*vector_allocation_cpumask)(int cpu); unsigned int (*cpu_mask_to_apicid)(const cpumask_t *cpumask); @@ -37,8 +34,6 @@ struct genapic { .name = aname, \ .probe = aprobe -#define INT_DELIVERY_MODE (genapic.int_delivery_mode) -#define INT_DEST_MODE (genapic.int_dest_mode) #define TARGET_CPUS ((const typeof(cpu_online_map) *)&cpu_online_map) #define init_apic_ldr() alternative_vcall(genapic.init_apic_ldr) #define cpu_mask_to_apicid(mask) ({ \ diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c index 776dd57720a2..60b39827c851 100644 --- a/xen/arch/x86/io_apic.c +++ b/xen/arch/x86/io_apic.c @@ -1080,8 +1080,8 @@ static void __init setup_IO_APIC_irqs(void) */ memset(&entry,0,sizeof(entry)); - entry.delivery_mode = INT_DELIVERY_MODE; - entry.dest_mode = INT_DEST_MODE; + entry.delivery_mode = dest_Fixed; + entry.dest_mode = 0; entry.mask = 0; /* enable IRQ */ idx = find_irq_entry(apic,pin,mp_INT); @@ -1150,10 +1150,10 @@ static void __init setup_ExtINT_IRQ0_pin(unsigned int apic, unsigned int pin, in * We use logical delivery to get the timer IRQ * to the first CPU. */ - entry.dest_mode = INT_DEST_MODE; + entry.dest_mode = 0; entry.mask = 0; /* unmask IRQ now */ SET_DEST(entry, logical, cpu_mask_to_apicid(TARGET_CPUS)); - entry.delivery_mode = INT_DELIVERY_MODE; + entry.delivery_mode = dest_Fixed; entry.polarity = 0; entry.trigger = 0; entry.vector = vector; @@ -2338,8 +2338,8 @@ int io_apic_set_pci_routing (int ioapic, int pin, int irq, int edge_level, int a memset(&entry,0,sizeof(entry)); - entry.delivery_mode = INT_DELIVERY_MODE; - entry.dest_mode = INT_DEST_MODE; + entry.delivery_mode = dest_Fixed; + entry.dest_mode = 0; entry.trigger = edge_level; entry.polarity = active_high_low; entry.mask = 1; @@ -2473,8 +2473,8 @@ int ioapic_guest_write(unsigned long physbase, unsigned int reg, u32 val) * The guest does not know physical APIC arrangement (flat vs. cluster). * Apply genapic conventions for this platform. */ - rte.delivery_mode = INT_DELIVERY_MODE; - rte.dest_mode = INT_DEST_MODE; + rte.delivery_mode = dest_Fixed; + rte.dest_mode = 0; irq = apic_pin_2_gsi_irq(apic, pin); if ( irq < 0 ) diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c index bf5b71822ea9..6c11d76015fb 100644 --- a/xen/arch/x86/msi.c +++ b/xen/arch/x86/msi.c @@ -174,18 +174,13 @@ void msi_compose_msg(unsigned vector, const cpumask_t *cpu_mask, struct msi_msg msg->address_hi = MSI_ADDR_BASE_HI; msg->address_lo = MSI_ADDR_BASE_LO | - (INT_DEST_MODE ? MSI_ADDR_DESTMODE_LOGIC - : MSI_ADDR_DESTMODE_PHYS) | - ((INT_DELIVERY_MODE != dest_LowestPrio) - ? MSI_ADDR_REDIRECTION_CPU - : MSI_ADDR_REDIRECTION_LOWPRI) | + MSI_ADDR_DESTMODE_PHYS | + MSI_ADDR_REDIRECTION_CPU | MSI_ADDR_DEST_ID(msg->dest32); msg->data = MSI_DATA_TRIGGER_EDGE | MSI_DATA_LEVEL_ASSERT | - ((INT_DELIVERY_MODE != dest_LowestPrio) - ? MSI_DATA_DELIVERY_FIXED - : MSI_DATA_DELIVERY_LOWPRI) | + MSI_DATA_DELIVERY_FIXED | MSI_DATA_VECTOR(vector); }
All local APIC drivers use physical destination and fixed delivery modes, remove the fields from the genapic struct and simplify the logic. No functional change intended. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/genapic/bigsmp.c | 2 -- xen/arch/x86/genapic/default.c | 2 -- xen/arch/x86/genapic/x2apic.c | 4 ---- xen/arch/x86/include/asm/genapic.h | 5 ----- xen/arch/x86/io_apic.c | 16 ++++++++-------- xen/arch/x86/msi.c | 11 +++-------- 6 files changed, 11 insertions(+), 29 deletions(-)