Hi David, Paolo, thanks for your reviews! On Thu, Sep 28, 2017 at 8:17 AM, David Hildenbrand <david@xxxxxxxxxx> wrote: > On 20.09.2017 19:42, Ken Hofsass wrote: > > - the register sets to be copied out out to kvm_run are selectable > > by userspace > > Why is this necessary? Why not simply store everything? And mark via > kvm_valid_regs which fields are actually valid? As Paolo surmised, it becomes expensive to store all register sets for every exit. > Also, I wonder if user space could simply modify (reduce) > vcpu->run->kvm_valid_regs to achieve the same behavior (when storing in > the kernel, simply check if the valid bit is set). I agree that we could overload kvm_valid_regs to be input/output rather than having the separate u64 sync_regs for input only. I will refactor for the next version of the patch. > > +For s390 specifics, please the source code. > "... please have a look at the source code."? > > > + > > +For x86: > > +- the capability can be enabled/disabled (s390 is always enabled). > Move that s390 comment to "For s390". Thanks for catching those, will incorporate. > You could simply always store and let user space control via > vcpu->run->kvm_valid_regs which ones to actually store. > This would get rid of the need for the capability. Yeah, looking at the code yet again, I saw your point that the ENABLE section of the capability code doesn't provide any new utility. I'll take that part out. However, I think keeping the QUERY part is needed for userspace to have positive confirmation of the capability since it is too expensive to forcibly store all the register sets. (And the capability value is already defined from the S390 version so there's no additional namespace pollution.) On Thu, Sep 28, 2017 at 8:34 AM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote: > I'm not sure which MSRs you'd want to store explicitly (e.g. efer is already parts of > sregs) though---Bjorn what are you using? I could be swayed to remove this for now (more below). > I'm also not sure about the debug_regs... In part for completeness but also to have them available in a non-interactive debugging/instrumentation scenario, e.g. when using KVM to support driver/kernel development work. > I like the patch, but I would > like to understand more how it's used (all I can guess is that it's > related to emulation in userspace). Thanks! And you're right that reducing overhead for userspace emulation (as presented by Andy Honig, et al at KVM forum) is part of the rationale. Similarly, we are also interested in building other low-level functionality in userspace to avoid expanding kernel-level attack surface. Toward that end, I expect to send you another patchset in the next few weeks that builds on SYNC_REGS. (Monitoring MSRs might make more sense when in that context.) > I would also like to have a testcase for kvm-unit-tests api/ in order to > commit it. Certainly! I'll get the test and the other fixes done and post a v2 patch. thanks again, Ken
