2021-02-12 05:00:08

by Shuo Liu

Subject: [PATCH] virt: acrn: Fix vCPU removing code build error

From: Shuo Liu <[email protected]>

vCPU removing code depends on CONFIG_HOTPLUG_CPU as it uses remove_cpu()
and add_cpu(). Make the vCPU removing interface building with
CONFIG_HOTPLUG_CPU.

../drivers/virt/acrn/hsm.c: In function ‘remove_cpu_store’:
../drivers/virt/acrn/hsm.c:389:3: error: implicit declaration of function ‘remove_cpu’; [-Werror=implicit-function-declaration]
remove_cpu(cpu);

../drivers/virt/acrn/hsm.c:402:2: error: implicit declaration of function ‘add_cpu’; [-Werror=implicit-function-declaration]
add_cpu(cpu);

Fixes: 279dcf693ac7 ("virt: acrn: Introduce an interface for Service VM to control vCPU")
Reported-by: Randy Dunlap <[email protected]>
Acked-by: Randy Dunlap <[email protected]> # build-tested
Signed-off-by: Shuo Liu <[email protected]>
---
drivers/virt/acrn/hsm.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/virt/acrn/hsm.c b/drivers/virt/acrn/hsm.c
index 1f6b7c54a1a4..e340788aacdf 100644
--- a/drivers/virt/acrn/hsm.c
+++ b/drivers/virt/acrn/hsm.c
@@ -372,6 +372,7 @@ static int acrn_dev_release(struct inode *inode, struct file *filp)
return 0;
}

+#ifdef CONFIG_HOTPLUG_CPU
static ssize_t remove_cpu_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
@@ -403,9 +404,12 @@ static ssize_t remove_cpu_store(struct device *dev,
return ret;
}
static DEVICE_ATTR_WO(remove_cpu);
+#endif

static struct attribute *acrn_attrs[] = {
+#ifdef CONFIG_HOTPLUG_CPU
&dev_attr_remove_cpu.attr,
+#endif
NULL
};


base-commit: 671176b0016c80b3943cb5387312c886aba3308d
--
2.28.0

2021-02-12 07:54:22

by Greg Kroah-Hartman

Subject: Re: [PATCH] virt: acrn: Fix vCPU removing code build error

On Fri, Feb 12, 2021 at 12:57:24PM +0800, [email protected] wrote:
> From: Shuo Liu <[email protected]>
>
> vCPU removing code depends on CONFIG_HOTPLUG_CPU as it uses remove_cpu()
> and add_cpu(). Make the vCPU removing interface building with
> CONFIG_HOTPLUG_CPU.
>
> ../drivers/virt/acrn/hsm.c: In function ‘remove_cpu_store’:
> ../drivers/virt/acrn/hsm.c:389:3: error: implicit declaration of function ‘remove_cpu’; [-Werror=implicit-function-declaration]
> remove_cpu(cpu);
>
> ../drivers/virt/acrn/hsm.c:402:2: error: implicit declaration of function ‘add_cpu’; [-Werror=implicit-function-declaration]
> add_cpu(cpu);
>
> Fixes: 279dcf693ac7 ("virt: acrn: Introduce an interface for Service VM to control vCPU")
> Reported-by: Randy Dunlap <[email protected]>
> Acked-by: Randy Dunlap <[email protected]> # build-tested
> Signed-off-by: Shuo Liu <[email protected]>
> ---
> drivers/virt/acrn/hsm.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/virt/acrn/hsm.c b/drivers/virt/acrn/hsm.c
> index 1f6b7c54a1a4..e340788aacdf 100644
> --- a/drivers/virt/acrn/hsm.c
> +++ b/drivers/virt/acrn/hsm.c
> @@ -372,6 +372,7 @@ static int acrn_dev_release(struct inode *inode, struct file *filp)
> return 0;
> }
>
> +#ifdef CONFIG_HOTPLUG_CPU
> static ssize_t remove_cpu_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> @@ -403,9 +404,12 @@ static ssize_t remove_cpu_store(struct device *dev,
> return ret;
> }
> static DEVICE_ATTR_WO(remove_cpu);
> +#endif
>
> static struct attribute *acrn_attrs[] = {
> +#ifdef CONFIG_HOTPLUG_CPU
> &dev_attr_remove_cpu.attr,
> +#endif
> NULL
> };
>
>

Shouldn't the real solution for this be that remove_cpu() and add_cpu()
have function prototypes for when this is not enabled in the kernel
build?

Putting #ifdef in .c files like this is not a good idea at all.

Then, at runtime, you can determine if you need to create this sysfs
file or not, as you do not want to expose it to userspace if the kernel
can not handle it, right?

