All of lore.kernel.org
 help / color / mirror / Atom feed
* RE: [patch] (resend) mask out nx bits when calculatingpfn/mfn
@ 2005-06-07 17:16 Nakajima, Jun
  2005-06-07 16:57 ` Scott Parish
  0 siblings, 1 reply; 5+ messages in thread
From: Nakajima, Jun @ 2005-06-07 17:16 UTC (permalink / raw)
  To: Scott Parish; +Cc: xen-devel

Scott Parish wrote:
> On Tue, Jun 07, 2005 at 09:58:20AM -0700, Nakajima, Jun wrote:
> 
>> But did you really see the NX bit? I think that (NX bit) would be a
>> good catch and it explains several failures of device drivers.  We
>> should fix the creator of the pte (by __supported_pte_mask), not the
>> consumer of it.
> 
> I was seeing it in xen_contig_memory(), which was getting called by
> e100_alloc_cbs(). The crash was happening when i would run "xend
> start" 
> 
> By "fix the creator", your suggesting that there shouldn't be an NX
> bit set? Why?

Right now, it's disabled (because Xen does not like it) in setup64.c
(see below). Adding NX support is on my plate, but welcome to take it.
See http://wiki.xensource.com/xenwiki/XenTodoList.


void __init check_efer(void)
{
	unsigned long efer;

        /*	rdmsrl(MSR_EFER, efer);  */

        /*
         * At this point, Xen does not like the bit 63.
         * So NX is not supported. Come back later.
         */
        efer = 0;

        if (!(efer & EFER_NX) || do_not_nx) { 
                __supported_pte_mask &= ~_PAGE_NX; 
        }       
}

> 
>> We also need to fix Xen because Xen should not reject the request
>> just because it has the NX bit on.
> 
> An NX bit is now an official part of a page frame number???
> 
> i'm confused
No, Xen should mask off the bit when calculating the page frame number,
and looks like it's not happening. 

> 
> sRp


Jun
---
Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 5+ messages in thread
* RE: [patch] (resend) mask out nx bits when calculatingpfn/mfn
@ 2005-06-07 18:33 Nakajima, Jun
  0 siblings, 0 replies; 5+ messages in thread
From: Nakajima, Jun @ 2005-06-07 18:33 UTC (permalink / raw)
  To: Scott Parish; +Cc: xen-devel

Scott Parish wrote:
> I'll poke around and try to find who's setting the NX bit. Its still
> not clear to me though why my patch isn't valid. Maybe the traceback
> will help:
> 
> (XEN) (file=dom_mem_ops.c, line=101) Domain 0 page number out of
> range (80000000016b0 >= 180000) ----------- [cut here ] ---------

Right, it's on.

> [please bite here ] --------- 
> Kernel BUG at pci_dma:98
> invalid operand: 0000 [1]
> CPU 0
> Modules linked in:
> Pid: 10266, comm: ifconfig Not tainted 2.6.11.11-xen0
> RIP: e030:[<ffffffff80113bdc>]
> <ffffffff80113bdc>{xen_contig_memory+460} 
> RSP: e02b:ffff880008881ca8  EFLAGS: 00010297
> RAX: 00000000ffffffff RBX: ffff88000e840000 RCX: ffffffff80113bd8
> RDX: 0000000000000001 RSI: ffff880008881cc8 RDI: 0000000000000001
> RBP: 0000000000000000 R08: 0000000000007ff0 R09: ffff880008881cc8
> R10: 0000000000000000 R11: 0000000000000293 R12: 003fffe20023a100
> R13: 000000000003a100 R14: ffff88000e840000 R15: 0000000000000001
> FS:  00002aaaaaac1250(0000) GS:ffffffff804f2400(0000)
> knlGS:0000000000000000 
> CS:  e033 DS: 0000 ES: 0000
> Process ifconfig (pid: 10266, threadinfo ffff880008880000, task
> ffff880000f3c230) 
> Stack: ffff880008881cc8 0000002000000000 0000000000000000
>        0000000400000000 00080000000016b0 ffff88000e840000
>        0000000000000004 0000000000000020 ffff880000b02870
> 0000000000009000 
> Call Trace:<ffffffff80113f97>{dma_alloc_coherent+343}
>        <ffffffff80274d31>{e100_alloc_cbs+113}
>        <ffffffff80275c00>{e100_up+48}
>        <ffffffff80276db8>{e100_open+56}
>        <ffffffff80330f23>{dev_open+67}
>        <ffffffff8033257a>{dev_change_flags+90}
>        <ffffffff80367ee9>{devinet_ioctl+697}
> <ffffffff80369fe7>{inet_ioctl+87} <ffffffff8032894c>{sock_ioctl+588}
> <ffffffff80168c21>{do_ioctl+33} <ffffffff80168f83>{vfs_ioctl+419}
> <ffffffff80168fed>{sys_ioctl+77} <ffffffff8010d421>{system_call+125}
> <ffffffff8010d3a4>{system_call+0}    
> 
> 
> If you look at free_dom_mem(), where the DPRINTK is called, the mfn is
> being copied directly from the user. The hypervisor then checks it to
> make sure that its not out of bounds with max_page, which is where we
> fail. This is why i'm still skeptical that we want the hypervisor to
> have to mask off bits in this exact code.
> 
> sRp

I'm not sure why free_dom_mem() is called (is it the result of the
problem?), but I agree. The caller should mask off the bits in this case
because they are not ptes (but a list of page frame numbers).

       if ( unlikely(__get_user(mpfn, &extent_list[i]) != 0) )
            return i;

        for ( j = 0; j < (1 << extent_order); j++ )
        {
            if ( unlikely((mpfn + j) >= max_page) )
            {
                DPRINTK("Domain %u page number out of range (%lx >=
%lx)\n",
                        d->domain_id, mpfn + j, max_page);
                return i;
            }


Jun
---
Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 5+ messages in thread
* RE: [patch] (resend) mask out nx bits when calculatingpfn/mfn
@ 2005-06-07 16:58 Nakajima, Jun
  2005-06-07 16:23 ` Scott Parish
  0 siblings, 1 reply; 5+ messages in thread
