linux-kselftest.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Paolo Bonzini <pbonzini@redhat.com>
To: Jing Zhang <jingzhangos@google.com>, KVM <kvm@vger.kernel.org>,
	KVMARM <kvmarm@lists.cs.columbia.edu>,
	LinuxMIPS <linux-mips@vger.kernel.org>,
	KVMPPC <kvm-ppc@vger.kernel.org>,
	LinuxS390 <linux-s390@vger.kernel.org>,
	Linuxkselftest <linux-kselftest@vger.kernel.org>,
	Marc Zyngier <maz@kernel.org>, James Morse <james.morse@arm.com>,
	Julien Thierry <julien.thierry.kdev@gmail.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Will Deacon <will@kernel.org>,
	Huacai Chen <chenhuacai@kernel.org>,
	Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>,
	Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
	Paul Mackerras <paulus@ozlabs.org>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Janosch Frank <frankja@linux.ibm.com>,
	David Hildenbrand <david@redhat.com>,
	Cornelia Huck <cohuck@redhat.com>,
	Claudio Imbrenda <imbrenda@linux.ibm.com>,
	Sean Christopherson <seanjc@google.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Jim Mattson <jmattson@google.com>,
	Peter Shier <pshier@google.com>, Oliver Upton <oupton@google.com>,
	David Rientjes <rientjes@google.com>,
	Emanuele Giuseppe Esposito <eesposit@redhat.com>,
	David Matlack <dmatlack@google.com>,
	Ricardo Koller <ricarkol@google.com>,
	Krish Sadhukhan <krish.sadhukhan@oracle.com>,
	Fuad Tabba <tabba@google.com>,
	Greg KH <gregkh@linuxfoundation.org>
Subject: Re: [PATCH v12 2/7] KVM: stats: Add fd-based API to read binary stats data
Date: Mon, 21 Jun 2021 18:54:06 +0200	[thread overview]
Message-ID: <0cde024e-a234-9a10-5157-d17ba423939e@redhat.com> (raw)
In-Reply-To: <20210618222709.1858088-3-jingzhangos@google.com>

On 19/06/21 00:27, Jing Zhang wrote:
> +/**
> + * kvm_stats_read() - Common vm/vcpu stats read function to userspace.

Common function to read from the binary statistics file descriptor.

> + * @id: identification string of the stats
> + * @header: stats header for a vm or a vcpu
> + * @desc: start address of an array of stats descriptors for a vm or a vcpu
> + * @stats: start address of stats data block for a vm or a vcpu
> + * @size_stats: the size of stats data block pointed by @stats
> + * @user_buffer: start address of userspace buffer
> + * @size: requested read size from userspace
> + * @offset: the start position from which the content will be read for the
> + *          corresponding vm or vcp file descriptor
> + *
> + * The file content of a vm/vcpu file descriptor is now defined as below:
> + * +-------------+
> + * |   Header    |
> + * +-------------+
> + * |  id string  |
> + * +-------------+
> + * | Descriptors |
> + * +-------------+
> + * | Stats Data  |
> + * +-------------+
> + * Although this function allows userspace to read any amount of data (as long
> + * as in the limit) from any position, the typical usage would follow below
> + * steps:
> + * 1. Read header from offset 0. Get the offset of descriptors and stats data
> + *    and some other necessary information. This is a one-time work for the
> + *    lifecycle of the corresponding vm/vcpu stats fd.
> + * 2. Read id string from its offset. This is a one-time work for the lifecycle
> + *    of the corresponding vm/vcpu stats fd.
> + * 3. Read descriptors from its offset and discover all the stats by parsing
> + *    descriptors. This is a one-time work for the lifecycle of the
> + *    corresponding vm/vcpu stats fd.
> + * 4. Periodically read stats data from its offset using pread.
> + *
> + * Return: the number of bytes that has been successfully read
> + */
> +ssize_t kvm_stats_read(char *id, const struct kvm_stats_header *header,
> +		       const struct _kvm_stats_desc *desc,
> +		       void *stats, size_t size_stats,
> +		       char __user *user_buffer, size_t size, loff_t *offset)


You can replace the header argument with just the number of descriptors, 
and then construct the header in the "if" statement below that copies it 
to userspace:

const struct kvm_stats_header kvm_vm_stats_header = {
	.name_size = KVM_STATS_NAME_SIZE,
	.num_desc = num_desc,
	.id_offset = size_header,
	.desc_offset = size_header + KVM_STATS_NAME_SIZE,
	.data_offset = size_header + KVM_STATS_NAME_SIZE +
		       size_desc,
};

Of course size_header can be assigned with sizeof (struct kvm_stats_header).

This removes the definition of the header in each architecture.

Paolo