thanks,

greg k-h

2021-02-12 11:01:10

by Shuo Liu

Subject: Re: [PATCH] virt: acrn: Fix vCPU removing code build error

Hi Greg,

On Fri 12.Feb'21 at 8:52:33 +0100, Greg Kroah-Hartman wrote:
>On Fri, Feb 12, 2021 at 12:57:24PM +0800, [email protected] wrote:
>> From: Shuo Liu <[email protected]>
>>
>> vCPU removing code depends on CONFIG_HOTPLUG_CPU as it uses remove_cpu()
>> and add_cpu(). Make the vCPU removing interface building with
>> CONFIG_HOTPLUG_CPU.
>>
>> ../drivers/virt/acrn/hsm.c: In function ‘remove_cpu_store’:
>> ../drivers/virt/acrn/hsm.c:389:3: error: implicit declaration of function ‘remove_cpu’; [-Werror=implicit-function-declaration]
>> remove_cpu(cpu);
>>
>> ../drivers/virt/acrn/hsm.c:402:2: error: implicit declaration of function ‘add_cpu’; [-Werror=implicit-function-declaration]
>> add_cpu(cpu);
>>
>> Fixes: 279dcf693ac7 ("virt: acrn: Introduce an interface for Service VM to control vCPU")
>> Reported-by: Randy Dunlap <[email protected]>
>> Acked-by: Randy Dunlap <[email protected]> # build-tested
>> Signed-off-by: Shuo Liu <[email protected]>
>> ---
>> drivers/virt/acrn/hsm.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/virt/acrn/hsm.c b/drivers/virt/acrn/hsm.c
>> index 1f6b7c54a1a4..e340788aacdf 100644
>> --- a/drivers/virt/acrn/hsm.c
>> +++ b/drivers/virt/acrn/hsm.c
>> @@ -372,6 +372,7 @@ static int acrn_dev_release(struct inode *inode, struct file *filp)
>> return 0;
>> }
>>
>> +#ifdef CONFIG_HOTPLUG_CPU
>> static ssize_t remove_cpu_store(struct device *dev,
>> struct device_attribute *attr,
>> const char *buf, size_t count)
>> @@ -403,9 +404,12 @@ static ssize_t remove_cpu_store(struct device *dev,
>> return ret;
>> }
>> static DEVICE_ATTR_WO(remove_cpu);
>> +#endif
>>
>> static struct attribute *acrn_attrs[] = {
>> +#ifdef CONFIG_HOTPLUG_CPU
>> &dev_attr_remove_cpu.attr,
>> +#endif
>> NULL
>> };
>>
>>
>
>Shouldn't the real solution for this be that remove_cpu() and add_cpu()
>have function prototypes for when this is not enabled in the kernel
>build?

Something like this in linux/cpu.h?

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 3aaa0687e8df..94a578a96202 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -108,6 +108,8 @@ static inline void cpu_maps_update_done(void)
{
}

+static inline int add_cpu(unsigned int cpu) { return 0;}
+
#endif /* CONFIG_SMP */
extern struct bus_type cpu_subsys;

@@ -137,6 +139,7 @@ static inline int cpus_read_trylock(void) { return
true; }
static inline void lockdep_assert_cpus_held(void) { }
static inline void cpu_hotplug_disable(void) { }
static inline void cpu_hotplug_enable(void) { }
+static inline int remove_cpu(unsigned int cpu) { return -EPERM; }
static inline void smp_shutdown_nonboot_cpus(unsigned int primary_cpu)
{ }
#endif /* !CONFIG_HOTPLUG_CPU */


>
>Putting #ifdef in .c files like this is not a good idea at all.
>
>Then, at runtime, you can determine if you need to create this sysfs
>file or not, as you do not want to expose it to userspace if the kernel
>can not handle it, right?

Right. I don't want to expose the sysfs to userspace if the kernel built
w/o CONFIG_HOTPLUG_CPU. But how to implement that if #ifdef is not used?
misc_register() creates sysfs with .groups (acrn_attr_groups)
unconditionally, then userspace can see the interface even it doesn't
work.

Thanks
shuo

2021-02-12 11:07:11

by Greg Kroah-Hartman

Subject: Re: [PATCH] virt: acrn: Fix vCPU removing code build error

