Re: [Xen-ia64-devel] [patch] alloc_page_dir() should return a virtual address

[Isaku Yamahata <yamahata@xxxxxxxxxxxxx>]

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