> +{
> +	ssize_t len;
> +	ssize_t copylen;
> +	ssize_t remain = size;
> +	size_t size_desc;
> +	size_t size_header;
> +	void *src;
> +	loff_t pos = *offset;
> +	char __user *dest = user_buffer;
> +
> +	size_header = sizeof(*header);
> +	size_desc = header->num_desc * sizeof(*desc);
> +
> +	len = KVM_STATS_NAME_SIZE + size_header + size_desc + size_stats - pos;
> +	len = min(len, remain);
> +	if (len <= 0)
> +		return 0;
> +	remain = len;
> +
> +	/*
> +	 * Copy kvm stats header.
> +	 * The header is the first block of content userspace usually read out.
> +	 * The pos is 0 and the copylen and remain would be the size of header.
> +	 * The copy of the header would be skipped if offset is larger than the
> +	 * size of header. That usually happens when userspace reads stats
> +	 * descriptors and stats data.
> +	 */
> +	copylen = size_header - pos;
> +	copylen = min(copylen, remain);
> +	if (copylen > 0) {
> +		src = (void *)header + pos;
> +		if (copy_to_user(dest, src, copylen))
> +			return -EFAULT;
> +		remain -= copylen;
> +		pos += copylen;
> +		dest += copylen;
> +	}
> +
> +	/*
> +	 * Copy kvm stats header id string.
> +	 * The id string is unique for every vm/vcpu, which is stored in kvm
> +	 * and kvm_vcpu structure.
> +	 * The id string is part of the stat header from the perspective of
> +	 * userspace, it is usually read out together with previous constant
> +	 * header part and could be skipped for later descriptors and stats
> +	 * data readings.
> +	 */
> +	copylen = size_header + KVM_STATS_NAME_SIZE - pos;

Should use header->id_offset instead of size_header here and in the 
computation of src.

> +	copylen = min(copylen, remain);
> +	if (copylen > 0) {
> +		src = id + pos - size_header;
> +		if (copy_to_user(dest, src, copylen))
> +			return -EFAULT;
> +		remain -= copylen;
> +		pos += copylen;
> +		dest += copylen;
> +	}
> +
> +	/*
> +	 * Copy kvm stats descriptors.
> +	 * The descriptors copy would be skipped in the typical case that
> +	 * userspace periodically read stats data, since the pos would be
> +	 * greater than the end address of descriptors
> +	 * (header->header.desc_offset + size_desc) causing copylen <= 0.
> +	 */
> +	copylen = header->desc_offset + size_desc - pos;
> +	copylen = min(copylen, remain);
> +	if (copylen > 0) {
> +		src = (void *)desc + pos - header->desc_offset;
> +		if (copy_to_user(dest, src, copylen))
> +			return -EFAULT;
> +		remain -= copylen;
> +		pos += copylen;
> +		dest += copylen;
> +	}
> +
> +	/* Copy kvm stats values */
> +	copylen = header->data_offset + size_stats - pos;
> +	copylen = min(copylen, remain);
> +	if (copylen > 0) {
> +		src = stats + pos - header->data_offset;
> +		if (copy_to_user(dest, src, copylen))
> +			return -EFAULT;
> +		remain -= copylen;
> +		pos += copylen;
> +		dest += copylen;
> +	}
> +
> +	*offset = pos;
> +	return len;
> +}


  reply	other threads:[~2021-06-21 16:55 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-18 22:27 [PATCH v12 0/7] KVM statistics data fd-based binary interface Jing Zhang
2021-06-18 22:27 ` [PATCH v12 1/7] KVM: stats: Separate generic stats from architecture specific ones Jing Zhang
2021-06-18 22:27 ` [PATCH v12 2/7] KVM: stats: Add fd-based API to read binary stats data Jing Zhang
2021-06-21 16:54   ` Paolo Bonzini [this message]
2021-06-21 17:46     ` Jing Zhang
2021-06-21 22:45       ` Paolo Bonzini
2021-06-21 22:58         ` Jing Zhang
2021-06-21 23:35           ` Paolo Bonzini
2021-06-21 23:38             ` Jing Zhang
2021-06-18 22:27 ` [PATCH v12 3/7] KVM: stats: Support binary stats retrieval for a VM Jing Zhang
2021-06-18 22:27 ` [PATCH v12 4/7] KVM: stats: Support binary stats retrieval for a VCPU Jing Zhang
2021-06-21 16:46   ` Paolo Bonzini
2021-06-21 17:42     ` Jing Zhang
2021-06-18 22:27 ` [PATCH v12 5/7] KVM: stats: Add documentation for binary statistics interface Jing Zhang
2021-06-18 22:27 ` [PATCH v12 6/7] KVM: selftests: Add selftest for KVM statistics data binary interface Jing Zhang
2021-06-18 22:27 ` [PATCH v12 7/7] KVM: stats: Remove code duplication for binary and debugfs stats Jing Zhang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0cde024e-a234-9a10-5157-d17ba423939e@redhat.com \
    --to=pbonzini@redhat.com \
    --cc=aleksandar.qemu.devel@gmail.com \
    --cc=borntraeger@de.ibm.com \
    --cc=chenhuacai@kernel.org \
    --cc=cohuck@redhat.com \
    --cc=david@redhat.com \
    --cc=dmatlack@google.com \
    --cc=eesposit@redhat.com \
    --cc=frankja@linux.ibm.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=imbrenda@linux.ibm.com \
    --cc=james.morse@arm.com \
    --cc=jingzhangos@google.com \
    --cc=jmattson@google.com \
    --cc=julien.thierry.kdev@gmail.com \
    --cc=krish.sadhukhan@oracle.com \
    --cc=kvm-ppc@vger.kernel.org \
    --cc=kvm@vger.kernel.org \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux-mips@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=maz@kernel.org \
    --cc=oupton@google.com \
    --cc=paulus@ozlabs.org \
    --cc=pshier@google.com \
    --cc=ricarkol@google.com \
    --cc=rientjes@google.com \
    --cc=seanjc@google.com \
    --cc=suzuki.poulose@arm.com \
    --cc=tabba@google.com \
    --cc=tsbogend@alpha.franken.de \
    --cc=vkuznets@redhat.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).