From: Nakajima, Jun @ 2005-06-07 16:58 UTC (permalink / raw)
  To: Scott Parish, xen-devel

But did you really see the NX bit? I think that (NX bit) would be a good
catch and it explains several failures of device drivers.  We should fix
the creator of the pte (by __supported_pte_mask), not the consumer of
it.

We also need to fix Xen because Xen should not reject the request just
because it has the NX bit on.

Jun
---
Intel Open Source Technology Center 

-----Original Message-----
From: xen-devel-bounces@lists.xensource.com
[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Scott Parish
Sent: Tuesday, June 07, 2005 9:00 AM
To: xen-devel@lists.xensource.com
Subject: Re: [Xen-devel] [patch] (resend) mask out nx bits when
calculatingpfn/mfn

Please ignore the first patch, it had a messed up macro.

sRp

On Tue, Jun 07, 2005 at 03:52:50PM +0000, Scott Parish wrote:

> This patch fixes a problem where it was possible (and seen) for the
> NX bit to not be masked out when calculating a pfn or mfn.
> 
> 
> sRp
> 
> -- 
> Scott Parish
> Signed-off-by: srparish@us.ibm.com

> diff -rN -u -p
old-xen-64-4/linux-2.6.11-xen-sparse/arch/xen/x86_64/kernel/pci-dma.c
new-xen-64-4/linux-2.6.11-xen-sparse/arch/xen/x86_64/kernel/pci-dma.c
> ---
old-xen-64-4/linux-2.6.11-xen-sparse/arch/xen/x86_64/kernel/pci-dma.c
2005-05-24 17:34:54.000000000 +0000
> +++
new-xen-64-4/linux-2.6.11-xen-sparse/arch/xen/x86_64/kernel/pci-dma.c
2005-06-07 15:35:43.000000000 +0000
> @@ -78,7 +78,7 @@ xen_contig_memory(unsigned long vstart, 
>  	pud_t         *pud; 
>  	pmd_t         *pmd;
>  	pte_t         *pte;
> -	unsigned long  pfn, i, flags;
> +	unsigned long  mfn, i, flags;
>  
>  	scrub_pages(vstart, 1 << order);
>  
> @@ -90,16 +90,16 @@ xen_contig_memory(unsigned long vstart, 
>  		pud = pud_offset(pgd, (vstart + (i*PAGE_SIZE)));
>  		pmd = pmd_offset(pud, (vstart + (i*PAGE_SIZE)));
>  		pte = pte_offset_kernel(pmd, (vstart + (i*PAGE_SIZE)));
> -		pfn = pte->pte >> PAGE_SHIFT;
> +		mfn = pte_mfn(*pte);
>  		xen_l1_entry_update(pte, 0);
>  		phys_to_machine_mapping[(__pa(vstart)>>PAGE_SHIFT)+i] =
>  			(u32)INVALID_P2M_ENTRY;
>  		if (HYPERVISOR_dom_mem_op(MEMOP_decrease_reservation, 
> -					  &pfn, 1, 0) != 1) BUG();
> +					  &mfn, 1, 0) != 1) BUG();
>  	}
>  	/* 2. Get a new contiguous memory extent. */
>  	if (HYPERVISOR_dom_mem_op(MEMOP_increase_reservation,
> -				  &pfn, 1, order) != 1) BUG();
> +				  &mfn, 1, order) != 1) BUG();
>  	/* 3. Map the new extent in place of old pages. */
>  	for (i = 0; i < (1<<order); i++) {
>  		pgd = pgd_offset_k(   (vstart + (i*PAGE_SIZE)));
> @@ -107,12 +107,12 @@ xen_contig_memory(unsigned long vstart, 
>  		pmd = pmd_offset(pud, (vstart + (i*PAGE_SIZE)));
>  		pte = pte_offset_kernel(pmd, (vstart + (i*PAGE_SIZE)));
>  		xen_l1_entry_update(
> -			pte, ((pfn+i)<<PAGE_SHIFT)|__PAGE_KERNEL);
> +			pte, ((mfn+i)<<PAGE_SHIFT)|__PAGE_KERNEL);
>  		xen_machphys_update(
> -			pfn+i, (__pa(vstart)>>PAGE_SHIFT)+i);
> +			mfn+i, (__pa(vstart)>>PAGE_SHIFT)+i);
>  		phys_to_machine_mapping[(__pa(vstart)>>PAGE_SHIFT)+i] =
> -			pfn+i;
> +			mfn+i;
>  	}
>  	/* Flush updates through and flush the TLB. */
>  	xen_tlb_flush();
> diff -rN -u -p
old-xen-64-4/linux-2.6.11-xen-sparse/arch/xen/x86_64/mm/hypervisor.c
new-xen-64-4/linux-2.6.11-xen-sparse/arch/xen/x86_64/mm/hypervisor.c
> ---
old-xen-64-4/linux-2.6.11-xen-sparse/arch/xen/x86_64/mm/hypervisor.c
2005-06-06 16:39:54.000000000 +0000
> +++
new-xen-64-4/linux-2.6.11-xen-sparse/arch/xen/x86_64/mm/hypervisor.c
2005-06-07 15:39:31.000000000 +0000
> @@ -256,7 +256,7 @@ unsigned long allocate_empty_lowmem_regi
>          pud = pud_offset(pgd, (vstart + (i*PAGE_SIZE)));
>          pmd = pmd_offset(pud, (vstart + (i*PAGE_SIZE)));
>          pte = pte_offset_kernel(pmd, (vstart + (i*PAGE_SIZE))); 
> -        pfn_array[i] = pte->pte >> PAGE_SHIFT;
> +        pfn_array[i] = pte_mfn(*pte);
>          xen_l1_entry_update(pte, 0);
>          phys_to_machine_mapping[(__pa(vstart)>>PAGE_SHIFT)+i] =
>              (u32)INVALID_P2M_ENTRY;
> diff -rN -u -p
old-xen-64-4/linux-2.6.11-xen-sparse/arch/xen/x86_64/mm/init.c
new-xen-64-4/linux-2.6.11-xen-sparse/arch/xen/x86_64/mm/init.c
> --- old-xen-64-4/linux-2.6.11-xen-sparse/arch/xen/x86_64/mm/init.c
2005-06-02 23:09:31.000000000 +0000
> +++ new-xen-64-4/linux-2.6.11-xen-sparse/arch/xen/x86_64/mm/init.c
2005-06-07 15:39:50.000000000 +0000
> @@ -395,7 +395,7 @@ unsigned long get_machine_pfn(unsigned l
>          pmd_t* pmd = pmd_offset(pud, addr);
>          pte_t *pte = pte_offset_kernel(pmd, addr);
>          
> -        return (pte->pte >> PAGE_SHIFT);
> +        return pte_mfn(*pte);
>  } 
>  
>  #define ALIGN_TO_4K __attribute__((section(".data.page_aligned")))
> diff -rN -u -p
old-xen-64-4/linux-2.6.11-xen-sparse/include/asm-xen/asm-x86_64/pgtable.
h
new-xen-64-4/linux-2.6.11-xen-sparse/include/asm-xen/asm-x86_64/pgtable.
h
> ---
old-xen-64-4/linux-2.6.11-xen-sparse/include/asm-xen/asm-x86_64/pgtable.
h	2005-05-28 09:20:36.000000000 +0000
> +++
new-xen-64-4/linux-2.6.11-xen-sparse/include/asm-xen/asm-x86_64/pgtable.
h	2005-06-07 15:32:47.000000000 +0000
> @@ -277,9 +277,10 @@ static inline unsigned long pud_bad(pud_
>   */
>  #define INVALID_P2M_ENTRY (~0UL)
>  #define FOREIGN_FRAME(_m) ((_m) | (1UL<<((sizeof(unsigned
long)*8)-1)))
> +#define pte_mfn(_pte) ((_pte).pte & PTE_MASK) >> PAGE_SHIFT;
>  #define pte_pfn(_pte)
\
>  ({
\
> -	unsigned long mfn = (_pte).pte >> PAGE_SHIFT;
\
> +	unsigned long mfn = pte_mfn(_pte);
\
>  	unsigned pfn = mfn_to_pfn(mfn);
\
>  	if ((pfn >= max_mapnr) || (pfn_to_mfn(pfn) != mfn))
\
>  		pfn = max_mapnr; /* special: force !pfn_valid() */
\

> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


-- 
Scott Parish
Signed-off-by: srparish@us.ibm.com

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2005-06-07 18:33 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-06-07 17:16 [patch] (resend) mask out nx bits when calculatingpfn/mfn Nakajima, Jun
2005-06-07 16:57 ` Scott Parish
  -- strict thread matches above, loose matches on Subject: below --
2005-06-07 18:33 Nakajima, Jun
2005-06-07 16:58 Nakajima, Jun
2005-06-07 16:23 ` Scott Parish

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.