Re: [PATCH -mm 1/3] i386 boot: replace boot_ioremap with enhanced bt_ioremap - enhance bt_ioremap

[Ian Campbell <ijc@hellion.org.uk>]

On Fri, 2008-01-18 at 22:54 +0800, huang ying wrote:
> On Jan 18, 2008 4:48 PM, Ian Campbell <ijc@hellion.org.uk> wrote:
> > On Tue, 2008-01-15 at 13:45 +0800, Huang, Ying wrote:
> > > +void __init bt_ioremap_init(void)
> > > +{
> > > [...]
> > > +     *pgd = __pa(bm_pte) | _PAGE_TABLE;
> > > +}
> > > [...]
> > > +static void __init __bt_set_fixmap(enum fixed_addresses idx,
> > > +                                unsigned long phys, pgprot_t flags)
> > > +{
> > > [...]
> > > +     if (pgprot_val(flags))
> > > +             *pte = (phys & PAGE_MASK) | pgprot_val(flags);
> > > +     else
> > > +             *pte = 0;
> > [...]
> >
> > Shouldn't these, and the rest of the file, be using the PTE accessor
> > macros set_pte,clear_pte etc? The boot_ioremap it replaces seems to have
> > done. Otherwise these patches don't appear to be paravirt_ops clean. I
> 
> If CONFIG_X86_PAE is defined, the set_pte, clear_pte etc will operate
> 3-level page tables, while on i386, the early page table is always
> 2-level, so set_pte, clear_pte etc functions can not be used here. The
> boot_ioremap use a trick to deal with this problem. The CONFIG_X86_PAE
> is undefined in arch/x86/mm/boot_ioremap_32.c unconditionally, so the
> 2-level page table handling function is always used.

Ah, when booting a Xen guest you get 3 level page tables right from the
start.

I'm hacking on a patch right now but I hadn't appreciated/noticed that
this was a two level page table even on a PAE kernel which would explain
why it isn't exactly working as planned...

> Is the method used by boot_ioremap better for Xen?

Well, it worked ;-) I haven't looked closely enough at the two ways of
doing things to comment further but I'm sure the new way can be made to
work for Xen too somehow.

> [snip]

> This crash has nothing to do with this patch. Because this patch is
> for i386 only, x86_64 has its own early_ioremap implementation.

It's a 64 bit hypervisor but the guest in question is a 32 bit (PAE)
guest.

Ian
-- 
Ian Campbell
Current Noise: Mistress - Cheyne Stoking

You can't kiss a girl unexpectedly -- only sooner than she thought you would.