* [PATCH 0/4] xen/arch: Treewide improvement of irq helper APIs
@ 2018-11-23 16:52 Andrew Cooper
2018-11-23 16:52 ` [PATCH 1/4] xen/arch: Switch local_*_is_enabled() predicates to return bool Andrew Cooper
` (4 more replies)
0 siblings, 5 replies; 12+ messages in thread
From: Andrew Cooper @ 2018-11-23 16:52 UTC (permalink / raw)
To: Xen-devel
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall,
Jan Beulich, Roger Pau Monné
This is a rather old cleanup series which I never got around to finishing. It
is restricted to the arch maintainers as it is purely mechanical outside of
system.h
The main purpose is to reduce the quantity of code we have which mutates
parameters by name.
One ARM point which I've noticed is that system.h is a mix of duplicated
common code in the arch{32,64}/ versions, and ifdefary in the common version.
Does this perhaps want to be made consistent? I've not touched the layout in
this series.
Andrew Cooper (4):
xen/arch: Switch local_*_is_enabled() predicates to return bool
xen/arch: Switch local_save_flags() to being a static inline helper
xen/arch: Switch local_irq_save() to being a static inline helper
xen/arch: Switch local_irq_restore() to being a static inline helper
xen/arch/arm/mm.c | 4 +--
xen/arch/arm/p2m.c | 2 +-
xen/arch/x86/acpi/power.c | 2 +-
xen/arch/x86/apic.c | 10 +++----
xen/arch/x86/cpu/mtrr/generic.c | 4 +--
xen/arch/x86/cpu/mtrr/main.c | 6 ++--
xen/arch/x86/domain.c | 2 +-
xen/arch/x86/domain_page.c | 4 +--
xen/arch/x86/flushtlb.c | 4 +--
xen/arch/x86/genapic/x2apic.c | 4 +--
xen/arch/x86/hpet.c | 3 +-
xen/arch/x86/hvm/vmx/vmcs.c | 6 ++--
xen/arch/x86/io_apic.c | 4 +--
xen/arch/x86/nmi.c | 3 +-
xen/arch/x86/platform_hypercall.c | 2 +-
xen/arch/x86/smp.c | 4 +--
xen/arch/x86/time.c | 5 ++--
xen/arch/x86/traps.c | 2 +-
xen/common/cpupool.c | 2 +-
xen/common/gdbstub.c | 6 ++--
xen/common/livepatch.c | 4 +--
xen/common/rcupdate.c | 2 +-
xen/common/spinlock.c | 3 +-
xen/common/timer.c | 2 +-
xen/drivers/char/console.c | 5 ++--
xen/drivers/char/serial.c | 4 +--
xen/drivers/passthrough/io.c | 4 +--
xen/include/asm-arm/arm32/system.h | 56 +++++++++++++++++-----------------
xen/include/asm-arm/arm64/system.h | 61 ++++++++++++++++++--------------------
xen/include/asm-arm/system.h | 6 ++--
xen/include/asm-x86/system.h | 48 +++++++++++++++---------------
xen/include/xen/rwlock.h | 7 ++---
xen/include/xen/spinlock.h | 2 +-
33 files changed, 133 insertions(+), 150 deletions(-)
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/4] xen/arch: Switch local_*_is_enabled() predicates to return bool
2018-11-23 16:52 [PATCH 0/4] xen/arch: Treewide improvement of irq helper APIs Andrew Cooper
@ 2018-11-23 16:52 ` Andrew Cooper
2018-11-26 13:04 ` Jan Beulich
2018-11-27 9:27 ` Julien Grall
2018-11-23 16:52 ` [PATCH 2/4] xen/arch: Switch local_save_flags() to being a static inline helper Andrew Cooper
` (3 subsequent siblings)
4 siblings, 2 replies; 12+ messages in thread
From: Andrew Cooper @ 2018-11-23 16:52 UTC (permalink / raw)
To: Xen-devel
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall,
Jan Beulich, Roger Pau Monné
No functional change, as the value returned was previously always 0 or 1.
While altering these, insert blank lines where appropriate and drop the
now-redundant !! from x86's local_irq_is_enabled().
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
xen/include/asm-arm/arm32/system.h | 8 ++++++--
xen/include/asm-arm/arm64/system.h | 8 ++++++--
xen/include/asm-arm/system.h | 4 +++-
xen/include/asm-x86/system.h | 6 ++++--
4 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/xen/include/asm-arm/arm32/system.h b/xen/include/asm-arm/arm32/system.h
index ab57abf..58c8fb3 100644
--- a/xen/include/asm-arm/arm32/system.h
+++ b/xen/include/asm-arm/arm32/system.h
@@ -28,10 +28,12 @@
: "memory", "cc"); \
})
-static inline int local_irq_is_enabled(void)
+static inline bool local_irq_is_enabled(void)
{
unsigned long flags;
+
local_save_flags(flags);
+
return !(flags & PSR_IRQ_MASK);
}
@@ -41,10 +43,12 @@ static inline int local_irq_is_enabled(void)
#define local_abort_enable() __asm__("cpsie a @ __sta\n" : : : "memory", "cc")
#define local_abort_disable() __asm__("cpsid a @ __sta\n" : : : "memory", "cc")
-static inline int local_fiq_is_enabled(void)
+static inline bool local_fiq_is_enabled(void)
{
unsigned long flags;
+
local_save_flags(flags);
+
return !(flags & PSR_FIQ_MASK);
}
diff --git a/xen/include/asm-arm/arm64/system.h b/xen/include/asm-arm/arm64/system.h
index 2e36573..d17fc9d 100644
--- a/xen/include/asm-arm/arm64/system.h
+++ b/xen/include/asm-arm/arm64/system.h
@@ -44,17 +44,21 @@
: "memory"); \
})
-static inline int local_irq_is_enabled(void)
+static inline bool local_irq_is_enabled(void)
{
unsigned long flags;
+
local_save_flags(flags);
+
return !(flags & PSR_IRQ_MASK);
}
-static inline int local_fiq_is_enabled(void)
+static inline bool local_fiq_is_enabled(void)
{
unsigned long flags;
+
local_save_flags(flags);
+
return !(flags & PSR_FIQ_MASK);
}
diff --git a/xen/include/asm-arm/system.h b/xen/include/asm-arm/system.h
index b94e56f..bc51300 100644
--- a/xen/include/asm-arm/system.h
+++ b/xen/include/asm-arm/system.h
@@ -51,10 +51,12 @@
# error "unknown ARM variant"
#endif
-static inline int local_abort_is_enabled(void)
+static inline bool local_abort_is_enabled(void)
{
unsigned long flags;
+
local_save_flags(flags);
+
return !(flags & PSR_ABT_MASK);
}
diff --git a/xen/include/asm-x86/system.h b/xen/include/asm-x86/system.h
index 483cd20..4b7056d 100644
--- a/xen/include/asm-x86/system.h
+++ b/xen/include/asm-x86/system.h
@@ -274,11 +274,13 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
"ri" ( (x) & X86_EFLAGS_IF ) ); \
})
-static inline int local_irq_is_enabled(void)
+static inline bool local_irq_is_enabled(void)
{
unsigned long flags;
+
local_save_flags(flags);
- return !!(flags & X86_EFLAGS_IF);
+
+ return flags & X86_EFLAGS_IF;
}
#define BROKEN_ACPI_Sx 0x0001
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/4] xen/arch: Switch local_save_flags() to being a static inline helper
2018-11-23 16:52 [PATCH 0/4] xen/arch: Treewide improvement of irq helper APIs Andrew Cooper
2018-11-23 16:52 ` [PATCH 1/4] xen/arch: Switch local_*_is_enabled() predicates to return bool Andrew Cooper
@ 2018-11-23 16:52 ` Andrew Cooper
2018-11-26 13:15 ` Jan Beulich
2018-11-27 9:31 ` Julien Grall
2018-11-23 16:52 ` [PATCH 3/4] xen/arch: Switch local_irq_save() " Andrew Cooper
` (2 subsequent siblings)
4 siblings, 2 replies; 12+ messages in thread
From: Andrew Cooper @ 2018-11-23 16:52 UTC (permalink / raw)
To: Xen-devel
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall,
Jan Beulich, Roger Pau Monné
... rather than a macro which writes to its parameter by name. A consequence
of this change is that the local variables in local_*_is_enabled() can be
dropped.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
xen/arch/x86/io_apic.c | 2 +-
xen/include/asm-arm/arm32/system.h | 30 +++++++++++++-----------------
xen/include/asm-arm/arm64/system.h | 32 ++++++++++++--------------------
xen/include/asm-arm/system.h | 6 +-----
xen/include/asm-x86/system.h | 22 +++++++++++-----------
5 files changed, 38 insertions(+), 54 deletions(-)
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index daa5e9e..aca4f63 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -1486,7 +1486,7 @@ static int __init timer_irq_works(void)
t1 = ACCESS_ONCE(pit0_ticks);
- local_save_flags(flags);
+ flags = local_save_flags();
local_irq_enable();
/* Let ten ticks pass... */
mdelay((10 * 1000) / HZ);
diff --git a/xen/include/asm-arm/arm32/system.h b/xen/include/asm-arm/arm32/system.h
index 58c8fb3..cbfa91d 100644
--- a/xen/include/asm-arm/arm32/system.h
+++ b/xen/include/asm-arm/arm32/system.h
@@ -7,15 +7,19 @@
#define local_irq_disable() asm volatile ( "cpsid i @ local_irq_disable\n" : : : "cc" )
#define local_irq_enable() asm volatile ( "cpsie i @ local_irq_enable\n" : : : "cc" )
-#define local_save_flags(x) \
-({ \
- BUILD_BUG_ON(sizeof(x) != sizeof(long)); \
- asm volatile ( "mrs %0, cpsr @ local_save_flags\n" \
- : "=r" (x) :: "memory", "cc" ); \
-})
+static inline unsigned long local_save_flags(void)
+{
+ unsigned long flags;
+
+ asm volatile ( "mrs %0, cpsr @ local_SAVE_flags\n"
+ : "=r" (flags) :: "memory", "cc" );
+
+ return flags;
+}
+
#define local_irq_save(x) \
({ \
- local_save_flags(x); \
+ x = local_save_flags(); \
local_irq_disable(); \
})
#define local_irq_restore(x) \
@@ -30,11 +34,7 @@
static inline bool local_irq_is_enabled(void)
{
- unsigned long flags;
-
- local_save_flags(flags);
-
- return !(flags & PSR_IRQ_MASK);
+ return !(local_save_flags() & PSR_IRQ_MASK);
}
#define local_fiq_enable() __asm__("cpsie f @ __stf\n" : : : "memory", "cc")
@@ -45,11 +45,7 @@ static inline bool local_irq_is_enabled(void)
static inline bool local_fiq_is_enabled(void)
{
- unsigned long flags;
-
- local_save_flags(flags);
-
- return !(flags & PSR_FIQ_MASK);
+ return !(local_save_flags() & PSR_FIQ_MASK);
}
#define CSDB ".inst 0xe320f014"
diff --git a/xen/include/asm-arm/arm64/system.h b/xen/include/asm-arm/arm64/system.h
index d17fc9d..89c5b64 100644
--- a/xen/include/asm-arm/arm64/system.h
+++ b/xen/include/asm-arm/arm64/system.h
@@ -19,19 +19,19 @@
#define local_abort_disable() asm volatile ( "msr daifset, #4\n" ::: "memory" )
#define local_abort_enable() asm volatile ( "msr daifclr, #4\n" ::: "memory" )
-#define local_save_flags(x) \
-({ \
- BUILD_BUG_ON(sizeof(x) != sizeof(long)); \
- asm volatile( \
- "mrs %0, daif // local_save_flags\n" \
- : "=r" (x) \
- : \
- : "memory"); \
-})
+static inline unsigned long local_save_flags(void)
+{
+ unsigned long flags;
+
+ asm volatile ( "mrs %0, daif // local_SAVE_flags\n"
+ : "=r" (flags) :: "memory");
+
+ return flags;
+}
#define local_irq_save(x) \
({ \
- local_save_flags(x); \
+ x = local_save_flags(); \
local_irq_disable(); \
})
#define local_irq_restore(x) \
@@ -46,20 +46,12 @@
static inline bool local_irq_is_enabled(void)
{
- unsigned long flags;
-
- local_save_flags(flags);
-
- return !(flags & PSR_IRQ_MASK);
+ return !(local_save_flags() & PSR_IRQ_MASK);
}
static inline bool local_fiq_is_enabled(void)
{
- unsigned long flags;
-
- local_save_flags(flags);
-
- return !(flags & PSR_FIQ_MASK);
+ return !(local_save_flags() & PSR_FIQ_MASK);
}
#define csdb() asm volatile ( "hint #20" : : : "memory" )
diff --git a/xen/include/asm-arm/system.h b/xen/include/asm-arm/system.h
index bc51300..6566528 100644
--- a/xen/include/asm-arm/system.h
+++ b/xen/include/asm-arm/system.h
@@ -53,11 +53,7 @@
static inline bool local_abort_is_enabled(void)
{
- unsigned long flags;
-
- local_save_flags(flags);
-
- return !(flags & PSR_ABT_MASK);
+ return !(local_save_flags() & PSR_ABT_MASK);
}
#define arch_fetch_and_add(x, v) __sync_fetch_and_add(x, v)
diff --git a/xen/include/asm-x86/system.h b/xen/include/asm-x86/system.h
index 4b7056d..faf2efe 100644
--- a/xen/include/asm-x86/system.h
+++ b/xen/include/asm-x86/system.h
@@ -253,14 +253,18 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
/* used when interrupts are already enabled or to shutdown the processor */
#define halt() asm volatile ( "hlt" : : : "memory" )
-#define local_save_flags(x) \
-({ \
- BUILD_BUG_ON(sizeof(x) != sizeof(long)); \
- asm volatile ( "pushf" __OS " ; pop" __OS " %0" : "=g" (x)); \
-})
+static inline unsigned long local_save_flags(void)
+{
+ unsigned long flags;
+
+ asm volatile ( "pushf; pop %0;" : "=g" (flags) );
+
+ return flags;
+}
+
#define local_irq_save(x) \
({ \
- local_save_flags(x); \
+ x = local_save_flags(); \
local_irq_disable(); \
})
#define local_irq_restore(x) \
@@ -276,11 +280,7 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
static inline bool local_irq_is_enabled(void)
{
- unsigned long flags;
-
- local_save_flags(flags);
-
- return flags & X86_EFLAGS_IF;
+ return local_save_flags() & X86_EFLAGS_IF;
}
#define BROKEN_ACPI_Sx 0x0001
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/4] xen/arch: Switch local_irq_save() to being a static inline helper
2018-11-23 16:52 [PATCH 0/4] xen/arch: Treewide improvement of irq helper APIs Andrew Cooper
2018-11-23 16:52 ` [PATCH 1/4] xen/arch: Switch local_*_is_enabled() predicates to return bool Andrew Cooper
2018-11-23 16:52 ` [PATCH 2/4] xen/arch: Switch local_save_flags() to being a static inline helper Andrew Cooper
@ 2018-11-23 16:52 ` Andrew Cooper
2018-11-26 13:22 ` Jan Beulich
2018-11-23 16:52 ` [PATCH 4/4] xen/arch: Switch local_irq_restore() " Andrew Cooper
2018-11-23 19:02 ` [PATCH 0/4] xen/arch: Treewide improvement of irq helper APIs Julien Grall
4 siblings, 1 reply; 12+ messages in thread
From: Andrew Cooper @ 2018-11-23 16:52 UTC (permalink / raw)
To: Xen-devel
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall,
Jan Beulich, Roger Pau Monné
... rather than a macro which writes to its parameter by name. Take the
opportunity to fold the assignment into the flags declaraion where
appropriate.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
xen/arch/arm/mm.c | 4 ++--
xen/arch/arm/p2m.c | 2 +-
xen/arch/x86/acpi/power.c | 2 +-
xen/arch/x86/apic.c | 10 +++++-----
xen/arch/x86/cpu/mtrr/generic.c | 4 ++--
xen/arch/x86/cpu/mtrr/main.c | 6 ++----
xen/arch/x86/domain.c | 2 +-
xen/arch/x86/domain_page.c | 4 ++--
xen/arch/x86/flushtlb.c | 4 ++--
xen/arch/x86/genapic/x2apic.c | 4 ++--
xen/arch/x86/hpet.c | 3 +--
xen/arch/x86/hvm/vmx/vmcs.c | 6 ++----
xen/arch/x86/io_apic.c | 2 +-
xen/arch/x86/nmi.c | 3 ++-
xen/arch/x86/platform_hypercall.c | 2 +-
xen/arch/x86/smp.c | 4 ++--
xen/arch/x86/time.c | 5 ++---
xen/arch/x86/traps.c | 2 +-
xen/common/cpupool.c | 2 +-
xen/common/gdbstub.c | 6 ++----
xen/common/livepatch.c | 4 ++--
xen/common/rcupdate.c | 2 +-
xen/common/spinlock.c | 3 +--
xen/common/timer.c | 2 +-
xen/drivers/char/console.c | 5 ++---
xen/drivers/char/serial.c | 4 +---
xen/drivers/passthrough/io.c | 4 ++--
xen/include/asm-arm/arm32/system.h | 14 +++++++++-----
xen/include/asm-arm/arm64/system.h | 14 +++++++++-----
xen/include/asm-x86/system.h | 14 +++++++++-----
xen/include/xen/rwlock.h | 7 +++----
xen/include/xen/spinlock.h | 2 +-
32 files changed, 76 insertions(+), 76 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 987fcb9..9b1da81 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -401,7 +401,7 @@ void *map_domain_page(mfn_t mfn)
lpae_t pte;
int i, slot;
- local_irq_save(flags);
+ flags = local_irq_save();
/* The map is laid out as an open-addressed hash table where each
* entry is a 2MB superpage pte. We use the available bits of each
@@ -467,7 +467,7 @@ void unmap_domain_page(const void *va)
lpae_t *map = this_cpu(xen_dommap);
int slot = ((unsigned long) va - DOMHEAP_VIRT_START) >> SECOND_SHIFT;
- local_irq_save(flags);
+ flags = local_irq_save();
ASSERT(slot >= 0 && slot < DOMHEAP_ENTRIES);
ASSERT(map[slot].pt.avail != 0);
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 6c76298..81a98ef 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -146,7 +146,7 @@ static void p2m_force_tlb_flush_sync(struct p2m_domain *p2m)
ovttbr = READ_SYSREG64(VTTBR_EL2);
if ( ovttbr != p2m->vttbr )
{
- local_irq_save(flags);
+ flags = local_irq_save();
WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2);
isb();
}
diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index 93e967f..7d64904 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -197,7 +197,7 @@ static int enter_state(u32 state)
console_start_sync();
printk("Entering ACPI S%d state.\n", state);
- local_irq_save(flags);
+ flags = local_irq_save();
spin_debug_disable();
if ( (error = device_power_down()) != SAVED_ALL )
diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index 7120107..031b3ee 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -718,7 +718,7 @@ int lapic_suspend(void)
if (maxlvt >= 5)
apic_pm_state.apic_thmr = apic_read(APIC_LVTTHMR);
- local_irq_save(flags);
+ flags = local_irq_save();
disable_local_APIC();
iommu_disable_x2apic_IR();
local_irq_restore(flags);
@@ -734,7 +734,7 @@ int lapic_resume(void)
if (!apic_pm_state.active)
return 0;
- local_irq_save(flags);
+ flags = local_irq_save();
/*
* Make sure the APICBASE points to the right address
@@ -1094,8 +1094,8 @@ static void __setup_APIC_LVTT(unsigned int clocks)
static void setup_APIC_timer(void)
{
- unsigned long flags;
- local_irq_save(flags);
+ unsigned long flags = local_irq_save();
+
__setup_APIC_LVTT(0);
local_irq_restore(flags);
}
@@ -1302,7 +1302,7 @@ void __init setup_boot_APIC_clock(void)
check_deadline_errata();
- local_irq_save(flags);
+ flags = local_irq_save();
calibrate_APIC_clock();
diff --git a/xen/arch/x86/cpu/mtrr/generic.c b/xen/arch/x86/cpu/mtrr/generic.c
index 8f9cf1b..3612b8a 100644
--- a/xen/arch/x86/cpu/mtrr/generic.c
+++ b/xen/arch/x86/cpu/mtrr/generic.c
@@ -493,7 +493,7 @@ static void generic_set_all(void)
unsigned long flags;
bool pge;
- local_irq_save(flags);
+ flags = local_irq_save();
pge = prepare_set();
/* Actually set the state */
@@ -528,7 +528,7 @@ static void generic_set_mtrr(unsigned int reg, unsigned long base,
vr = &mtrr_state.var_ranges[reg];
- local_irq_save(flags);
+ flags = local_irq_save();
pge = prepare_set();
if (size == 0) {
diff --git a/xen/arch/x86/cpu/mtrr/main.c b/xen/arch/x86/cpu/mtrr/main.c
index e9df53f..c0780e2 100644
--- a/xen/arch/x86/cpu/mtrr/main.c
+++ b/xen/arch/x86/cpu/mtrr/main.c
@@ -137,9 +137,7 @@ static void ipi_handler(void *info)
*/
{
struct set_mtrr_data *data = info;
- unsigned long flags;
-
- local_irq_save(flags);
+ unsigned long flags = local_irq_save();
atomic_dec(&data->count);
while(!atomic_read(&data->gate))
@@ -230,7 +228,7 @@ static void set_mtrr(unsigned int reg, unsigned long base,
/* Start the ball rolling on other CPUs */
on_selected_cpus(&allbutself, ipi_handler, &data, 0);
- local_irq_save(flags);
+ flags = local_irq_save();
while (atomic_read(&data.count))
cpu_relax();
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index b4d5948..c73d640 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1866,7 +1866,7 @@ int __sync_local_execstate(void)
unsigned long flags;
int switch_required;
- local_irq_save(flags);
+ flags = local_irq_save();
switch_required = (this_cpu(curr_vcpu) != current);
diff --git a/xen/arch/x86/domain_page.c b/xen/arch/x86/domain_page.c
index 4a07cfb..0128577 100644
--- a/xen/arch/x86/domain_page.c
+++ b/xen/arch/x86/domain_page.c
@@ -92,7 +92,7 @@ void *map_domain_page(mfn_t mfn)
perfc_incr(map_domain_page_count);
- local_irq_save(flags);
+ flags = local_irq_save();
hashent = &vcache->hash[MAPHASH_HASHFN(mfn_x(mfn))];
if ( hashent->mfn == mfn_x(mfn) )
@@ -196,7 +196,7 @@ void unmap_domain_page(const void *ptr)
mfn = l1e_get_pfn(MAPCACHE_L1ENT(idx));
hashent = &v->arch.pv.mapcache.hash[MAPHASH_HASHFN(mfn)];
- local_irq_save(flags);
+ flags = local_irq_save();
if ( hashent->idx == idx )
{
diff --git a/xen/arch/x86/flushtlb.c b/xen/arch/x86/flushtlb.c
index ed0504c..b71a0be 100644
--- a/xen/arch/x86/flushtlb.c
+++ b/xen/arch/x86/flushtlb.c
@@ -82,7 +82,7 @@ static void do_tlb_flush(void)
u32 t;
/* This non-reentrant function is sometimes called in interrupt context. */
- local_irq_save(flags);
+ flags = local_irq_save();
t = pre_flush();
@@ -108,7 +108,7 @@ void switch_cr3_cr4(unsigned long cr3, unsigned long cr4)
unsigned long old_pcid = cr3_pcid(read_cr3());
/* This non-reentrant function is sometimes called in interrupt context. */
- local_irq_save(flags);
+ flags = local_irq_save();
t = pre_flush();
diff --git a/xen/arch/x86/genapic/x2apic.c b/xen/arch/x86/genapic/x2apic.c
index 7e2e89d..83ff5d9 100644
--- a/xen/arch/x86/genapic/x2apic.c
+++ b/xen/arch/x86/genapic/x2apic.c
@@ -113,7 +113,7 @@ static void send_IPI_mask_x2apic_phys(const cpumask_t *cpumask, int vector)
*/
smp_mb();
- local_irq_save(flags);
+ flags = local_irq_save();
for_each_cpu ( cpu, cpumask )
{
@@ -137,7 +137,7 @@ static void send_IPI_mask_x2apic_cluster(const cpumask_t *cpumask, int vector)
smp_mb(); /* See above for an explanation. */
- local_irq_save(flags);
+ flags = local_irq_save();
cpumask_andnot(ipimask, &cpu_online_map, cpumask_of(cpu));
diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 4b08488..06a62ff 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -97,9 +97,8 @@ static inline unsigned long ns2ticks(unsigned long nsec, int shift,
static int hpet_next_event(unsigned long delta, int timer)
{
uint32_t cnt, cmp;
- unsigned long flags;
+ unsigned long flags = local_irq_save();
- local_irq_save(flags);
cnt = hpet_read32(HPET_COUNTER);
cmp = cnt + delta;
hpet_write32(cmp, HPET_Tn_CMP(timer));
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index dec21d1..f13b1d9 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -549,9 +549,7 @@ static void vmx_clear_vmcs(struct vcpu *v)
static void vmx_load_vmcs(struct vcpu *v)
{
- unsigned long flags;
-
- local_irq_save(flags);
+ unsigned long flags = local_irq_save();
if ( v->arch.hvm.vmx.active_cpu == -1 )
{
@@ -713,7 +711,7 @@ void vmx_cpu_down(void)
if ( !this_cpu(vmxon) )
return;
- local_irq_save(flags);
+ flags = local_irq_save();
while ( !list_empty(active_vmcs_list) )
__vmx_clear_vmcs(list_entry(active_vmcs_list->next,
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index aca4f63..47b1649 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -1883,7 +1883,7 @@ static void __init check_timer(void)
unsigned long flags;
cpumask_t mask_all;
- local_irq_save(flags);
+ flags = local_irq_save();
/*
* get/set the timer IRQ vector:
diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
index d7fce28..3e94311 100644
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -560,7 +560,8 @@ void self_nmi(void)
{
unsigned long flags;
u32 id = get_apic_id();
- local_irq_save(flags);
+
+ flags = local_irq_save();
apic_wait_icr_idle();
apic_icr_write(APIC_DM_NMI | APIC_DEST_PHYSICAL, id);
local_irq_restore(flags);
diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index b19f6ec..949fc69 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -147,7 +147,7 @@ void resource_access(void *info)
unlikely(entry[1].idx == MSR_IA32_TSC);
if ( unlikely(read_tsc) )
- local_irq_save(flags);
+ flags = local_irq_save();
ret = rdmsr_safe(entry->idx, entry->val);
diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c
index b15d4f0..e324882 100644
--- a/xen/arch/x86/smp.c
+++ b/xen/arch/x86/smp.c
@@ -134,7 +134,7 @@ void send_IPI_mask_flat(const cpumask_t *cpumask, int vector)
if ( mask == 0 )
return;
- local_irq_save(flags);
+ flags = local_irq_save();
/*
* Wait for idle.
@@ -165,7 +165,7 @@ void send_IPI_mask_phys(const cpumask_t *mask, int vector)
unsigned long cfg, flags;
unsigned int query_cpu;
- local_irq_save(flags);
+ flags = local_irq_save();
for_each_cpu ( query_cpu, mask )
{
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 24d4c27..f78d0fb 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1585,9 +1585,8 @@ static struct cpu_time_stamp ap_bringup_ref;
void time_latch_stamps(void)
{
- unsigned long flags;
+ unsigned long flags = local_irq_save();
- local_irq_save(flags);
ap_bringup_ref.master_stime = read_platform_stime(NULL);
ap_bringup_ref.local_tsc = rdtsc_ordered();
local_irq_restore(flags);
@@ -1655,7 +1654,7 @@ void init_percpu_time(void)
}
}
- local_irq_save(flags);
+ flags = local_irq_save();
now = read_platform_stime(NULL);
tsc = rdtsc_ordered();
local_irq_restore(flags);
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 9471d89..4a9ab51 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1315,7 +1315,7 @@ static enum pf_type spurious_page_fault(unsigned long addr,
* Disabling interrupts prevents TLB flushing, and hence prevents
* page tables from becoming invalid under our feet during the walk.
*/
- local_irq_save(flags);
+ flags = local_irq_save();
pf_type = __page_fault_type(addr, regs);
local_irq_restore(flags);
diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index e89bb67..995934d 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -739,7 +739,7 @@ void dump_runq(unsigned char key)
struct cpupool **c;
spin_lock(&cpupool_lock);
- local_irq_save(flags);
+ flags = local_irq_save();
printk("sched_smt_power_savings: %s\n",
sched_smt_power_savings? "enabled":"disabled");
diff --git a/xen/common/gdbstub.c b/xen/common/gdbstub.c
index 07095e1..0d60f57 100644
--- a/xen/common/gdbstub.c
+++ b/xen/common/gdbstub.c
@@ -603,7 +603,7 @@ __trap_to_gdb(struct cpu_user_regs *regs, unsigned long cookie)
gdb_smp_pause();
- local_irq_save(flags);
+ flags = local_irq_save();
watchdog_disable();
console_start_sync();
@@ -661,9 +661,7 @@ presmp_initcall(initialise_gdb);
static void gdb_pause_this_cpu(void *unused)
{
- unsigned long flags;
-
- local_irq_save(flags);
+ unsigned long flags = local_irq_save();
atomic_set(&gdb_cpu[smp_processor_id()].ack, 1);
atomic_inc(&gdb_smp_paused_count);
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index d6eaae6..1ad83e0 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1357,7 +1357,7 @@ void check_for_livepatch_work(void)
if ( !livepatch_spin(&livepatch_work.semaphore, timeout, cpus, "IRQ") )
{
- local_irq_save(flags);
+ flags = local_irq_save();
/* Do the patching. */
livepatch_do_action();
/* Serialize and flush out the CPU via CPUID instruction (on x86). */
@@ -1384,7 +1384,7 @@ void check_for_livepatch_work(void)
cpu_relax();
/* Disable IRQs and signal. */
- local_irq_save(flags);
+ flags = local_irq_save();
/*
* We re-use the sempahore, so MUST have it reset by master before
* we exit the loop above.
diff --git a/xen/common/rcupdate.c b/xen/common/rcupdate.c
index 3517790..1f8fe46 100644
--- a/xen/common/rcupdate.c
+++ b/xen/common/rcupdate.c
@@ -224,7 +224,7 @@ void call_rcu(struct rcu_head *head,
head->func = func;
head->next = NULL;
- local_irq_save(flags);
+ flags = local_irq_save();
rdp = &__get_cpu_var(rcu_data);
*rdp->nxttail = head;
rdp->nxttail = &head->next;
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 6bc52d7..d87cedb 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -169,9 +169,8 @@ void _spin_lock_irq(spinlock_t *lock)
unsigned long _spin_lock_irqsave(spinlock_t *lock)
{
- unsigned long flags;
+ unsigned long flags = local_irq_save();
- local_irq_save(flags);
_spin_lock(lock);
return flags;
}
diff --git a/xen/common/timer.c b/xen/common/timer.c
index 376581b..fc1b2d6 100644
--- a/xen/common/timer.c
+++ b/xen/common/timer.c
@@ -256,7 +256,7 @@ static inline bool_t timer_lock(struct timer *timer)
#define timer_lock_irqsave(t, flags) ({ \
bool_t __x; \
- local_irq_save(flags); \
+ flags = local_irq_save(); \
if ( !(__x = timer_lock(t)) ) \
local_irq_restore(flags); \
__x; \
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 675193a..89c4bb6 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -817,7 +817,7 @@ static void vprintk_common(const char *prefix, const char *fmt, va_list args)
unsigned long flags;
/* console_lock can be acquired recursively from __printk_ratelimit(). */
- local_irq_save(flags);
+ flags = local_irq_save();
spin_lock_recursive(&console_lock);
state = &this_cpu(state);
@@ -1050,9 +1050,8 @@ void console_end_log_everything(void)
unsigned long console_lock_recursive_irqsave(void)
{
- unsigned long flags;
+ unsigned long flags = local_irq_save();
- local_irq_save(flags);
spin_lock_recursive(&console_lock);
return flags;
diff --git a/xen/drivers/char/serial.c b/xen/drivers/char/serial.c
index 09a20ac..0795602 100644
--- a/xen/drivers/char/serial.c
+++ b/xen/drivers/char/serial.c
@@ -71,9 +71,7 @@ void serial_rx_interrupt(struct serial_port *port, struct cpu_user_regs *regs)
void serial_tx_interrupt(struct serial_port *port, struct cpu_user_regs *regs)
{
int i, n;
- unsigned long flags;
-
- local_irq_save(flags);
+ unsigned long flags = local_irq_save();
/*
* Avoid spinning for a long time: if there is a long-term lock holder
diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
index a6eb8a4..a7c4167 100644
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -67,7 +67,7 @@ static void raise_softirq_for(struct hvm_pirq_dpci *pirq_dpci)
get_knownalive_domain(pirq_dpci->dom);
- local_irq_save(flags);
+ flags = local_irq_save();
list_add_tail(&pirq_dpci->softirq_list, &this_cpu(dpci_list));
local_irq_restore(flags);
@@ -1057,7 +1057,7 @@ static void dpci_softirq(void)
unsigned long flags;
/* Put back on the list and retry. */
- local_irq_save(flags);
+ flags = local_irq_save();
list_add_tail(&pirq_dpci->softirq_list, &this_cpu(dpci_list));
local_irq_restore(flags);
diff --git a/xen/include/asm-arm/arm32/system.h b/xen/include/asm-arm/arm32/system.h
index cbfa91d..f7c8e53 100644
--- a/xen/include/asm-arm/arm32/system.h
+++ b/xen/include/asm-arm/arm32/system.h
@@ -17,11 +17,15 @@ static inline unsigned long local_save_flags(void)
return flags;
}
-#define local_irq_save(x) \
-({ \
- x = local_save_flags(); \
- local_irq_disable(); \
-})
+static inline unsigned long local_irq_save(void)
+{
+ unsigned long flags = local_save_flags();
+
+ local_irq_disable();
+
+ return flags;
+}
+
#define local_irq_restore(x) \
({ \
BUILD_BUG_ON(sizeof(x) != sizeof(long)); \
diff --git a/xen/include/asm-arm/arm64/system.h b/xen/include/asm-arm/arm64/system.h
index 89c5b64..67e2ed4 100644
--- a/xen/include/asm-arm/arm64/system.h
+++ b/xen/include/asm-arm/arm64/system.h
@@ -29,11 +29,15 @@ static inline unsigned long local_save_flags(void)
return flags;
}
-#define local_irq_save(x) \
-({ \
- x = local_save_flags(); \
- local_irq_disable(); \
-})
+static inline unsigned long local_irq_save(void)
+{
+ unsigned long flags = local_save_flags();
+
+ local_irq_disable();
+
+ return flags;
+}
+
#define local_irq_restore(x) \
({ \
BUILD_BUG_ON(sizeof(x) != sizeof(long)); \
diff --git a/xen/include/asm-x86/system.h b/xen/include/asm-x86/system.h
index faf2efe..756b21f 100644
--- a/xen/include/asm-x86/system.h
+++ b/xen/include/asm-x86/system.h
@@ -262,11 +262,15 @@ static inline unsigned long local_save_flags(void)
return flags;
}
-#define local_irq_save(x) \
-({ \
- x = local_save_flags(); \
- local_irq_disable(); \
-})
+static inline unsigned long local_irq_save(void)
+{
+ unsigned long flags = local_save_flags();
+
+ local_irq_disable();
+
+ return flags;
+}
+
#define local_irq_restore(x) \
({ \
BUILD_BUG_ON(sizeof(x) != sizeof(long)); \
diff --git a/xen/include/xen/rwlock.h b/xen/include/xen/rwlock.h
index 35657c5..10d0f56 100644
--- a/xen/include/xen/rwlock.h
+++ b/xen/include/xen/rwlock.h
@@ -79,8 +79,8 @@ static inline void _read_lock_irq(rwlock_t *lock)
static inline unsigned long _read_lock_irqsave(rwlock_t *lock)
{
- unsigned long flags;
- local_irq_save(flags);
+ unsigned long flags = local_irq_save();
+
_read_lock(lock);
return flags;
}
@@ -136,9 +136,8 @@ static inline void _write_lock_irq(rwlock_t *lock)
static inline unsigned long _write_lock_irqsave(rwlock_t *lock)
{
- unsigned long flags;
+ unsigned long flags = local_irq_save();
- local_irq_save(flags);
_write_lock(lock);
return flags;
}
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index a811b73..3c44c99 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -187,7 +187,7 @@ void _spin_unlock_recursive(spinlock_t *lock);
#define spin_trylock_irqsave(lock, flags) \
({ \
- local_irq_save(flags); \
+ flags = local_irq_save(); \
spin_trylock(lock) ? \
1 : ({ local_irq_restore(flags); 0; }); \
})
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 4/4] xen/arch: Switch local_irq_restore() to being a static inline helper
2018-11-23 16:52 [PATCH 0/4] xen/arch: Treewide improvement of irq helper APIs Andrew Cooper
` (2 preceding siblings ...)
2018-11-23 16:52 ` [PATCH 3/4] xen/arch: Switch local_irq_save() " Andrew Cooper
@ 2018-11-23 16:52 ` Andrew Cooper
2018-11-26 13:30 ` Jan Beulich
2018-11-23 19:02 ` [PATCH 0/4] xen/arch: Treewide improvement of irq helper APIs Julien Grall
4 siblings, 1 reply; 12+ messages in thread
From: Andrew Cooper @ 2018-11-23 16:52 UTC (permalink / raw)
To: Xen-devel
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall,
Jan Beulich, Roger Pau Monné
There is no need for these to be macros, so change them for symetry with
local_irq_save().
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
RFC, as I expect this patch to get some objection for removing the IRQ safety
check, but the only reasons that change was made in e5fc6434d7 was because I
talk talked into doing it while trying to clean up some unnecessary use of
magic numbers.
No users are changing any flags (seeing as I've auditing them all in this
series), and the improvement in emitted code nets us:
add/remove: 1/2 grow/shrink: 2/50 up/down: 835/-2049 (-1214)
Function old new delta
__page_fault_type - 692 +692
reprogram_hpet_evt_channel 243 354 +111
do_page_fault 1249 1281 +32
time_latch_stamps 73 62 -11
vmx_cpu_down 152 136 -16
setup_boot_APIC_clock 896 880 -16
send_IPI_mask_flat 181 165 -16
resource_access 311 295 -16
map_domain_page 1204 1188 -16
lapic_resume 1100 1084 -16
hvm_do_IRQ_dpci 210 194 -16
generic_set_mtrr 258 242 -16
flush_area_local 604 588 -16
dump_runq 293 277 -16
dpci_softirq 726 710 -16
csched2_sys_cntl 237 221 -16
csched2_init_pdata 229 213 -16
csched2_deinit_pdata 354 338 -16
csched2_alloc_domdata 402 386 -16
__printk_ratelimit 332 316 -16
vmx_load_vmcs 156 139 -17
timer_irq_works 85 68 -17
switch_cr3_cr4 249 232 -17
send_IPI_mask_x2apic_phys 199 182 -17
setup_APIC_timer 31 13 -18
serial_tx_interrupt 286 268 -18
console_unlock_recursive_irqrestore 38 20 -18
_spin_unlock_irqrestore 49 31 -18
send_IPI_mask_x2apic_cluster 423 404 -19
csched2_dump 1297 1278 -19
__sync_local_execstate 83 64 -19
enter_state_helper 1210 1190 -20
vprintk_common 346 322 -24
unmap_domain_page 475 451 -24
set_mtrr 346 322 -24
lapic_suspend 742 718 -24
kill_timer 562 538 -24
ipi_handler 155 131 -24
init_percpu_time 411 387 -24
generic_set_all 703 679 -24
csched2_free_domdata 138 114 -24
send_IPI_mask_phys 305 278 -27
call_rcu 223 196 -27
self_nmi 144 112 -32
erst_write 620 588 -32
_ehci_dbgp_poll 473 433 -40
timer_expires_before 328 285 -43
check_for_livepatch_work 813 767 -46
stop_timer 400 352 -48
set_timer 499 450 -49
init_timer 395 346 -49
csched2_dom_cntl 1573 1516 -57
setup_IO_APIC 4797 4738 -59
hpet_next_event 84 - -84
spurious_page_fault 756 - -756
Total: Before=3301185, After=3299971, chg -0.04%
which in particular changes some inlining considerations.
We could have an assertion that system flags dont change, and might want to
consider having some kind of CONFIG_HARDEN for use in release builds, but
given 6 years of hindsight, it would be better for developer builds to
complain loudly for misuses, rather than to silenty squash the error.
---
xen/include/asm-arm/arm32/system.h | 14 +++++---------
xen/include/asm-arm/arm64/system.h | 15 ++++++---------
xen/include/asm-x86/system.h | 14 ++++----------
3 files changed, 15 insertions(+), 28 deletions(-)
diff --git a/xen/include/asm-arm/arm32/system.h b/xen/include/asm-arm/arm32/system.h
index f7c8e53..2fef536 100644
--- a/xen/include/asm-arm/arm32/system.h
+++ b/xen/include/asm-arm/arm32/system.h
@@ -26,15 +26,11 @@ static inline unsigned long local_irq_save(void)
return flags;
}
-#define local_irq_restore(x) \
-({ \
- BUILD_BUG_ON(sizeof(x) != sizeof(long)); \
- asm volatile ( \
- "msr cpsr_c, %0 @ local_irq_restore\n" \
- : \
- : "r" (x) \
- : "memory", "cc"); \
-})
+static inline void local_irq_restore(unsigned long flags)
+{
+ asm volatile ("msr cpsr_c, %0 @ local_irq_restore\n"
+ :: "r" (flags) : "memory", "cc");
+}
static inline bool local_irq_is_enabled(void)
{
diff --git a/xen/include/asm-arm/arm64/system.h b/xen/include/asm-arm/arm64/system.h
index 67e2ed4..7330b0a 100644
--- a/xen/include/asm-arm/arm64/system.h
+++ b/xen/include/asm-arm/arm64/system.h
@@ -38,15 +38,12 @@ static inline unsigned long local_irq_save(void)
return flags;
}
-#define local_irq_restore(x) \
-({ \
- BUILD_BUG_ON(sizeof(x) != sizeof(long)); \
- asm volatile ( \
- "msr daif, %0 // local_irq_restore" \
- : \
- : "r" (x) \
- : "memory"); \
-})
+static inline void local_irq_restore(unsigned long flags)
+{
+ asm volatile ( "msr daif, %0 // local_irq_restore"
+ :: "r" (flags) : "memory" );
+
+}
static inline bool local_irq_is_enabled(void)
{
diff --git a/xen/include/asm-x86/system.h b/xen/include/asm-x86/system.h
index 756b21f..b0906e7 100644
--- a/xen/include/asm-x86/system.h
+++ b/xen/include/asm-x86/system.h
@@ -271,16 +271,10 @@ static inline unsigned long local_irq_save(void)
return flags;
}
-#define local_irq_restore(x) \
-({ \
- BUILD_BUG_ON(sizeof(x) != sizeof(long)); \
- asm volatile ( "pushfq\n\t" \
- "andq %0, (%%rsp)\n\t" \
- "orq %1, (%%rsp)\n\t" \
- "popfq" \
- : : "i?r" ( ~X86_EFLAGS_IF ), \
- "ri" ( (x) & X86_EFLAGS_IF ) ); \
-})
+static inline void local_irq_restore(unsigned long flags)
+{
+ asm volatile ( "push %0; popf;" :: "g" (flags) : "memory" );
+}
static inline bool local_irq_is_enabled(void)
{
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 0/4] xen/arch: Treewide improvement of irq helper APIs
2018-11-23 16:52 [PATCH 0/4] xen/arch: Treewide improvement of irq helper APIs Andrew Cooper
` (3 preceding siblings ...)
2018-11-23 16:52 ` [PATCH 4/4] xen/arch: Switch local_irq_restore() " Andrew Cooper
@ 2018-11-23 19:02 ` Julien Grall
4 siblings, 0 replies; 12+ messages in thread
From: Julien Grall @ 2018-11-23 19:02 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: Stefano Stabellini, Wei Liu, Jan Beulich, Roger Pau Monné
Hi Andrew,
On 11/23/18 4:52 PM, Andrew Cooper wrote:
> This is a rather old cleanup series which I never got around to finishing. It
> is restricted to the arch maintainers as it is purely mechanical outside of
> system.h
>
> The main purpose is to reduce the quantity of code we have which mutates
> parameters by name.
>
> One ARM point which I've noticed is that system.h is a mix of duplicated
> common code in the arch{32,64}/ versions, and ifdefary in the common version.
> Does this perhaps want to be made consistent? I've not touched the layout in
> this series.
Hmmm, I don't really want to have the barrier split between multiple
headers. So we would have to duplicate them in arm32 and arm64.
For the IRQ helpers, we can probably be a bit smarter. I can have a look
at clean-up once this is merged.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] xen/arch: Switch local_*_is_enabled() predicates to return bool
2018-11-23 16:52 ` [PATCH 1/4] xen/arch: Switch local_*_is_enabled() predicates to return bool Andrew Cooper
@ 2018-11-26 13:04 ` Jan Beulich
2018-11-27 9:27 ` Julien Grall
1 sibling, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2018-11-26 13:04 UTC (permalink / raw)
To: Andrew Cooper
Cc: Julien Grall, Stefano Stabellini, Wei Liu, Xen-devel, Roger Pau Monne
>>> On 23.11.18 at 17:52, <andrew.cooper3@citrix.com> wrote:
> No functional change, as the value returned was previously always 0 or 1.
> While altering these, insert blank lines where appropriate and drop the
> now-redundant !! from x86's local_irq_is_enabled().
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/4] xen/arch: Switch local_save_flags() to being a static inline helper
2018-11-23 16:52 ` [PATCH 2/4] xen/arch: Switch local_save_flags() to being a static inline helper Andrew Cooper
@ 2018-11-26 13:15 ` Jan Beulich
2018-11-27 9:31 ` Julien Grall
1 sibling, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2018-11-26 13:15 UTC (permalink / raw)
To: Andrew Cooper
Cc: Julien Grall, Stefano Stabellini, Wei Liu, Xen-devel, Roger Pau Monne
>>> On 23.11.18 at 17:52, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/asm-x86/system.h
> +++ b/xen/include/asm-x86/system.h
> @@ -253,14 +253,18 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
> /* used when interrupts are already enabled or to shutdown the processor */
> #define halt() asm volatile ( "hlt" : : : "memory" )
>
> -#define local_save_flags(x) \
> -({ \
> - BUILD_BUG_ON(sizeof(x) != sizeof(long)); \
> - asm volatile ( "pushf" __OS " ; pop" __OS " %0" : "=g" (x)); \
> -})
> +static inline unsigned long local_save_flags(void)
> +{
> + unsigned long flags;
> +
> + asm volatile ( "pushf; pop %0;" : "=g" (flags) );
> +
> + return flags;
> +}
Provided this doesn't defeat the current possibility of the compiler
POP-ing directly into a stack variable
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Apart from this I'm a little puzzled by the uppercasing of SAVE in
the Arm asm() comments, but I'll leave that to the Arm maintainers.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/4] xen/arch: Switch local_irq_save() to being a static inline helper
2018-11-23 16:52 ` [PATCH 3/4] xen/arch: Switch local_irq_save() " Andrew Cooper
@ 2018-11-26 13:22 ` Jan Beulich
0 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2018-11-26 13:22 UTC (permalink / raw)
To: Andrew Cooper
Cc: Julien Grall, Stefano Stabellini, Wei Liu, Xen-devel, Roger Pau Monne
>>> On 23.11.18 at 17:52, <andrew.cooper3@citrix.com> wrote:
> ... rather than a macro which writes to its parameter by name. Take the
> opportunity to fold the assignment into the flags declaraion where
> appropriate.
Do you really? Why not ...
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -401,7 +401,7 @@ void *map_domain_page(mfn_t mfn)
> lpae_t pte;
> int i, slot;
>
> - local_irq_save(flags);
> + flags = local_irq_save();
... here, for example? There are a few more cases, I think.
> --- a/xen/include/asm-x86/system.h
> +++ b/xen/include/asm-x86/system.h
> @@ -262,11 +262,15 @@ static inline unsigned long local_save_flags(void)
> return flags;
> }
>
> -#define local_irq_save(x) \
> -({ \
> - x = local_save_flags(); \
> - local_irq_disable(); \
> -})
> +static inline unsigned long local_irq_save(void)
> +{
> + unsigned long flags = local_save_flags();
> +
> + local_irq_disable();
> +
> + return flags;
> +}
Do we really need/want to retain this as a per-arch construct?
Since you touch all instances anyway, do we really want to
stick to its misleading name?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 4/4] xen/arch: Switch local_irq_restore() to being a static inline helper
2018-11-23 16:52 ` [PATCH 4/4] xen/arch: Switch local_irq_restore() " Andrew Cooper
@ 2018-11-26 13:30 ` Jan Beulich
0 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2018-11-26 13:30 UTC (permalink / raw)
To: Andrew Cooper
Cc: Julien Grall, Stefano Stabellini, Wei Liu, Xen-devel, Roger Pau Monne
>>> On 23.11.18 at 17:52, <andrew.cooper3@citrix.com> wrote:
> RFC, as I expect this patch to get some objection for removing the IRQ safety
> check, but the only reasons that change was made in e5fc6434d7 was because I
> talk talked into doing it while trying to clean up some unnecessary use of
> magic numbers.
>
> No users are changing any flags (seeing as I've auditing them all in this
> series), and the improvement in emitted code nets us:
Users not changing any flags as per your audit is only one half of it:
I assume you mean the value returned from local_irq_save() gets
passed back unmodified into local_irq_restore(). The other question
is whether intermediately another change to the CPU register has
occurred, which is meant to persist. With Arm also controlling e.g.
enabled status of aborts this way, it may be even more of an issue
there than on x86.
> +static inline void local_irq_restore(unsigned long flags)
> +{
> + asm volatile ( "push %0; popf;" :: "g" (flags) : "memory" );
I'm afraid this is not entirely right (but perhaps benign): Not
all constants are valid to be handed to PUSH. If you want to
allow immediates in the first place, this would need to be "rme",
I think.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] xen/arch: Switch local_*_is_enabled() predicates to return bool
2018-11-23 16:52 ` [PATCH 1/4] xen/arch: Switch local_*_is_enabled() predicates to return bool Andrew Cooper
2018-11-26 13:04 ` Jan Beulich
@ 2018-11-27 9:27 ` Julien Grall
1 sibling, 0 replies; 12+ messages in thread
From: Julien Grall @ 2018-11-27 9:27 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: Stefano Stabellini, Wei Liu, Jan Beulich, Roger Pau Monné
Hi Andrew,
On 11/23/18 4:52 PM, Andrew Cooper wrote:
> No functional change, as the value returned was previously always 0 or 1.
> While altering these, insert blank lines where appropriate and drop the
> now-redundant !! from x86's local_irq_is_enabled().
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Julien Grall <julien.grall@arm.com>
Cheers,
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
> xen/include/asm-arm/arm32/system.h | 8 ++++++--
> xen/include/asm-arm/arm64/system.h | 8 ++++++--
> xen/include/asm-arm/system.h | 4 +++-
> xen/include/asm-x86/system.h | 6 ++++--
> 4 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/xen/include/asm-arm/arm32/system.h b/xen/include/asm-arm/arm32/system.h
> index ab57abf..58c8fb3 100644
> --- a/xen/include/asm-arm/arm32/system.h
> +++ b/xen/include/asm-arm/arm32/system.h
> @@ -28,10 +28,12 @@
> : "memory", "cc"); \
> })
>
> -static inline int local_irq_is_enabled(void)
> +static inline bool local_irq_is_enabled(void)
> {
> unsigned long flags;
> +
> local_save_flags(flags);
> +
> return !(flags & PSR_IRQ_MASK);
> }
>
> @@ -41,10 +43,12 @@ static inline int local_irq_is_enabled(void)
> #define local_abort_enable() __asm__("cpsie a @ __sta\n" : : : "memory", "cc")
> #define local_abort_disable() __asm__("cpsid a @ __sta\n" : : : "memory", "cc")
>
> -static inline int local_fiq_is_enabled(void)
> +static inline bool local_fiq_is_enabled(void)
> {
> unsigned long flags;
> +
> local_save_flags(flags);
> +
> return !(flags & PSR_FIQ_MASK);
> }
>
> diff --git a/xen/include/asm-arm/arm64/system.h b/xen/include/asm-arm/arm64/system.h
> index 2e36573..d17fc9d 100644
> --- a/xen/include/asm-arm/arm64/system.h
> +++ b/xen/include/asm-arm/arm64/system.h
> @@ -44,17 +44,21 @@
> : "memory"); \
> })
>
> -static inline int local_irq_is_enabled(void)
> +static inline bool local_irq_is_enabled(void)
> {
> unsigned long flags;
> +
> local_save_flags(flags);
> +
> return !(flags & PSR_IRQ_MASK);
> }
>
> -static inline int local_fiq_is_enabled(void)
> +static inline bool local_fiq_is_enabled(void)
> {
> unsigned long flags;
> +
> local_save_flags(flags);
> +
> return !(flags & PSR_FIQ_MASK);
> }
>
> diff --git a/xen/include/asm-arm/system.h b/xen/include/asm-arm/system.h
> index b94e56f..bc51300 100644
> --- a/xen/include/asm-arm/system.h
> +++ b/xen/include/asm-arm/system.h
> @@ -51,10 +51,12 @@
> # error "unknown ARM variant"
> #endif
>
> -static inline int local_abort_is_enabled(void)
> +static inline bool local_abort_is_enabled(void)
> {
> unsigned long flags;
> +
> local_save_flags(flags);
> +
> return !(flags & PSR_ABT_MASK);
> }
>
> diff --git a/xen/include/asm-x86/system.h b/xen/include/asm-x86/system.h
> index 483cd20..4b7056d 100644
> --- a/xen/include/asm-x86/system.h
> +++ b/xen/include/asm-x86/system.h
> @@ -274,11 +274,13 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
> "ri" ( (x) & X86_EFLAGS_IF ) ); \
> })
>
> -static inline int local_irq_is_enabled(void)
> +static inline bool local_irq_is_enabled(void)
> {
> unsigned long flags;
> +
> local_save_flags(flags);
> - return !!(flags & X86_EFLAGS_IF);
> +
> + return flags & X86_EFLAGS_IF;
> }
>
> #define BROKEN_ACPI_Sx 0x0001
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/4] xen/arch: Switch local_save_flags() to being a static inline helper
2018-11-23 16:52 ` [PATCH 2/4] xen/arch: Switch local_save_flags() to being a static inline helper Andrew Cooper
2018-11-26 13:15 ` Jan Beulich
@ 2018-11-27 9:31 ` Julien Grall
1 sibling, 0 replies; 12+ messages in thread
From: Julien Grall @ 2018-11-27 9:31 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: Stefano Stabellini, Wei Liu, Jan Beulich, Roger Pau Monné
On 11/23/18 4:52 PM, Andrew Cooper wrote:
> ... rather than a macro which writes to its parameter by name. A consequence
> of this change is that the local variables in local_*_is_enabled() can be
> dropped.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
> xen/arch/x86/io_apic.c | 2 +-
> xen/include/asm-arm/arm32/system.h | 30 +++++++++++++-----------------
> xen/include/asm-arm/arm64/system.h | 32 ++++++++++++--------------------
> xen/include/asm-arm/system.h | 6 +-----
> xen/include/asm-x86/system.h | 22 +++++++++++-----------
> 5 files changed, 38 insertions(+), 54 deletions(-)
>
> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> index daa5e9e..aca4f63 100644
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -1486,7 +1486,7 @@ static int __init timer_irq_works(void)
>
> t1 = ACCESS_ONCE(pit0_ticks);
>
> - local_save_flags(flags);
> + flags = local_save_flags();
> local_irq_enable();
> /* Let ten ticks pass... */
> mdelay((10 * 1000) / HZ);
> diff --git a/xen/include/asm-arm/arm32/system.h b/xen/include/asm-arm/arm32/system.h
> index 58c8fb3..cbfa91d 100644
> --- a/xen/include/asm-arm/arm32/system.h
> +++ b/xen/include/asm-arm/arm32/system.h
> @@ -7,15 +7,19 @@
> #define local_irq_disable() asm volatile ( "cpsid i @ local_irq_disable\n" : : : "cc" )
> #define local_irq_enable() asm volatile ( "cpsie i @ local_irq_enable\n" : : : "cc" )
>
> -#define local_save_flags(x) \
> -({ \
> - BUILD_BUG_ON(sizeof(x) != sizeof(long)); \
> - asm volatile ( "mrs %0, cpsr @ local_save_flags\n" \
> - : "=r" (x) :: "memory", "cc" ); \
> -})
> +static inline unsigned long local_save_flags(void)
> +{
> + unsigned long flags;
> +
> + asm volatile ( "mrs %0, cpsr @ local_SAVE_flags\n"
Any reason for writing "save" in uppercase?
> + : "=r" (flags) :: "memory", "cc" );
> +
> + return flags;
> +}
> +
[...]
The rest of the code looks good to me.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-11-27 9:31 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-23 16:52 [PATCH 0/4] xen/arch: Treewide improvement of irq helper APIs Andrew Cooper
2018-11-23 16:52 ` [PATCH 1/4] xen/arch: Switch local_*_is_enabled() predicates to return bool Andrew Cooper
2018-11-26 13:04 ` Jan Beulich
2018-11-27 9:27 ` Julien Grall
2018-11-23 16:52 ` [PATCH 2/4] xen/arch: Switch local_save_flags() to being a static inline helper Andrew Cooper
2018-11-26 13:15 ` Jan Beulich
2018-11-27 9:31 ` Julien Grall
2018-11-23 16:52 ` [PATCH 3/4] xen/arch: Switch local_irq_save() " Andrew Cooper
2018-11-26 13:22 ` Jan Beulich
2018-11-23 16:52 ` [PATCH 4/4] xen/arch: Switch local_irq_restore() " Andrew Cooper
2018-11-26 13:30 ` Jan Beulich
2018-11-23 19:02 ` [PATCH 0/4] xen/arch: Treewide improvement of irq helper APIs Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.