On Thu, Oct 09, 2014 at 02:36:26PM +0100, Catalin Marinas wrote:
> On Thu, Oct 09, 2014 at 12:01:37PM +0100, Christoffer Dall wrote:
> > On Wed, Oct 08, 2014 at 10:47:04AM +0100, Catalin Marinas wrote:
> > > On Tue, Oct 07, 2014 at 08:39:54PM +0100, Christoffer Dall wrote:
> > > > +static inline int kvm_prealloc_hwpgd(struct kvm *kvm, pgd_t *pgd)
> > > > +{
> > > > + pud_t *pud;
> > > > + pmd_t *pmd;
> > > > + unsigned int order, i;
> > > > + unsigned long hwpgd;
> > > > +
> > > > + if (KVM_PREALLOC_LEVEL == 0)
> > > > + return 0;
> > > > +
> > > > + order = get_order(PTRS_PER_S2_PGD);
> > >
> > > Isn't order always 0 here? Based on our IRC discussion, PTRS_PER_S2_PGD
> > > is 16 or less and the order should not be used.
> >
> > no, if the kernel has 4K pages and 4 levels, then PGDIR_SHIFT is 39, and
> > KVM_PHYS_SHIFT stays 40, so that means PTRS_PER_S2_PGD becomes 2, which
> > means we concatenate two first level stage-2 page tables, which means we
> > need to allocate two consecutive pages, giving us an order of 1, not 0.
>
> So if PTRS_PER_S2_PGD is 2, how come get_order(PTRS_PER_S2_PGD) == 1? My
> reading of the get_order() macro is that get_order(2) == 0.
>
> Did you mean get_order(PTRS_PER_S2_PGD * PAGE_SIZE)?
Ah, you're right. Sorry. Yes, that's what I meant.
>
> Or you could define a PTRS_PER_S2_PGD_SHIFT as (KVM_PHYS_SHIFT -
> PGDIR_SHIFT) and use this as the order directly.
>
That's better. I also experimented with defining S2_HWPGD_ORDER or
S2_PREALLOC_ORDER, but it didn't look much clear, so sticking with
PTRS_PER_S2_PGD_SHIFT.
> > > > + hwpgd = __get_free_pages(GFP_KERNEL | __GFP_ZERO, order);
> > >
> > > I assume you need __get_free_pages() for alignment.
> >
> > yes, would you prefer a comment to that fact?
>
> No, that's fine.
>
Thanks,
-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
