On 8/2/23 21:18, Sean Christopherson wrote: > On Tue, Aug 01, 2023, Michal Luczaj wrote: >> All right, so assuming the revert is not happening and the API is not misused >> (i.e. unless vcpu->run->kvm_valid_regs is set, no one is expecting up to date >> values in vcpu->run->s.regs), is assignment copying >> >> struct kvm_vcpu_events events = vcpu->run->s.regs.events; >> >> the right approach or should it be a memcpy(), like in ioctl handlers? > > Both approaches are fine, though I am gaining a preference for the copy-by-value > method. With gcc-12 and probably most compilers, the code generation is identical > for both as the compiler generates a call to memcpy() to handle the the struct > assignment. > > The advantage of copy-by-value for structs, and why I think I now prefer it, is > that it provides type safety. E.g. this compiles without complaint > > memcpy(&events, &vcpu->run->s.regs.sregs, sizeof(events)); > > whereas this > > struct kvm_vcpu_events events = vcpu->run->s.regs.sregs; > > yields > > arch/x86/kvm/x86.c: In function ‘sync_regs’: > arch/x86/kvm/x86.c:11793:49: error: invalid initializer > 11793 | struct kvm_vcpu_events events = vcpu->run->s.regs.sregs; > | ^~~~ > > The downside is that it's less obvious when reading the code that there is a > large-ish memcpy happening, but IMO it's worth gaining the type safety. Sure, that makes sense. I was a bit concerned how a padding within a struct might affect performance of such copy-by-value, but (obviously?) there's no padding in kvm_sregs, nor kvm_vcpu_events... Anyway, while there, could you take a look at __set_sregs_common()? *mmu_reset_needed |= kvm_read_cr0(vcpu) != sregs->cr0; static_call(kvm_x86_set_cr0)(vcpu, sregs->cr0); vcpu->arch.cr0 = sregs->cr0; That last assignment seems redundant as both vmx_set_cr0() and svm_set_cr0() take care of it, but I may be missing something (even if selftests pass with that line removed). thanks, Michal
