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(-)
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);
}
--
2.48.1
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
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>
© 2016 - 2025 Red Hat, Inc.