On Fri, Mar 22, 2013 at 05:17:38PM -0700, Andrew Honig wrote: > kvm_write_guest would work, but it will hurt performance a bit because > it'll be doing the address translation each time the time is updated, > which happens on most guest enters. > Time updates are rare, so this should no be an issue. Marcelo?
> Another possibility would be to change kvm_gfn_to_hva_cache_init to > accept a size parameter. If the requested range is all on one page > then it operates the same as it currently does. If the address range > is on more than one page then it falls back to kvm_write_guest. This > preserves the good performance for all cases that currently work, > while still supporting the unlikely case of page straddling requests. > It also makes it harder to write a security bugs for other callers of > kvm_gfn_to_hva_cache_init by explicitly requiring a size parameter. > > I can write a patch if you like the idea. Nice idea. Send a patch please. > > Andy > > On Fri, Mar 22, 2013 at 2:57 PM, Gleb Natapov <[email protected]> wrote: > > On Fri, Mar 22, 2013 at 06:19:47PM -0300, Marcelo Tosatti wrote: > >> > >> The alignment check is not necessary given that "KVM: x86: Convert > >> MSR_KVM_SYSTEM_TIME to use gfn_to_hva_cache" (0b79459b482e85cb742) > >> uses kvm_write_guest which is able to handle data across page > >> properly. > >> > > It uses kvm_write_guest_cached which calls to copy_to_user() directly. > > While this will not allow to overwrite page that does not belong to qemu > > process, it is possible to write outside of memory slot. May be we > > should use kvm_write_guest to do system time updates. > > > >> Signed-off-by: Marcelo Tosatti <[email protected]> > >> > >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > >> index f19ac0a..ec830fa 100644 > >> --- a/arch/x86/kvm/x86.c > >> +++ b/arch/x86/kvm/x86.c > >> @@ -1952,10 +1952,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, > >> struct msr_data *msr_info) > >> > >> gpa_offset = data & ~(PAGE_MASK | 1); > >> > >> - /* Check that the address is 32-byte aligned. */ > >> - if (gpa_offset & (sizeof(struct pvclock_vcpu_time_info) - 1)) > >> - break; > >> - > >> if (kvm_gfn_to_hva_cache_init(vcpu->kvm, > >> &vcpu->arch.pv_time, data & ~1ULL)) > >> vcpu->arch.pv_time_enabled = false; > > > > -- > > Gleb. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
