Re: [PATCH v9 2/5] KVM: stats: Add fd-based API to read binary stats data

Paolo Bonzini
2021-06-16T17:12:37Z

On 14/06/21 23:21, Jing Zhang wrote:
> + /* Copy kvm stats values */
> + copylen = header->header.data_offset + size_stats - pos;
> + copylen = min(copylen, remain);
> + if (copylen > 0) {
> + src = stats + pos - header->header.data_offset;
> + if (copy_to_user(dest, src, copylen))
> + return -EFAULT;
> + remain -= copylen;
> + pos += copylen;
> + dest += copylen;
> + }

Hi Jing,

this code is causing usercopy warnings because the statistics are not
part of the vcpu slab's usercopy region. You need to move struct
kvm_vcpu_stat next to struct kvm_vcpu_arch, and adjust the call to
kmem_cache_create_usercopy in kvm_init.

Can you post a new version of the series, and while you are at it
explain the rationale for binary stats in the commit message for this
patch? This should include:

- the problem statement (e.g. frequency of the accesses)

- what are the benefits compared to debugfs

- why the schema is included in the file descriptor as well

You can probably find a lot or all of the information in my emails from
the last couple days, but you might also have other breadcrumbs from
Google's internal implementation of binary stats.

Thanks,

Paolo