WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-ia64-devel

[Xen-ia64-devel] [PATCH] create page table for virtual frame table

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>

Attachment: 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