Re: [PATCH 12/23] KVM: PPC: Book3S HV: Create debugfs file for each guest's HPT

[Alexander Graf <agraf@suse.de>]

On 20.03.15 10:39, Paul Mackerras wrote:
> This creates a debugfs directory for each HV guest (assuming debugfs
> is enabled in the kernel config), and within that directory, a file
> by which the contents of the guest's HPT (hashed page table) can be
> read.  The directory is named vmnnnn, where nnnn is the PID of the
> process that created the guest.  The file is named "htab".  This is
> intended to help in debugging problems in the host's management
> of guest memory.
> 
> The contents of the file consist of a series of lines like this:
> 
>   3f48 4000d032bf003505 0000000bd7ff1196 00000003b5c71196
> 
> The first field is the index of the entry in the HPT, the second and
> third are the HPT entry, so the third entry contains the real page
> number that is mapped by the entry if the entry's valid bit is set.
> The fourth field is the guest's view of the second doubleword of the
> entry, so it contains the guest physical address.  (The format of the
> second through fourth fields are described in the Power ISA and also
> in arch/powerpc/include/asm/mmu-hash64.h.)
> 
> Signed-off-by: Paul Mackerras <paulus@samba.org>
> ---
>  arch/powerpc/include/asm/kvm_book3s_64.h |   2 +
>  arch/powerpc/include/asm/kvm_host.h      |   2 +
>  arch/powerpc/kvm/book3s_64_mmu_hv.c      | 136 +++++++++++++++++++++++++++++++
>  arch/powerpc/kvm/book3s_hv.c             |  12 +++
>  virt/kvm/kvm_main.c                      |   1 +
>  5 files changed, 153 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
> index 0789a0f..869c53f 100644
> --- a/arch/powerpc/include/asm/kvm_book3s_64.h
> +++ b/arch/powerpc/include/asm/kvm_book3s_64.h
> @@ -436,6 +436,8 @@ static inline struct kvm_memslots *kvm_memslots_raw(struct kvm *kvm)
>  	return rcu_dereference_raw_notrace(kvm->memslots);
>  }
>  
> +extern void kvmppc_mmu_debugfs_init(struct kvm *kvm);
> +
>  #endif /* CONFIG_KVM_BOOK3S_HV_POSSIBLE */
>  
>  #endif /* __ASM_KVM_BOOK3S_64_H__ */
> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
> index 015773f..f1d0bbc 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -238,6 +238,8 @@ struct kvm_arch {
>  	atomic_t hpte_mod_interest;
>  	cpumask_t need_tlb_flush;
>  	int hpt_cma_alloc;
> +	struct dentry *debugfs_dir;
> +	struct dentry *htab_dentry;
>  #endif /* CONFIG_KVM_BOOK3S_HV_POSSIBLE */
>  #ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE
>  	struct mutex hpt_mutex;
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index 6c6825a..d6fe308 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -27,6 +27,7 @@
>  #include <linux/srcu.h>
>  #include <linux/anon_inodes.h>
>  #include <linux/file.h>
> +#include <linux/debugfs.h>
>  
>  #include <asm/tlbflush.h>
>  #include <asm/kvm_ppc.h>
> @@ -1490,6 +1491,141 @@ int kvm_vm_ioctl_get_htab_fd(struct kvm *kvm, struct kvm_get_htab_fd *ghf)
>  	return ret;
>  }
>  
> +struct debugfs_htab_state {
> +	struct kvm	*kvm;
> +	struct mutex	mutex;
> +	unsigned long	hpt_index;
> +	int		chars_left;
> +	int		buf_index;
> +	char		buf[64];
> +};
> +
> +static int debugfs_htab_open(struct inode *inode, struct file *file)
> +{
> +	struct kvm *kvm = inode->i_private;
> +	struct debugfs_htab_state *p;
> +
> +	p = kzalloc(sizeof(*p), GFP_KERNEL);
> +	if (!p)
> +		return -ENOMEM;
> +
> +	kvm_get_kvm(kvm);
> +	p->kvm = kvm;
> +	mutex_init(&p->mutex);
> +	file->private_data = p;
> +
> +	return nonseekable_open(inode, file);
> +}
> +
> +static int debugfs_htab_release(struct inode *inode, struct file *file)
> +{
> +	struct debugfs_htab_state *p = file->private_data;
> +
> +	kvm_put_kvm(p->kvm);
> +	kfree(p);
> +	return 0;
> +}
> +
> +static ssize_t debugfs_htab_read(struct file *file, char __user *buf,
> +				 size_t len, loff_t *ppos)
> +{
> +	struct debugfs_htab_state *p = file->private_data;
> +	ssize_t ret, r;
> +	unsigned long i, n;
> +	unsigned long v, hr, gr;
> +	struct kvm *kvm;
> +	__be64 *hptp;
> +
> +	ret = mutex_lock_interruptible(&p->mutex);
> +	if (ret)
> +		return ret;
> +
> +	if (p->chars_left) {
> +		n = p->chars_left;
> +		if (n > len)
> +			n = len;
> +		r = copy_to_user(buf, p->buf + p->buf_index, n);
> +		n -= r;
> +		p->chars_left -= n;
> +		p->buf_index += n;
> +		buf += n;
> +		len -= n;
> +		ret = n;
> +		if (r) {
> +			if (!n)
> +				ret = -EFAULT;
> +			goto out;
> +		}
> +	}
> +
> +	kvm = p->kvm;
> +	i = p->hpt_index;
> +	hptp = (__be64 *)(kvm->arch.hpt_virt + (i * HPTE_SIZE));
> +	for (; len != 0 && i < kvm->arch.hpt_npte; ++i, hptp += 2) {
> +		if (!(be64_to_cpu(hptp[0]) & (HPTE_V_VALID | HPTE_V_ABSENT)))
> +			continue;
> +
> +		/* lock the HPTE so it's stable and read it */
> +		preempt_disable();
> +		while (!try_lock_hpte(hptp, HPTE_V_HVLOCK))
> +			cpu_relax();
> +		v = be64_to_cpu(hptp[0]) & ~HPTE_V_HVLOCK;
> +		hr = be64_to_cpu(hptp[1]);
> +		gr = kvm->arch.revmap[i].guest_rpte;
> +		unlock_hpte(hptp, v);
> +		preempt_enable();
> +
> +		if (!(v & (HPTE_V_VALID | HPTE_V_ABSENT)))
> +			continue;
> +
> +		n = scnprintf(p->buf, sizeof(p->buf),
> +			      "%6lx %.16lx %.16lx %.16lx\n",
> +			      i, v, hr, gr);
> +		p->chars_left = n;
> +		if (n > len)
> +			n = len;
> +		r = copy_to_user(buf, p->buf, n);
> +		n -= r;
> +		p->chars_left -= n;
> +		p->buf_index = n;
> +		buf += n;
> +		len -= n;
> +		ret += n;
> +		if (r) {
> +			if (!ret)
> +				ret = -EFAULT;
> +			goto out;
> +		}
> +	}
> +	p->hpt_index = i;
> +
> + out:
> +	mutex_unlock(&p->mutex);
> +	return ret;
> +}
> +
> +ssize_t debugfs_htab_write(struct file *file, const char __user *buf,
> +			   size_t len, loff_t *ppos)
> +{
> +	return -EACCES;
> +}
> +
> +static const struct file_operations debugfs_htab_fops = {
> +	.owner	 = THIS_MODULE,
> +	.open	 = debugfs_htab_open,
> +	.release = debugfs_htab_release,
> +	.read	 = debugfs_htab_read,
> +	.write	 = debugfs_htab_write,
> +	.llseek	 = generic_file_llseek,
> +};
> +
> +void kvmppc_mmu_debugfs_init(struct kvm *kvm)
> +{
> +	kvm->arch.htab_dentry = debugfs_create_file("htab", 0400,
> +						    kvm->arch.debugfs_dir, kvm,
> +						    &debugfs_htab_fops);
> +}
> +
>  void kvmppc_mmu_book3s_hv_init(struct kvm_vcpu *vcpu)
>  {
>  	struct kvmppc_mmu *mmu = &vcpu->arch.mmu;
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 7b7102a..6631b53 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -32,6 +32,7 @@
>  #include <linux/page-flags.h>
>  #include <linux/srcu.h>
>  #include <linux/miscdevice.h>
> +#include <linux/debugfs.h>
>  
>  #include <asm/reg.h>
>  #include <asm/cputable.h>
> @@ -2307,6 +2308,7 @@ static int kvmppc_hv_setup_htab_rma(struct kvm_vcpu *vcpu)
>  static int kvmppc_core_init_vm_hv(struct kvm *kvm)
>  {
>  	unsigned long lpcr, lpid;
> +	char buf[32];
>  
>  	/* Allocate the guest's logical partition ID */
>  
> @@ -2347,6 +2349,14 @@ static int kvmppc_core_init_vm_hv(struct kvm *kvm)
>  	 */
>  	kvm_hv_vm_activated();
>  
> +	/*
> +	 * Create a debugfs directory for the VM
> +	 */
> +	sprintf(buf, "vm%d", current->pid);

The same comment stands here. I don't see how you could possibly overrun
32 bytes with vm%d, but I'd feel a lot safer if this was an snprintf.


Alex