| Message ID | 20200203230911.39755-2-bgardon@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] kvm: mmu: Replace unsigned with unsigned int for PTE access | expand |
Ben Gardon <bgardon@google.com> writes: > Separate the functions for generating MMIO page table entries from the > function that inserts them into the paging structure. This refactoring > will facilitate changes to the MMU sychronization model to use atomic > compare / exchanges (which are not guaranteed to succeed) instead of a > monolithic MMU lock. > > No functional change expected. > > Tested by running kvm-unit-tests on an Intel Haswell machine. This > commit introduced no new failures. > > This commit can be viewed in Gerrit at: > https://linux-review.googlesource.com/c/virt/kvm/kvm/+/2359 > > Signed-off-by: Ben Gardon <bgardon@google.com> > Reviewed-by: Oliver Upton <oupton@google.com> > Reviewed-by: Peter Shier <pshier@google.com> > --- > arch/x86/kvm/mmu/mmu.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index a9c593dec49bf..b81010d0edae1 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -451,9 +451,9 @@ static u64 get_mmio_spte_generation(u64 spte) > return gen; > } > > -static void mark_mmio_spte(struct kvm_vcpu *vcpu, u64 *sptep, u64 gfn, > - unsigned int access) > +static u64 make_mmio_spte(struct kvm_vcpu *vcpu, u64 gfn, unsigned int access) > { > + Unneded newline. > u64 gen = kvm_vcpu_memslots(vcpu)->generation & MMIO_SPTE_GEN_MASK; > u64 mask = generation_mmio_spte_mask(gen); > u64 gpa = gfn << PAGE_SHIFT; > @@ -464,6 +464,17 @@ static void mark_mmio_spte(struct kvm_vcpu *vcpu, u64 *sptep, u64 gfn, > mask |= (gpa & shadow_nonpresent_or_rsvd_mask) > << shadow_nonpresent_or_rsvd_mask_len; > > + return mask; > +} > + > +static void mark_mmio_spte(struct kvm_vcpu *vcpu, u64 *sptep, u64 gfn, > + unsigned int access) > +{ > + u64 mask = make_mmio_spte(vcpu, gfn, access); > + unsigned int gen = get_mmio_spte_generation(mask); > + > + access = mask & ACC_ALL; > + > trace_mark_mmio_spte(sptep, gfn, access, gen); 'access' and 'gen' are only being used for tracing, would it rather make sense to rename&move it to the newly introduced make_mmio_spte()? Or do we actually need tracing for both? Also, I dislike re-purposing function parameters. > mmu_spte_set(sptep, mask); > }
On 05/02/20 14:37, Vitaly Kuznetsov wrote: >> +static void mark_mmio_spte(struct kvm_vcpu *vcpu, u64 *sptep, u64 gfn, >> + unsigned int access) >> +{ >> + u64 mask = make_mmio_spte(vcpu, gfn, access); >> + unsigned int gen = get_mmio_spte_generation(mask); >> + >> + access = mask & ACC_ALL; >> + >> trace_mark_mmio_spte(sptep, gfn, access, gen); > 'access' and 'gen' are only being used for tracing, would it rather make > sense to rename&move it to the newly introduced make_mmio_spte()? Or do > we actually need tracing for both? You would have the same issue with sptep. > Also, I dislike re-purposing function parameters. Yes, "trace_mark_mmio_spte(sptep, gfn, mask & ACC_ALL, gen);" is slightly better. Paolo
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index a9c593dec49bf..b81010d0edae1 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -451,9 +451,9 @@ static u64 get_mmio_spte_generation(u64 spte) return gen; } -static void mark_mmio_spte(struct kvm_vcpu *vcpu, u64 *sptep, u64 gfn, - unsigned int access) +static u64 make_mmio_spte(struct kvm_vcpu *vcpu, u64 gfn, unsigned int access) { + u64 gen = kvm_vcpu_memslots(vcpu)->generation & MMIO_SPTE_GEN_MASK; u64 mask = generation_mmio_spte_mask(gen); u64 gpa = gfn << PAGE_SHIFT; @@ -464,6 +464,17 @@ static void mark_mmio_spte(struct kvm_vcpu *vcpu, u64 *sptep, u64 gfn, mask |= (gpa & shadow_nonpresent_or_rsvd_mask) << shadow_nonpresent_or_rsvd_mask_len; + return mask; +} + +static void mark_mmio_spte(struct kvm_vcpu *vcpu, u64 *sptep, u64 gfn, + unsigned int access) +{ + u64 mask = make_mmio_spte(vcpu, gfn, access); + unsigned int gen = get_mmio_spte_generation(mask); + + access = mask & ACC_ALL; + trace_mark_mmio_spte(sptep, gfn, access, gen); mmu_spte_set(sptep, mask); }