From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Date: Thu, 17 Jun 2021 11:23:35 +0000 Subject: Re: [PATCH v10 2/5] KVM: stats: Add fd-based API to read binary stats data Message-Id: List-Id: References: <20210617044146.2667540-1-jingzhangos@google.com> <20210617044146.2667540-3-jingzhangos@google.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Greg KH , Jing Zhang Cc: KVM , KVMARM , LinuxMIPS , KVMPPC , LinuxS390 , Linuxkselftest , Marc Zyngier , James Morse , Julien Thierry , Suzuki K Poulose , Will Deacon , Huacai Chen , Aleksandar Markovic , Thomas Bogendoerfer , Paul Mackerras , Christian Borntraeger , Janosch Frank , David Hildenbrand , Cornelia Huck , Claudio Imbrenda , Sean Christopherson , Vitaly Kuznetsov , Jim Mattson , Peter Shier , Oliver Upton , David Rientjes , Emanuele Giuseppe Esposito , David Matlack , Ricardo Koller , Krish Sadhukhan , Fuad Tabba On 17/06/21 09:03, Greg KH wrote: >> 3. Fd-based solution provides the possibility that a telemetry can >> read KVM stats in a less privileged situation. > "possiblity"? Does this work or not? Have you tested it? > I think this is essentially s/that/for/. But more precisely: 3. Compared for example to a ioctl, a separate file descriptor makes it possible for an external program to read statistics, while maintaining privilege separation between VMM and telemetry code. >> >> + snprintf(&kvm_vm_stats_header.id[0], sizeof(kvm_vm_stats_header.id), >> + "kvm-%d", task_pid_nr(current)); > > Why do you write to this static variable for EVERY read? Shouldn't you > just do it once at open? How can it change? > > Wait, it's a single shared variable, what happens when multiple tasks > open this thing and read from it? You race between writing to this > variable here and then: > >> + return kvm_stats_read(&kvm_vm_stats_header, &kvm_vm_stats_desc[0], >> + &kvm->stat, sizeof(kvm->stat), user_buffer, size, offset); > > Accessing it here. > > So how is this really working? It's not - Jing, kvm_vm_stats_header is small enough that you can store a copy in struct kvm. Paolo