On Fri, Feb 12, 2021 at 06:58:53PM +0800, Shuo A Liu wrote:
> Hi Greg,
>
> On Fri 12.Feb'21 at 8:52:33 +0100, Greg Kroah-Hartman wrote:
> > On Fri, Feb 12, 2021 at 12:57:24PM +0800, [email protected] wrote:
> > > From: Shuo Liu <[email protected]>
> > >
> > > vCPU removing code depends on CONFIG_HOTPLUG_CPU as it uses remove_cpu()
> > > and add_cpu(). Make the vCPU removing interface building with
> > > CONFIG_HOTPLUG_CPU.
> > >
> > > ../drivers/virt/acrn/hsm.c: In function ‘remove_cpu_store’:
> > > ../drivers/virt/acrn/hsm.c:389:3: error: implicit declaration of function ‘remove_cpu’; [-Werror=implicit-function-declaration]
> > > remove_cpu(cpu);
> > >
> > > ../drivers/virt/acrn/hsm.c:402:2: error: implicit declaration of function ‘add_cpu’; [-Werror=implicit-function-declaration]
> > > add_cpu(cpu);
> > >
> > > Fixes: 279dcf693ac7 ("virt: acrn: Introduce an interface for Service VM to control vCPU")
> > > Reported-by: Randy Dunlap <[email protected]>
> > > Acked-by: Randy Dunlap <[email protected]> # build-tested
> > > Signed-off-by: Shuo Liu <[email protected]>
> > > ---
> > > drivers/virt/acrn/hsm.c | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/virt/acrn/hsm.c b/drivers/virt/acrn/hsm.c
> > > index 1f6b7c54a1a4..e340788aacdf 100644
> > > --- a/drivers/virt/acrn/hsm.c
> > > +++ b/drivers/virt/acrn/hsm.c
> > > @@ -372,6 +372,7 @@ static int acrn_dev_release(struct inode *inode, struct file *filp)
> > > return 0;
> > > }
> > >
> > > +#ifdef CONFIG_HOTPLUG_CPU
> > > static ssize_t remove_cpu_store(struct device *dev,
> > > struct device_attribute *attr,
> > > const char *buf, size_t count)
> > > @@ -403,9 +404,12 @@ static ssize_t remove_cpu_store(struct device *dev,
> > > return ret;
> > > }
> > > static DEVICE_ATTR_WO(remove_cpu);
> > > +#endif
> > >
> > > static struct attribute *acrn_attrs[] = {
> > > +#ifdef CONFIG_HOTPLUG_CPU
> > > &dev_attr_remove_cpu.attr,
> > > +#endif
> > > NULL
> > > };
> > >
> > >
> >
> > Shouldn't the real solution for this be that remove_cpu() and add_cpu()
> > have function prototypes for when this is not enabled in the kernel
> > build?
>
> Something like this in linux/cpu.h?
>
> diff --git a/include/linux/cpu.h b/include/linux/cpu.h
> index 3aaa0687e8df..94a578a96202 100644
> --- a/include/linux/cpu.h
> +++ b/include/linux/cpu.h
> @@ -108,6 +108,8 @@ static inline void cpu_maps_update_done(void)
> {
> }
>
> +static inline int add_cpu(unsigned int cpu) { return 0;}
> +
> #endif /* CONFIG_SMP */
> extern struct bus_type cpu_subsys;
>
> @@ -137,6 +139,7 @@ static inline int cpus_read_trylock(void) { return
> true; }
> static inline void lockdep_assert_cpus_held(void) { }
> static inline void cpu_hotplug_disable(void) { }
> static inline void cpu_hotplug_enable(void) { }
> +static inline int remove_cpu(unsigned int cpu) { return -EPERM; }
> static inline void smp_shutdown_nonboot_cpus(unsigned int primary_cpu)
> { }
> #endif /* !CONFIG_HOTPLUG_CPU */
>
>
> >
> > Putting #ifdef in .c files like this is not a good idea at all.
> >
> > Then, at runtime, you can determine if you need to create this sysfs
> > file or not, as you do not want to expose it to userspace if the kernel
> > can not handle it, right?
>
> Right. I don't want to expose the sysfs to userspace if the kernel built
> w/o CONFIG_HOTPLUG_CPU. But how to implement that if #ifdef is not used?
> misc_register() creates sysfs with .groups (acrn_attr_groups)
> unconditionally, then userspace can see the interface even it doesn't
> work.

Use the is_visible() callback for your attribute group.

thanks,

greg k-h

2021-02-12 11:23:38

by Shuo Liu

Subject: Re: [PATCH] virt: acrn: Fix vCPU removing code build error

