We got these coccinelle warning:
./arch/x86/kvm/debugfs.c:23:0-23: WARNING: vcpu_timer_advance_ns_fops
should be defined with DEFINE_DEBUGFS_ATTRIBUTE
./arch/x86/kvm/debugfs.c:32:0-23: WARNING: vcpu_tsc_offset_fops should
be defined with DEFINE_DEBUGFS_ATTRIBUTE
./arch/x86/kvm/debugfs.c:41:0-23: WARNING: vcpu_tsc_scaling_fops should
be defined with DEFINE_DEBUGFS_ATTRIBUTE
./arch/x86/kvm/debugfs.c:49:0-23: WARNING: vcpu_tsc_scaling_frac_fops
should be defined with DEFINE_DEBUGFS_ATTRIBUTE
Use DEFINE_DEBUGFS_ATTRIBUTE() rather than DEFINE_SIMPLE_ATTRIBUTE()
to fix this.
Signed-off-by: Yi Wang <[email protected]>
---
arch/x86/kvm/debugfs.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/arch/x86/kvm/debugfs.c b/arch/x86/kvm/debugfs.c
index 329361b..24016fb 100644
--- a/arch/x86/kvm/debugfs.c
+++ b/arch/x86/kvm/debugfs.c
@@ -20,7 +20,7 @@ static int vcpu_get_timer_advance_ns(void *data, u64 *val)
return 0;
}
-DEFINE_SIMPLE_ATTRIBUTE(vcpu_timer_advance_ns_fops, vcpu_get_timer_advance_ns, NULL, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(vcpu_timer_advance_ns_fops, vcpu_get_timer_advance_ns, NULL, "%llu\n");
static int vcpu_get_tsc_offset(void *data, u64 *val)
{
@@ -29,7 +29,7 @@ static int vcpu_get_tsc_offset(void *data, u64 *val)
return 0;
}
-DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_offset_fops, vcpu_get_tsc_offset, NULL, "%lld\n");
+DEFINE_DEBUGFS_ATTRIBUTE(vcpu_tsc_offset_fops, vcpu_get_tsc_offset, NULL, "%lld\n");
static int vcpu_get_tsc_scaling_ratio(void *data, u64 *val)
{
@@ -38,7 +38,7 @@ static int vcpu_get_tsc_scaling_ratio(void *data, u64 *val)
return 0;
}
-DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_scaling_fops, vcpu_get_tsc_scaling_ratio, NULL, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(vcpu_tsc_scaling_fops, vcpu_get_tsc_scaling_ratio, NULL, "%llu\n");
static int vcpu_get_tsc_scaling_frac_bits(void *data, u64 *val)
{
@@ -46,20 +46,20 @@ static int vcpu_get_tsc_scaling_frac_bits(void *data, u64 *val)
return 0;
}
-DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_scaling_frac_fops, vcpu_get_tsc_scaling_frac_bits, NULL, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(vcpu_tsc_scaling_frac_fops, vcpu_get_tsc_scaling_frac_bits, NULL, "%llu\n");
int kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
{
struct dentry *ret;
- ret = debugfs_create_file("tsc-offset", 0444,
+ ret = debugfs_create_file_unsafe("tsc-offset", 0444,
vcpu->debugfs_dentry,
vcpu, &vcpu_tsc_offset_fops);
if (!ret)
return -ENOMEM;
if (lapic_in_kernel(vcpu)) {
- ret = debugfs_create_file("lapic_timer_advance_ns", 0444,
+ ret = debugfs_create_file_unsafe("lapic_timer_advance_ns", 0444,
vcpu->debugfs_dentry,
vcpu, &vcpu_timer_advance_ns_fops);
if (!ret)
@@ -67,12 +67,12 @@ int kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
}
if (kvm_has_tsc_control) {
- ret = debugfs_create_file("tsc-scaling-ratio", 0444,
+ ret = debugfs_create_file_unsafe("tsc-scaling-ratio", 0444,
vcpu->debugfs_dentry,
vcpu, &vcpu_tsc_scaling_fops);
if (!ret)
return -ENOMEM;
- ret = debugfs_create_file("tsc-scaling-ratio-frac-bits", 0444,
+ ret = debugfs_create_file_unsafe("tsc-scaling-ratio-frac-bits", 0444,
vcpu->debugfs_dentry,
vcpu, &vcpu_tsc_scaling_frac_fops);
if (!ret)
--
1.8.3.1
On Tue, Sep 17, 2019 at 07:18:33PM +0200, Paolo Bonzini wrote:
> On 22/07/19 09:33, Yi Wang wrote:
> > We got these coccinelle warning:
> > ./arch/x86/kvm/debugfs.c:23:0-23: WARNING: vcpu_timer_advance_ns_fops
> > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > ./arch/x86/kvm/debugfs.c:32:0-23: WARNING: vcpu_tsc_offset_fops should
> > be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > ./arch/x86/kvm/debugfs.c:41:0-23: WARNING: vcpu_tsc_scaling_fops should
> > be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > ./arch/x86/kvm/debugfs.c:49:0-23: WARNING: vcpu_tsc_scaling_frac_fops
> > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> >
> > Use DEFINE_DEBUGFS_ATTRIBUTE() rather than DEFINE_SIMPLE_ATTRIBUTE()
> > to fix this.
> >
> > Signed-off-by: Yi Wang <[email protected]>
>
> It sucks though that you have to use a function with "unsafe" in the name.
I agree, why make this change?
> Greg, is the patch doing the right thing?
I can't tell. What coccinelle script generated this patch?
thanks,
greg k-h
On 22/07/19 09:33, Yi Wang wrote:
> We got these coccinelle warning:
> ./arch/x86/kvm/debugfs.c:23:0-23: WARNING: vcpu_timer_advance_ns_fops
> should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> ./arch/x86/kvm/debugfs.c:32:0-23: WARNING: vcpu_tsc_offset_fops should
> be defined with DEFINE_DEBUGFS_ATTRIBUTE
> ./arch/x86/kvm/debugfs.c:41:0-23: WARNING: vcpu_tsc_scaling_fops should
> be defined with DEFINE_DEBUGFS_ATTRIBUTE
> ./arch/x86/kvm/debugfs.c:49:0-23: WARNING: vcpu_tsc_scaling_frac_fops
> should be defined with DEFINE_DEBUGFS_ATTRIBUTE
>
> Use DEFINE_DEBUGFS_ATTRIBUTE() rather than DEFINE_SIMPLE_ATTRIBUTE()
> to fix this.
>
> Signed-off-by: Yi Wang <[email protected]>
It sucks though that you have to use a function with "unsafe" in the name.
Greg, is the patch doing the right thing?
Paolo
> ---
> arch/x86/kvm/debugfs.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kvm/debugfs.c b/arch/x86/kvm/debugfs.c
> index 329361b..24016fb 100644
> --- a/arch/x86/kvm/debugfs.c
> +++ b/arch/x86/kvm/debugfs.c
> @@ -20,7 +20,7 @@ static int vcpu_get_timer_advance_ns(void *data, u64 *val)
> return 0;
> }
>
> -DEFINE_SIMPLE_ATTRIBUTE(vcpu_timer_advance_ns_fops, vcpu_get_timer_advance_ns, NULL, "%llu\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(vcpu_timer_advance_ns_fops, vcpu_get_timer_advance_ns, NULL, "%llu\n");
>
> static int vcpu_get_tsc_offset(void *data, u64 *val)
> {
> @@ -29,7 +29,7 @@ static int vcpu_get_tsc_offset(void *data, u64 *val)
> return 0;
> }
>
> -DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_offset_fops, vcpu_get_tsc_offset, NULL, "%lld\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(vcpu_tsc_offset_fops, vcpu_get_tsc_offset, NULL, "%lld\n");
>
> static int vcpu_get_tsc_scaling_ratio(void *data, u64 *val)
> {
> @@ -38,7 +38,7 @@ static int vcpu_get_tsc_scaling_ratio(void *data, u64 *val)
> return 0;
> }
>
> -DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_scaling_fops, vcpu_get_tsc_scaling_ratio, NULL, "%llu\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(vcpu_tsc_scaling_fops, vcpu_get_tsc_scaling_ratio, NULL, "%llu\n");
>
> static int vcpu_get_tsc_scaling_frac_bits(void *data, u64 *val)
> {
> @@ -46,20 +46,20 @@ static int vcpu_get_tsc_scaling_frac_bits(void *data, u64 *val)
> return 0;
> }
>
> -DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_scaling_frac_fops, vcpu_get_tsc_scaling_frac_bits, NULL, "%llu\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(vcpu_tsc_scaling_frac_fops, vcpu_get_tsc_scaling_frac_bits, NULL, "%llu\n");
>
> int kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
> {
> struct dentry *ret;
>
> - ret = debugfs_create_file("tsc-offset", 0444,
> + ret = debugfs_create_file_unsafe("tsc-offset", 0444,
> vcpu->debugfs_dentry,
> vcpu, &vcpu_tsc_offset_fops);
> if (!ret)
> return -ENOMEM;
>
> if (lapic_in_kernel(vcpu)) {
> - ret = debugfs_create_file("lapic_timer_advance_ns", 0444,
> + ret = debugfs_create_file_unsafe("lapic_timer_advance_ns", 0444,
> vcpu->debugfs_dentry,
> vcpu, &vcpu_timer_advance_ns_fops);
> if (!ret)
> @@ -67,12 +67,12 @@ int kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
> }
>
> if (kvm_has_tsc_control) {
> - ret = debugfs_create_file("tsc-scaling-ratio", 0444,
> + ret = debugfs_create_file_unsafe("tsc-scaling-ratio", 0444,
> vcpu->debugfs_dentry,
> vcpu, &vcpu_tsc_scaling_fops);
> if (!ret)
> return -ENOMEM;
> - ret = debugfs_create_file("tsc-scaling-ratio-frac-bits", 0444,
> + ret = debugfs_create_file_unsafe("tsc-scaling-ratio-frac-bits", 0444,
> vcpu->debugfs_dentry,
> vcpu, &vcpu_tsc_scaling_frac_fops);
> if (!ret)
>
On 17/09/19 20:12, Greg Kroah-Hartman wrote:
> On Tue, Sep 17, 2019 at 07:18:33PM +0200, Paolo Bonzini wrote:
>> On 22/07/19 09:33, Yi Wang wrote:
>>> We got these coccinelle warning:
>>> ./arch/x86/kvm/debugfs.c:23:0-23: WARNING: vcpu_timer_advance_ns_fops
>>> should be defined with DEFINE_DEBUGFS_ATTRIBUTE
>>> ./arch/x86/kvm/debugfs.c:32:0-23: WARNING: vcpu_tsc_offset_fops should
>>> be defined with DEFINE_DEBUGFS_ATTRIBUTE
>>> ./arch/x86/kvm/debugfs.c:41:0-23: WARNING: vcpu_tsc_scaling_fops should
>>> be defined with DEFINE_DEBUGFS_ATTRIBUTE
>>> ./arch/x86/kvm/debugfs.c:49:0-23: WARNING: vcpu_tsc_scaling_frac_fops
>>> should be defined with DEFINE_DEBUGFS_ATTRIBUTE
>>>
>>> Use DEFINE_DEBUGFS_ATTRIBUTE() rather than DEFINE_SIMPLE_ATTRIBUTE()
>>> to fix this.
>>>
>>> Signed-off-by: Yi Wang <[email protected]>
>>
>> It sucks though that you have to use a function with "unsafe" in the name.
>
> I agree, why make this change?
>
>> Greg, is the patch doing the right thing?
>
> I can't tell. What coccinelle script generated this patch?
Seems to be scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci.
//# Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file()
//# imposes some significant overhead as compared to
//# DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe().
Paolo
> thanks,
>
> greg k-h
>
On Wed, Sep 18, 2019 at 08:19:44AM +0800, [email protected] wrote:
> Hi Paolo,
>
> > On 22/07/19 09:33, Yi Wang wrote:
> > > We got these coccinelle warning:
> > > ./arch/x86/kvm/debugfs.c:23:0-23: WARNING: vcpu_timer_advance_ns_fops
> > > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > > ./arch/x86/kvm/debugfs.c:32:0-23: WARNING: vcpu_tsc_offset_fops should
> > > be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > > ./arch/x86/kvm/debugfs.c:41:0-23: WARNING: vcpu_tsc_scaling_fops should
> > > be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > > ./arch/x86/kvm/debugfs.c:49:0-23: WARNING: vcpu_tsc_scaling_frac_fops
> > > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > >
> > > Use DEFINE_DEBUGFS_ATTRIBUTE() rather than DEFINE_SIMPLE_ATTRIBUTE()
> > > to fix this.
> > >
> > > Signed-off-by: Yi Wang <[email protected]>
> >
> > It sucks though that you have to use a function with "unsafe" in the name.
>
> Yes, it does, but I found some patches in the git log:
> https://git.kernel.org/pub/scm/virt/kvm/kvm.git/log/?qt=grep&q=DEFINE_DEBUGFS_ATTRIBUTE+
>
> And, do you think the function name "debugfs_create_file_unsafe" is not proper?
Only if you _KNOW_ you are creating/removing these files in a way that
is safe is it ok to use these calls. Hint, what is your locking
strategy for when these files are removed?
Is that the case here? If not, please stick with what is there today,
as we know it works, and it is "safe" to do so.
thanks,
greg k-h