Hi,
I was so bad.
I should not use p?d_populate() functions to create the page table for
virtual frametable. It should be independent of domain's page table
structure. p?d_populate() are also used for domain's page table.
Currently page table for virtual frametable consists of 3-level
table. And the domain's page table is also. But I think 2-level might
be enough for virtual frametable. It should be configurable without any
concern for domain's structure. Once I had a plan to tune it up.
To aboid the confusion, I wrote an attached patch.
Isaku Yamahata writes:
> #ifdef CONFIG_VIRTUAL_FRAME_TABLE
> - shr r22=r16,56 // Test for the address of virtual frame_table
> + shr.u r22=r16,56 // Test for the address of virtual frame_table
> ;;
> - cmp.eq p8,p0=((VIRT_FRAME_TABLE_ADDR>>56)&0xff)-0x100,r22
> + cmp.eq p8,p0=(VIRT_FRAME_TABLE_ADDR>>56),r22
> (p8) br.cond.sptk frametable_miss ;;
> #endif
That was a dirty hack.
Unfortunately, 'cmp' instruction can't handle such a immediate value(0xf3).
When the above patch is applied, The assembler complains:
ivt.S:547: Error: Operand 3 of `cmp.eq' should be an 8-bit integer (-128-127)
Thanks,
Kouya
Signed-off-by: Kouya SHIMURA <kouya@xxxxxxxxxxxxxx>
xenmem.patch
Description: Binary data
Isaku Yamahata writes:
> Hi.
>
> As Jes explained, p?d_populate() requires virtual address for third argument.
> So alloc_dir_page() should return virtual address.
> On the otherhand __pa(__pa(va)) == __pa(va) because __pa() masks high 3bits
> instead of __pa(va) = va - PAGE_OFFSET. Thus the current alloc_dir_page()
> creates correct frame tables fortunately.
> However alloc_dir_page() should be fixed, I think.
>
>
> About ivt.S part. You might have missed that shr is signed extended shift.
> Probably the following is more readable.
>
> #ifdef CONFIG_VIRTUAL_FRAME_TABLE
> - shr r22=r16,56 // Test for the address of virtual frame_table
> + shr.u r22=r16,56 // Test for the address of virtual frame_table
> ;;
> - cmp.eq p8,p0=((VIRT_FRAME_TABLE_ADDR>>56)&0xff)-0x100,r22
> + cmp.eq p8,p0=(VIRT_FRAME_TABLE_ADDR>>56),r22
> (p8) br.cond.sptk frametable_miss ;;
> #endif
>
> thanks.
>
> On Thu, Sep 21, 2006 at 01:26:57PM +0200, Jes Sorensen wrote:
> > Kouya SHIMURA wrote:
> > > Hi Jes,
> > >
> > > I wrote the patch corresponding to discontig. You are wrong.
> > > alloc_dir_page() must return a physical address because the page table
> > > walker for frame_table runs under off-state of data-address-translation.
> > > (i.e. psr.dt=0)
> > >
> > > p*d_populate() functions never be called while handling TLB miss to
> > > frame_table. The page table walker for frame_table is written in
> > > assembler in xen/arch/ia64/xen/ivt.S. See the code between
> > > 'GLOBAL_ENTRY(frametable_miss)' and 'END(frametable_miss)'
> >
> > Hi Kouya,
> >
> > I am not talking about during the TLB miss but during the creation
> > of the tables. I changed the code in ivt.S to make sure what was
> > happening was explicit, ie. if we are trying to resolve things in the
> > VIRT_FRAME_TABLE_ADDR space, then the test I put into ivt.S should be
> > identical to your original code, but I cannot boot with your code, it
> > MCA's because frametable_miss is never called since we are comparing for
> > the wrong address.
> >
> > Please take a look at this code:
> >
> > arch/ia64/xen/xenmem.c:
> > static int
> > create_frametable_page_table (u64 start, u64 end, void *arg)
> > {
> > ...
> > [snip]
> > ...
> >
> > if (pud_none(*pud))
> > pud_populate(NULL, pud, alloc_dir_page());
> > pmd = pmd_offset(pud, address);
> >
> > if (pmd_none(*pmd))
> > pmd_populate_kernel(NULL, pmd,
> > alloc_dir_page());
> > pte = pte_offset_kernel(pmd, address);
> >
> > Then in include/asm-ia64/linux-xen/asm/pgalloc.h you have this:
> >
> > static inline void
> > pud_populate(struct mm_struct *mm, pud_t * pud_entry, pmd_t * pmd)
> > {
> > pud_val(*pud_entry) = __pa(pmd);
> > }
> > .... and....
> > static inline void
> > pmd_populate_kernel(struct mm_struct *mm, pmd_t * pmd_entry, pte_t * pte)
> > {
> > pmd_val(*pmd_entry) = __pa(pte);
> > }
> >
> > In other words, if alloc_dir_page() returns a physical address as it
> > did before, you end up doing __pa() on the physical address which
> > just cannot be correct.
> >
> > My guess is that we were in fact doing a __pa() on the physical address
> > and the compare in ivt.S somehow was made to look at this address
> > instead of what it was pretending to do.
> >
> > I'd like to be proven wrong though, but without this patch Xen on SN2
> > only gets to an MCA at the moment it tries to initialize the page table.
> >
> > > You'd better have a look at the thread below:
> > > http://lists.xensource.com/archives/html/xen-ia64-devel/2006-04/msg00014.html
> >
> > I'll take a look.
> >
> > Cheers,
> > Jes
> >
> >
> > > Thanks,
> > > Kouya
> > >
> > > Jes Sorensen writes:
> > > > Hi,
> > > >
> > > > I sent this patch to Alex last week, but it didn't make it onto the
> > > list
> > > > because it's wrongly configured, so here we go again.
> > > >
> > > > I know that this patch is causing problems on ZX1, but I have looked
> > > at
> > > > it over and over again and I feel pretty certain it is correct. In
> > > fact
> > > > I cannot understand that Xen could boot on any ia64 platform prior to
> > > > this at all.
> > > >
> > > > I would be very interested in hearing how this patch affects other
> > > > platforms such as DIG and Fujitsu's machines (if they are not DIG :)
> > > >
> > > > Any input or comments on this is most welcome - if you think I am
> > > wrong
> > > > about this patch, please tell me, I really want to understand why this
> > > > worked in the past.
> > > >
> > > > Thanks,
> > > > Jes
> > > >
> > > > alloc_dir_page() must return a virtual address so it can handle being
> > > > passed to the p*d_populate() functions which do a __pa() on the
> > > address
> > > > before sticking them into the page tables.
> > > >
> > > > To match this it is also necessary to correctly check the faulting
> > > > address for being in the virtual frame table range, otherwise page
> > > > faults for this space weren't being served at all.
> > > >
> > > > This could probably be done more efficiently, but for now I think it's
> > > > better to keep the code explicit.
> > > >
> > > > Signed-off-by: Jes Sorensen <jes@xxxxxxx>
> > > >
> > > > diff -r 3e4fa8b5b245 xen/arch/ia64/xen/ivt.S
> > > > --- a/xen/arch/ia64/xen/ivt.S Tue Sep 12 11:43:22 2006 -0600
> > > > +++ b/xen/arch/ia64/xen/ivt.S Wed Sep 20 14:56:37 2006 +0200
> > > > @@ -542,8 +542,16 @@ late_alt_dtlb_miss:
> > > > ;;
> > > > #ifdef CONFIG_VIRTUAL_FRAME_TABLE
> > > > shr r22=r16,56 // Test for the address of virtual
> > > frame_table
> > > > +#if 1
> > > > + mov r23=VIRT_FRAME_TABLE_ADDR>>56
> > > > + ;;
> > > > + xor r23=r22,r23
> > > > + ;;
> > > > + cmp.eq p8,p0=r23,r0
> > > > +#else
> > > > ;;
> > > > cmp.eq p8,p0=((VIRT_FRAME_TABLE_ADDR>>56)&0xff)-0x100,r22
> > > > +#endif
> > > > (p8) br.cond.sptk frametable_miss ;;
> > > > #endif
> > > > // If it is not a Xen address, handle it via page_fault.
> > > > diff -r 3e4fa8b5b245 xen/arch/ia64/xen/xenmem.c
> > > > --- a/xen/arch/ia64/xen/xenmem.c Tue Sep 12 11:43:22 2006 -0600
> > > > +++ b/xen/arch/ia64/xen/xenmem.c Wed Sep 20 17:14:01 2006 +0200
> > > > @@ -76,13 +76,13 @@ alloc_dir_page(void)
> > > > alloc_dir_page(void)
> > > > {
> > > > unsigned long mfn = alloc_boot_pages(1, 1);
> > > > - unsigned long dir;
> > > > + unsigned char *virtual;
> > > > if (!mfn)
> > > > panic("Not enough memory for virtual frame table!\n");
> > > > ++table_size;
> > > > - dir = mfn << PAGE_SHIFT;
> > > > - memset(__va(dir), 0, PAGE_SIZE);
> > > > - return (void *)dir;
> > > > + virtual = __va(mfn << PAGE_SHIFT);
> > > > + memset(virtual, 0, PAGE_SIZE);
> > > > + return virtual;
> > > > }
> > > >
> > > > static inline unsigned long
> > > > _______________________________________________
> > > > Xen-ia64-devel mailing list
> > > > Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
> > > > http://lists.xensource.com/xen-ia64-devel
> >
> >
> > _______________________________________________
> > Xen-ia64-devel mailing list
> > Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
> > http://lists.xensource.com/xen-ia64-devel
>
> --
> yamahata
_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel
|
Home