Re: [PATCH kernel] KVM: PPC: Book3S: Merge powerpc's debugfs entry content into generic entry

On 04/09/2021 00:28, Fabiano Rosas wrote:

Alexey Kardashevskiy [off-list ref] writes:

quoted

At the moment the generic KVM code creates an "%pid-%fd" entry per a KVM
instance; and the PPC HV KVM creates its own at "vm%pid".

The rproblems with the PPC entries are:
1. they do not allow multiple VMs in the same process (which is extremely
rare case mostly used by syzkaller fuzzer);
2. prone to race bugs like the generic KVM code had fixed in
commit 85cd39af14f4 ("KVM: Do not leak memory for duplicate debugfs
directories").

This defines kvm_arch_create_kvm_debugfs() similar to one for vcpus.

I think kvm_arch_create_vm_debugfs is a bit mode accurate?

ah yes, it is better.

                         ^

quoted

This defines 2 hooks in kvmppc_ops for allowing specific KVM
implementations to add necessary entries.

This makes use of already existing kvm_arch_create_vcpu_debugfs.

This removes no more used debugfs_dir pointers from PPC kvm_arch structs.

Suggested-by: Fabiano Rosas <redacted>
Signed-off-by: Alexey Kardashevskiy <redacted>

...

quoted

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index c8f12b056968..325b388c725a 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c

@@ -2771,19 +2771,14 @@ static const struct file_operations debugfs_timings_ops = {
  };
  
  /* Create a debugfs directory for the vcpu */
-static void debugfs_vcpu_init(struct kvm_vcpu *vcpu, unsigned int id)
+static void kvmppc_arch_create_vcpu_debugfs_hv(struct kvm_vcpu *vcpu, struct dentry *debugfs_dentry)

This could lose the 'arch' since it is already inside our code and
accessed only via ops. I see that we already have a
kvmppc_create_vcpu_debugfs that's used for some BookE processor, this

Ouch, missed kvmppc_create_vcpu_debugfs(). Good eye :)

would make:

kvmppc_create_vcpu_debugfs
kvmppc_create_vcpu_debugfs_hv
kvmppc_create_vcpu_debugfs_pr (possibly)

which perhaps is more consistent.

Or  kvm_arch_vm_ioctl_hv(). I really like having "arch" in the name, 
tells right away what it is about. "kvmppc" might be excessive. Thanks,

quoted

  {
-	char buf[16];
-	struct kvm *kvm = vcpu->kvm;
-
-	snprintf(buf, sizeof(buf), "vcpu%u", id);
-	vcpu->arch.debugfs_dir = debugfs_create_dir(buf, kvm->arch.debugfs_dir);
-	debugfs_create_file("timings", 0444, vcpu->arch.debugfs_dir, vcpu,
+	debugfs_create_file("timings", 0444, debugfs_dentry, vcpu,
  			    &debugfs_timings_ops);
  }
  
  #else /* CONFIG_KVM_BOOK3S_HV_EXIT_TIMING */
-static void debugfs_vcpu_init(struct kvm_vcpu *vcpu, unsigned int id)
+static void kvmppc_arch_create_vcpu_debugfs_hv(struct kvm_vcpu *vcpu, struct dentry *debugfs_dentry)
  {
  }
  #endif /* CONFIG_KVM_BOOK3S_HV_EXIT_TIMING */

-- 
Alexey