| Message ID | 20221207061815.7404-4-vikram.garhwal@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dynamic node programming using overlay dtbo | expand |
On 07.12.2022 07:18, Vikram Garhwal wrote: > --- a/xen/include/xen/cpu.h > +++ b/xen/include/xen/cpu.h > @@ -5,6 +5,10 @@ > #include <xen/spinlock.h> > #include <xen/notifier.h> > > +#ifdef CONFIG_ARM > +#include <asm/cpu.h> > +#endif > + > /* Safely access cpu_online_map, cpu_present_map, etc. */ > bool get_cpu_maps(void); > void put_cpu_maps(void); > --- a/xen/include/xen/softirq.h > +++ b/xen/include/xen/softirq.h > @@ -19,6 +19,10 @@ enum { > #include <asm/hardirq.h> > #include <asm/softirq.h> > > +#ifdef CONFIG_ARM > +#include <asm/cpu.h> > +#endif We generally try to avoid such #ifdef-ary whenever possible, so the description wants to justify these if they cannot be avoided. Jan
Hi Vikram, On 07/12/2022 06:18, Vikram Garhwal wrote: > Dynamic programming ops will modify the dt_host and there might be other > function which are browsing the dt_host at the same time. To avoid the race > conditions, adding rwlock for browsing the dt_host. But adding rwlock in > device_tree.h causes following circular dependency: > device_tree.h->rwlock.h->smp.h->asm/smp.h->device_tree.h > > Inside arch/arm/include/asm/smp.h, there is one function which needs > device_tree.h, moved the cpu related function to a new file: > arch/arm/include/asm/cpu.h Given there is only one function, I don't really see the benefits of splitting smp.h and then adding #ifdef CONFIG_ARM in the common code. Instead, it would be better if we don't include device_tree.h in the header but in the c files that need to call arch_cpu_init() and forward declare dt_device_node. Another potential approach is to move out the percpu_rwlock helpers in a separate header. The advantage with this approach is we would reduce the number of definition included everywhere (there are not many use of the percpu rwlock). Cheers,
Hi Julien On 12/7/22 8:28 AM, Julien Grall wrote: > Hi Vikram, > > On 07/12/2022 06:18, Vikram Garhwal wrote: >> Dynamic programming ops will modify the dt_host and there might be other >> function which are browsing the dt_host at the same time. To avoid >> the race >> conditions, adding rwlock for browsing the dt_host. But adding rwlock in >> device_tree.h causes following circular dependency: >> device_tree.h->rwlock.h->smp.h->asm/smp.h->device_tree.h >> >> Inside arch/arm/include/asm/smp.h, there is one function which needs >> device_tree.h, moved the cpu related function to a new file: >> arch/arm/include/asm/cpu.h > > Given there is only one function, I don't really see the benefits of > splitting smp.h and then adding #ifdef CONFIG_ARM in the common code. > > Instead, it would be better if we don't include device_tree.h in the > header but in the c files that need to call arch_cpu_init() and > forward declare dt_device_node. > This was my initial approach also and there were less changes(compare to my v4) but then though someone might have issues with forward declaration of dt_device_node in smp.h. > Another potential approach is to move out the percpu_rwlock helpers in > a separate header. The advantage with this approach is we would reduce > the number of definition included everywhere (there are not many use > of the percpu rwlock). Will check this option. Thank you suggestions! > > Cheers, >
On 07.12.2022 20:39, Vikram Garhwal wrote: > Hi Julien > > On 12/7/22 8:28 AM, Julien Grall wrote: >> Hi Vikram, >> >> On 07/12/2022 06:18, Vikram Garhwal wrote: >>> Dynamic programming ops will modify the dt_host and there might be other >>> function which are browsing the dt_host at the same time. To avoid >>> the race >>> conditions, adding rwlock for browsing the dt_host. But adding rwlock in >>> device_tree.h causes following circular dependency: >>> device_tree.h->rwlock.h->smp.h->asm/smp.h->device_tree.h >>> >>> Inside arch/arm/include/asm/smp.h, there is one function which needs >>> device_tree.h, moved the cpu related function to a new file: >>> arch/arm/include/asm/cpu.h >> >> Given there is only one function, I don't really see the benefits of >> splitting smp.h and then adding #ifdef CONFIG_ARM in the common code. >> >> Instead, it would be better if we don't include device_tree.h in the >> header but in the c files that need to call arch_cpu_init() and >> forward declare dt_device_node. >> > This was my initial approach also and there were less changes(compare to > my v4) but then though someone might have issues with forward > declaration of dt_device_node in smp.h. We use forward declarations of struct/union is many places, precisely to limit dependencies among headers. Jan
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h index 43a836c3a7..ca40c6f73f 100644 --- a/xen/arch/arm/efi/efi-boot.h +++ b/xen/arch/arm/efi/efi-boot.h @@ -5,6 +5,7 @@ */ #include <xen/device_tree.h> #include <xen/libfdt/libfdt.h> +#include <asm/cpu.h> #include <asm/setup.h> #include <asm/smp.h> diff --git a/xen/arch/arm/include/asm/cpu.h b/xen/arch/arm/include/asm/cpu.h new file mode 100644 index 0000000000..4df80ca1b5 --- /dev/null +++ b/xen/arch/arm/include/asm/cpu.h @@ -0,0 +1,35 @@ +#ifndef __ASM_CPU_H +#define __ASM_CPU_H + +#ifndef __ASSEMBLY__ +#include <xen/cpumask.h> +#include <xen/device_tree.h> +#include <asm/current.h> +#endif + +DECLARE_PER_CPU(cpumask_var_t, cpu_sibling_mask); +DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask); + +#define cpu_is_offline(cpu) unlikely(!cpu_online(cpu)) + +/* + * Do we, for platform reasons, need to actually keep CPUs online when we + * would otherwise prefer them to be off? + */ +#define park_offline_cpus false + +extern void noreturn stop_cpu(void); + +extern int arch_cpu_init(int cpu, struct dt_device_node *dn); +extern int arch_cpu_up(int cpu); + +int cpu_up_send_sgi(int cpu); + +/* Secondary CPU entry point */ +extern void init_secondary(void); + +#define cpu_physical_id(cpu) cpu_logical_map(cpu) + +#endif + + diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h index 2ce6764322..f9440e5c7e 100644 --- a/xen/arch/arm/include/asm/domain.h +++ b/xen/arch/arm/include/asm/domain.h @@ -3,6 +3,7 @@ #include <xen/cache.h> #include <xen/timer.h> +#include <asm/cpu.h> #include <asm/page.h> #include <asm/p2m.h> #include <asm/vfp.h> diff --git a/xen/arch/arm/include/asm/psci.h b/xen/arch/arm/include/asm/psci.h index 832f77afff..74c1bc6368 100644 --- a/xen/arch/arm/include/asm/psci.h +++ b/xen/arch/arm/include/asm/psci.h @@ -1,6 +1,7 @@ #ifndef __ASM_PSCI_H__ #define __ASM_PSCI_H__ +#include <asm/cpu.h> #include <asm/smccc.h> /* PSCI return values (inclusive of all PSCI versions) */ diff --git a/xen/arch/arm/include/asm/smp.h b/xen/arch/arm/include/asm/smp.h index 8133d5c295..76944b07f7 100644 --- a/xen/arch/arm/include/asm/smp.h +++ b/xen/arch/arm/include/asm/smp.h @@ -3,40 +3,16 @@ #ifndef __ASSEMBLY__ #include <xen/cpumask.h> -#include <xen/device_tree.h> #include <asm/current.h> #endif -DECLARE_PER_CPU(cpumask_var_t, cpu_sibling_mask); -DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask); - -#define cpu_is_offline(cpu) unlikely(!cpu_online(cpu)) - #define smp_processor_id() get_processor_id() -/* - * Do we, for platform reasons, need to actually keep CPUs online when we - * would otherwise prefer them to be off? - */ -#define park_offline_cpus false - -extern void noreturn stop_cpu(void); - extern int arch_smp_init(void); -extern int arch_cpu_init(int cpu, struct dt_device_node *dn); -extern int arch_cpu_up(int cpu); - -int cpu_up_send_sgi(int cpu); - -/* Secondary CPU entry point */ -extern void init_secondary(void); extern void smp_init_cpus(void); extern void smp_clear_cpu_maps (void); extern unsigned int smp_get_max_cpus(void); - -#define cpu_physical_id(cpu) cpu_logical_map(cpu) - #endif /* diff --git a/xen/include/xen/cpu.h b/xen/include/xen/cpu.h index e8eeb217a0..ce93eb0003 100644 --- a/xen/include/xen/cpu.h +++ b/xen/include/xen/cpu.h @@ -5,6 +5,10 @@ #include <xen/spinlock.h> #include <xen/notifier.h> +#ifdef CONFIG_ARM +#include <asm/cpu.h> +#endif + /* Safely access cpu_online_map, cpu_present_map, etc. */ bool get_cpu_maps(void); void put_cpu_maps(void); diff --git a/xen/include/xen/softirq.h b/xen/include/xen/softirq.h index 1f6c4783da..cc98a65287 100644 --- a/xen/include/xen/softirq.h +++ b/xen/include/xen/softirq.h @@ -19,6 +19,10 @@ enum { #include <asm/hardirq.h> #include <asm/softirq.h> +#ifdef CONFIG_ARM +#include <asm/cpu.h> +#endif + #define NR_SOFTIRQS (NR_COMMON_SOFTIRQS + NR_ARCH_SOFTIRQS) typedef void (*softirq_handler)(void);
Dynamic programming ops will modify the dt_host and there might be other function which are browsing the dt_host at the same time. To avoid the race conditions, adding rwlock for browsing the dt_host. But adding rwlock in device_tree.h causes following circular dependency: device_tree.h->rwlock.h->smp.h->asm/smp.h->device_tree.h Inside arch/arm/include/asm/smp.h, there is one function which needs device_tree.h, moved the cpu related function to a new file: arch/arm/include/asm/cpu.h Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> --- xen/arch/arm/efi/efi-boot.h | 1 + xen/arch/arm/include/asm/cpu.h | 35 +++++++++++++++++++++++++++++++ xen/arch/arm/include/asm/domain.h | 1 + xen/arch/arm/include/asm/psci.h | 1 + xen/arch/arm/include/asm/smp.h | 24 --------------------- xen/include/xen/cpu.h | 4 ++++ xen/include/xen/softirq.h | 4 ++++ 7 files changed, 46 insertions(+), 24 deletions(-) create mode 100644 xen/arch/arm/include/asm/cpu.h