| Message ID | 1368476500-20031-3-git-send-email-sanjayl@kymasys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 13, 2013 at 01:21:40PM -0700, Sanjay Lal wrote: > Explain kvm_mips_map_page() changes here please. The text from the cover letter does not go into git history. > Signed-off-by: Sanjay Lal <sanjayl@kymasys.com> > --- > arch/mips/kvm/kvm_tlb.c | 38 +++++++++++++++++++++++++++++--------- > 1 file changed, 29 insertions(+), 9 deletions(-) > > diff --git a/arch/mips/kvm/kvm_tlb.c b/arch/mips/kvm/kvm_tlb.c > index 89511a9..218075f 100644 > --- a/arch/mips/kvm/kvm_tlb.c > +++ b/arch/mips/kvm/kvm_tlb.c > @@ -16,7 +16,10 @@ > #include <linux/mm.h> > #include <linux/delay.h> > #include <linux/module.h> > +#include <linux/bootmem.h> > #include <linux/kvm_host.h> > +#include <linux/srcu.h> > + > > #include <asm/cpu.h> > #include <asm/bootinfo.h> > @@ -36,6 +39,8 @@ > /* Use VZ EntryHi.EHINV to invalidate TLB entries */ > #define UNIQUE_ENTRYHI(idx) (CKSEG0 + ((idx) << (PAGE_SHIFT + 1))) > > +EXPORT_SYMBOL(min_low_pfn); /* defined by bootmem.c, but not exported by generic code */ > + What you need this for? It is not used anywhere in this patch and by mips/kvm code in general. > atomic_t kvm_mips_instance; > EXPORT_SYMBOL(kvm_mips_instance); > > @@ -169,21 +174,27 @@ void kvm_mips_dump_shadow_tlbs(struct kvm_vcpu *vcpu) > } > } > > -static void kvm_mips_map_page(struct kvm *kvm, gfn_t gfn) > +static int kvm_mips_map_page(struct kvm *kvm, gfn_t gfn) > { > + int srcu_idx, err = 0; > pfn_t pfn; > > if (kvm->arch.guest_pmap[gfn] != KVM_INVALID_PAGE) > - return; > + return 0; > > + srcu_idx = srcu_read_lock(&kvm->srcu); Use tabs to indent. > pfn = kvm_mips_gfn_to_pfn(kvm, gfn); > > if (kvm_mips_is_error_pfn(pfn)) { > - panic("Couldn't get pfn for gfn %#" PRIx64 "!\n", gfn); > + kvm_err("Couldn't get pfn for gfn %#" PRIx64 "!\n", gfn); > + err = -EFAULT; > + goto out; > } > > kvm->arch.guest_pmap[gfn] = pfn; > - return; > +out: > + srcu_read_unlock(&kvm->srcu, srcu_idx); > + return err; > } > > /* Translate guest KSEG0 addresses to Host PA */ > @@ -207,7 +218,10 @@ unsigned long kvm_mips_translate_guest_kseg0_to_hpa(struct kvm_vcpu *vcpu, > gva); > return KVM_INVALID_PAGE; > } > - kvm_mips_map_page(vcpu->kvm, gfn); > + > + if (kvm_mips_map_page(vcpu->kvm, gfn) < 0) > + return KVM_INVALID_ADDR; > + > return (kvm->arch.guest_pmap[gfn] << PAGE_SHIFT) + offset; > } > > @@ -310,8 +324,11 @@ int kvm_mips_handle_kseg0_tlb_fault(unsigned long badvaddr, > even = !(gfn & 0x1); > vaddr = badvaddr & (PAGE_MASK << 1); > > - kvm_mips_map_page(vcpu->kvm, gfn); > - kvm_mips_map_page(vcpu->kvm, gfn ^ 0x1); > + if (kvm_mips_map_page(vcpu->kvm, gfn) < 0) > + return -1; > + > + if (kvm_mips_map_page(vcpu->kvm, gfn ^ 0x1) < 0) > + return -1; > > if (even) { > pfn0 = kvm->arch.guest_pmap[gfn]; > @@ -389,8 +406,11 @@ kvm_mips_handle_mapped_seg_tlb_fault(struct kvm_vcpu *vcpu, > pfn0 = 0; > pfn1 = 0; > } else { > - kvm_mips_map_page(kvm, mips3_tlbpfn_to_paddr(tlb->tlb_lo0) >> PAGE_SHIFT); > - kvm_mips_map_page(kvm, mips3_tlbpfn_to_paddr(tlb->tlb_lo1) >> PAGE_SHIFT); > + if (kvm_mips_map_page(kvm, mips3_tlbpfn_to_paddr(tlb->tlb_lo0) >> PAGE_SHIFT) < 0) > + return -1; > + > + if (kvm_mips_map_page(kvm, mips3_tlbpfn_to_paddr(tlb->tlb_lo1) >> PAGE_SHIFT) < 0) > + return -1; > > pfn0 = kvm->arch.guest_pmap[mips3_tlbpfn_to_paddr(tlb->tlb_lo0) >> PAGE_SHIFT]; > pfn1 = kvm->arch.guest_pmap[mips3_tlbpfn_to_paddr(tlb->tlb_lo1) >> PAGE_SHIFT]; > -- > 1.7.11.3 -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On May 14, 2013, at 2:27 AM, Gleb Natapov wrote: >> >> >> +EXPORT_SYMBOL(min_low_pfn); /* defined by bootmem.c, but not exported by generic code */ >> + > What you need this for? It is not used anywhere in this patch and by > mips/kvm code in general. I did some digging around myself, since the linker keeps complaining that it can't find min_low_pfn when compiling the KVM module. It seems that it is indirectly pulled in by the cache management functions. Regards Sanjay -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/15/2013 08:54 AM, Sanjay Lal wrote: > > On May 14, 2013, at 2:27 AM, Gleb Natapov wrote: > >>> >>> >>> +EXPORT_SYMBOL(min_low_pfn); /* defined by bootmem.c, but not exported by generic code */ >>> + >> What you need this for? It is not used anywhere in this patch and by >> mips/kvm code in general. > > I did some digging around myself, since the linker keeps complaining that it can't find min_low_pfn when compiling the KVM module. It seems that it is indirectly pulled in by the cache management functions. > If it is really needed, then the export should probably be done at the site of the min_low_pfn definition, not in some random architecture file. An alternative is to fix the cache management functions so they don't require the export. David Daney > > Regards > Sanjay > > > > > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 15, 2013 at 09:54:24AM -0700, David Daney wrote: > On 05/15/2013 08:54 AM, Sanjay Lal wrote: > > > >On May 14, 2013, at 2:27 AM, Gleb Natapov wrote: > > > >>> > >>> > >>>+EXPORT_SYMBOL(min_low_pfn); /* defined by bootmem.c, but not exported by generic code */ > >>>+ > >>What you need this for? It is not used anywhere in this patch and by > >>mips/kvm code in general. > > > >I did some digging around myself, since the linker keeps complaining that it can't find min_low_pfn when compiling the KVM module. It seems that it is indirectly pulled in by the cache management functions. > > > > If it is really needed, then the export should probably be done at > the site of the min_low_pfn definition, not in some random > architecture file. > Definitely. We cannot snick it here like that. Please drop it from this patch. > An alternative is to fix the cache management functions so they > don't require the export. > > David Daney > > > > >Regards > >Sanjay > > > > > > > > > > > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On May 15, 2013, at 10:30 AM, Gleb Natapov wrote: > On Wed, May 15, 2013 at 09:54:24AM -0700, David Daney wrote: >> On 05/15/2013 08:54 AM, Sanjay Lal wrote: >>> >>> On May 14, 2013, at 2:27 AM, Gleb Natapov wrote: >>> >>>>> >>>>> >>>>> +EXPORT_SYMBOL(min_low_pfn); /* defined by bootmem.c, but not exported by generic code */ >>>>> + >>>> What you need this for? It is not used anywhere in this patch and by >>>> mips/kvm code in general. >>> >>> I did some digging around myself, since the linker keeps complaining that it can't find min_low_pfn when compiling the KVM module. It seems that it is indirectly pulled in by the cache management functions. >>> >> >> If it is really needed, then the export should probably be done at >> the site of the min_low_pfn definition, not in some random >> architecture file. >> > Definitely. We cannot snick it here like that. Please drop it from this > patch. > I did export min_low_pfn where it was defined (in .../mm/bootmem.c) as part of the original patch set. It conflicted with the ia64/metag ports. min_low_pfn is exported in arch/ia64/kernel/ia64_ksyms.c and in arch/metag/kernel/metag_ksyms.c. There was some chatter about this when the KVM/MIPS code ended up in linux-next. From what I can gather, the maintainers for the other architectures agreed that exporting this symbol in bootmem.c was fine and should flow from the MIPS tree. I'll do that as part of v2 of the patch set. Regards Sanjay -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 15, 2013 at 11:36:02AM -0700, Sanjay Lal wrote: > > On May 15, 2013, at 10:30 AM, Gleb Natapov wrote: > > > On Wed, May 15, 2013 at 09:54:24AM -0700, David Daney wrote: > >> On 05/15/2013 08:54 AM, Sanjay Lal wrote: > >>> > >>> On May 14, 2013, at 2:27 AM, Gleb Natapov wrote: > >>> > >>>>> > >>>>> > >>>>> +EXPORT_SYMBOL(min_low_pfn); /* defined by bootmem.c, but not exported by generic code */ > >>>>> + > >>>> What you need this for? It is not used anywhere in this patch and by > >>>> mips/kvm code in general. > >>> > >>> I did some digging around myself, since the linker keeps complaining that it can't find min_low_pfn when compiling the KVM module. It seems that it is indirectly pulled in by the cache management functions. > >>> > >> > >> If it is really needed, then the export should probably be done at > >> the site of the min_low_pfn definition, not in some random > >> architecture file. > >> > > Definitely. We cannot snick it here like that. Please drop it from this > > patch. > > > > I did export min_low_pfn where it was defined (in .../mm/bootmem.c) as part of the original patch set. It conflicted with the ia64/metag ports. min_low_pfn is exported in arch/ia64/kernel/ia64_ksyms.c and in arch/metag/kernel/metag_ksyms.c. > > There was some chatter about this when the KVM/MIPS code ended up in linux-next. From what I can gather, the maintainers for the other architectures agreed that exporting this symbol in bootmem.c was fine and should flow from the MIPS tree. I'll do that as part of v2 of the patch set. > Make it a separate patch and send it to linux-kernel and linux-mm@kvack.org and affected arch maintainers. Or you can add export to arch/mips/kernel/mips_ksyms.c and ask Ralf to take it. I can take it via kvm tree with Ralf's ack too. In the commit message have a good explanation why it is needed please. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/mips/kvm/kvm_tlb.c b/arch/mips/kvm/kvm_tlb.c index 89511a9..218075f 100644 --- a/arch/mips/kvm/kvm_tlb.c +++ b/arch/mips/kvm/kvm_tlb.c @@ -16,7 +16,10 @@ #include <linux/mm.h> #include <linux/delay.h> #include <linux/module.h> +#include <linux/bootmem.h> #include <linux/kvm_host.h> +#include <linux/srcu.h> + #include <asm/cpu.h> #include <asm/bootinfo.h> @@ -36,6 +39,8 @@ /* Use VZ EntryHi.EHINV to invalidate TLB entries */ #define UNIQUE_ENTRYHI(idx) (CKSEG0 + ((idx) << (PAGE_SHIFT + 1))) +EXPORT_SYMBOL(min_low_pfn); /* defined by bootmem.c, but not exported by generic code */ + atomic_t kvm_mips_instance; EXPORT_SYMBOL(kvm_mips_instance); @@ -169,21 +174,27 @@ void kvm_mips_dump_shadow_tlbs(struct kvm_vcpu *vcpu) } } -static void kvm_mips_map_page(struct kvm *kvm, gfn_t gfn) +static int kvm_mips_map_page(struct kvm *kvm, gfn_t gfn) { + int srcu_idx, err = 0; pfn_t pfn; if (kvm->arch.guest_pmap[gfn] != KVM_INVALID_PAGE) - return; + return 0; + srcu_idx = srcu_read_lock(&kvm->srcu); pfn = kvm_mips_gfn_to_pfn(kvm, gfn); if (kvm_mips_is_error_pfn(pfn)) { - panic("Couldn't get pfn for gfn %#" PRIx64 "!\n", gfn); + kvm_err("Couldn't get pfn for gfn %#" PRIx64 "!\n", gfn); + err = -EFAULT; + goto out; } kvm->arch.guest_pmap[gfn] = pfn; - return; +out: + srcu_read_unlock(&kvm->srcu, srcu_idx); + return err; } /* Translate guest KSEG0 addresses to Host PA */ @@ -207,7 +218,10 @@ unsigned long kvm_mips_translate_guest_kseg0_to_hpa(struct kvm_vcpu *vcpu, gva); return KVM_INVALID_PAGE; } - kvm_mips_map_page(vcpu->kvm, gfn); + + if (kvm_mips_map_page(vcpu->kvm, gfn) < 0) + return KVM_INVALID_ADDR; + return (kvm->arch.guest_pmap[gfn] << PAGE_SHIFT) + offset; } @@ -310,8 +324,11 @@ int kvm_mips_handle_kseg0_tlb_fault(unsigned long badvaddr, even = !(gfn & 0x1); vaddr = badvaddr & (PAGE_MASK << 1); - kvm_mips_map_page(vcpu->kvm, gfn); - kvm_mips_map_page(vcpu->kvm, gfn ^ 0x1); + if (kvm_mips_map_page(vcpu->kvm, gfn) < 0) + return -1; + + if (kvm_mips_map_page(vcpu->kvm, gfn ^ 0x1) < 0) + return -1; if (even) { pfn0 = kvm->arch.guest_pmap[gfn]; @@ -389,8 +406,11 @@ kvm_mips_handle_mapped_seg_tlb_fault(struct kvm_vcpu *vcpu, pfn0 = 0; pfn1 = 0; } else { - kvm_mips_map_page(kvm, mips3_tlbpfn_to_paddr(tlb->tlb_lo0) >> PAGE_SHIFT); - kvm_mips_map_page(kvm, mips3_tlbpfn_to_paddr(tlb->tlb_lo1) >> PAGE_SHIFT); + if (kvm_mips_map_page(kvm, mips3_tlbpfn_to_paddr(tlb->tlb_lo0) >> PAGE_SHIFT) < 0) + return -1; + + if (kvm_mips_map_page(kvm, mips3_tlbpfn_to_paddr(tlb->tlb_lo1) >> PAGE_SHIFT) < 0) + return -1; pfn0 = kvm->arch.guest_pmap[mips3_tlbpfn_to_paddr(tlb->tlb_lo0) >> PAGE_SHIFT]; pfn1 = kvm->arch.guest_pmap[mips3_tlbpfn_to_paddr(tlb->tlb_lo1) >> PAGE_SHIFT];
Signed-off-by: Sanjay Lal <sanjayl@kymasys.com> --- arch/mips/kvm/kvm_tlb.c | 38 +++++++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 9 deletions(-)