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