On Fri 12.Feb'21 at 12:02:18 +0100, Greg Kroah-Hartman wrote:
>On Fri, Feb 12, 2021 at 06:58:53PM +0800, Shuo A Liu wrote:
>> Hi Greg,
>>
>> On Fri 12.Feb'21 at 8:52:33 +0100, Greg Kroah-Hartman wrote:
>> > On Fri, Feb 12, 2021 at 12:57:24PM +0800, [email protected] wrote:
>> > > From: Shuo Liu <[email protected]>
>> > >
>> > > vCPU removing code depends on CONFIG_HOTPLUG_CPU as it uses remove_cpu()
>> > > and add_cpu(). Make the vCPU removing interface building with
>> > > CONFIG_HOTPLUG_CPU.
>> > >
>> > > ../drivers/virt/acrn/hsm.c: In function ‘remove_cpu_store’:
>> > > ../drivers/virt/acrn/hsm.c:389:3: error: implicit declaration of function ‘remove_cpu’; [-Werror=implicit-function-declaration]
>> > > remove_cpu(cpu);
>> > >
>> > > ../drivers/virt/acrn/hsm.c:402:2: error: implicit declaration of function ‘add_cpu’; [-Werror=implicit-function-declaration]
>> > > add_cpu(cpu);
>> > >
>> > > Fixes: 279dcf693ac7 ("virt: acrn: Introduce an interface for Service VM to control vCPU")
>> > > Reported-by: Randy Dunlap <[email protected]>
>> > > Acked-by: Randy Dunlap <[email protected]> # build-tested
>> > > Signed-off-by: Shuo Liu <[email protected]>
>> > > ---
>> > > drivers/virt/acrn/hsm.c | 4 ++++
>> > > 1 file changed, 4 insertions(+)
>> > >
>> > > diff --git a/drivers/virt/acrn/hsm.c b/drivers/virt/acrn/hsm.c
>> > > index 1f6b7c54a1a4..e340788aacdf 100644
>> > > --- a/drivers/virt/acrn/hsm.c
>> > > +++ b/drivers/virt/acrn/hsm.c
>> > > @@ -372,6 +372,7 @@ static int acrn_dev_release(struct inode *inode, struct file *filp)
>> > > return 0;
>> > > }
>> > >
>> > > +#ifdef CONFIG_HOTPLUG_CPU
>> > > static ssize_t remove_cpu_store(struct device *dev,
>> > > struct device_attribute *attr,
>> > > const char *buf, size_t count)
>> > > @@ -403,9 +404,12 @@ static ssize_t remove_cpu_store(struct device *dev,
>> > > return ret;
>> > > }
>> > > static DEVICE_ATTR_WO(remove_cpu);
>> > > +#endif
>> > >
>> > > static struct attribute *acrn_attrs[] = {
>> > > +#ifdef CONFIG_HOTPLUG_CPU
>> > > &dev_attr_remove_cpu.attr,
>> > > +#endif
>> > > NULL
>> > > };
>> > >
>> > >
>> >
>> > Shouldn't the real solution for this be that remove_cpu() and add_cpu()
>> > have function prototypes for when this is not enabled in the kernel
>> > build?
>>
>> Something like this in linux/cpu.h?
>>
>> diff --git a/include/linux/cpu.h b/include/linux/cpu.h
>> index 3aaa0687e8df..94a578a96202 100644
>> --- a/include/linux/cpu.h
>> +++ b/include/linux/cpu.h
>> @@ -108,6 +108,8 @@ static inline void cpu_maps_update_done(void)
>> {
>> }
>>
>> +static inline int add_cpu(unsigned int cpu) { return 0;}
>> +
>> #endif /* CONFIG_SMP */
>> extern struct bus_type cpu_subsys;
>>
>> @@ -137,6 +139,7 @@ static inline int cpus_read_trylock(void) { return
>> true; }
>> static inline void lockdep_assert_cpus_held(void) { }
>> static inline void cpu_hotplug_disable(void) { }
>> static inline void cpu_hotplug_enable(void) { }
>> +static inline int remove_cpu(unsigned int cpu) { return -EPERM; }
>> static inline void smp_shutdown_nonboot_cpus(unsigned int primary_cpu)
>> { }
>> #endif /* !CONFIG_HOTPLUG_CPU */
>>
>>
>> >
>> > Putting #ifdef in .c files like this is not a good idea at all.
>> >
>> > Then, at runtime, you can determine if you need to create this sysfs
>> > file or not, as you do not want to expose it to userspace if the kernel
>> > can not handle it, right?
>>
>> Right. I don't want to expose the sysfs to userspace if the kernel built
>> w/o CONFIG_HOTPLUG_CPU. But how to implement that if #ifdef is not used?
>> misc_register() creates sysfs with .groups (acrn_attr_groups)
>> unconditionally, then userspace can see the interface even it doesn't
>> work.
>
>Use the is_visible() callback for your attribute group.

Thanks for the guide, is_visible() is good. Seems good i can provide the
callback with
return IS_ENABLED(CONFIG_HOTPLUG_CPU);

Will update in v2.

Thanks
shuo