All of lore.kernel.org
 help / color / mirror / Atom feed
* RE: [patch] predicate NX flag
@ 2005-06-07 21:02 Nakajima, Jun
  2005-06-08 18:06 ` [patch] nx bit shouldn't get set when disabled Scott Parish
  0 siblings, 1 reply; 15+ messages in thread
From: Nakajima, Jun @ 2005-06-07 21:02 UTC (permalink / raw)
  To: Nakajima, Jun, Scott Parish; +Cc: xen-devel

BTW, did this solve the driver problem on your machine?

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 Nakajima,
Jun
Sent: Tuesday, June 07, 2005 1:46 PM
To: Scott Parish
Cc: xen-devel@lists.xensource.com
Subject: RE: [Xen-devel] [patch] predicate NX flag

Scott Parish wrote:
> the NX flag should only be set when its use is enabled.
> 
> sRp

Rather than changing __PAGE_KERNEL, I think we should change set_p?d (?
= g, u, m, e) like

#define set_pmd(pmdptr, pmdval) xen_l2_entry_update(pmdptr, (pmdval)&
__supported_pte_mask)

Jun
---
Intel Open Source Technology Center

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

^ permalink raw reply	[flat|nested] 15+ messages in thread
* RE: [patch] nx bit shouldn't get set when disabled
@ 2005-06-08 21:30 Ian Pratt
  2005-06-08 21:28 ` Keir Fraser
  0 siblings, 1 reply; 15+ messages in thread
From: Ian Pratt @ 2005-06-08 21:30 UTC (permalink / raw)
  To: Keir Fraser, Scott Parish; +Cc: xen-devel, Nakajima, Jun

 
> >> Why the extra mask ops with __supported_pte_mask? Native x86/64 
> >> builds obviously don't need them...
> >
> > Definitions such as __PAGE_KERNEL set NX, but as Jun pointed out, 
> > those should only be set when NX mode is enabled.
> 
> So the extra masking isn't required?

I suspect normal hardware is prepared to put up with the bit being set
even if its not supported...

Ian

^ permalink raw reply	[flat|nested] 15+ messages in thread
* RE: [patch] nx bit shouldn't get set when disabled
@ 2005-06-08 21:56 Nakajima, Jun
  2005-06-08 22:00 ` Keir Fraser
  0 siblings, 1 reply; 15+ messages in thread
From: Nakajima, Jun @ 2005-06-08 21:56 UTC (permalink / raw)
  To: Keir Fraser, Scott Parish; +Cc: xen-devel

Keir Fraser wrote:
> On 8 Jun 2005, at 20:47, Scott Parish wrote:
> 
>>> Why does x86_64 get pte_mfn, but not pae i386? I think pci-dma.c
>>> should probably be shared between i386 and x86/64.
>> 
>> Last time i checked, the linux side of i386 pae wasn't merged into
>> bk, so i have nothing to test such a patch against. I'll plan on
>> getting a pae setup going again and sending a patch to gerd.
> 
> The functions that are changed aren't pae-specific, and they are
> already in the xen/i386 tree. They can be patched in anticipation of
> pae, even though they can only be properly tested non-pae for the time
> being. I'm not inclined to take patches for xen/x86_64/pci-dma.c
> anyway: I think we can patch the xen/i386 one and share it with
> xen/x86_64. Otherwise we're going to get unnecessary divergence
> between what really ought to be two identical files. (I already did
> this for arch/xen/i386/kernel/time.c, for example.)
> 
>>> Why the extra mask ops with __supported_pte_mask? Native x86/64
>>> builds obviously don't need them...
>> 
>> Definitions such as __PAGE_KERNEL set NX, but as Jun pointed out,
>> those should only be set when NX mode is enabled.
> 
> So the extra masking isn't required?
> 
>   -- Keir
Hold on.

Scott, in your patch:

-		pfn = pte->pte >> PAGE_SHIFT;
+		mfn = pte_mfn(*pte);

+#define pte_mfn(_pte) (((_pte).pte & PTE_MASK) >> PAGE_SHIFT)

These should not be necessary if pte is created correctly (w/ or w/o NX
bit depending on __supported_pte_mask) in the first place, as Keir
pointed out. That's what I meant by "We should fix the creator of the
pte (by __supported_pte_mask), not the consumer of it." And you always
cut off the NX bit in your patch.

So remove the changes to pci-dma.c and init.c. If that does not work,
move check_efer to right before pda_init(0) in x86_64_start_kernel() in
head64.c.

Jun
---
Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 15+ messages in thread
* RE: [patch] nx bit shouldn't get set when disabled
@ 2005-06-08 22:47 Nakajima, Jun
  2005-06-08 22:33 ` Scott Parish
  0 siblings, 1 reply; 15+ messages in thread
From: Nakajima, Jun @ 2005-06-08 22:47 UTC (permalink / raw)
  To: Keir Fraser; +Cc: xen-devel, Scott Parish

Keir Fraser wrote:
> On 8 Jun 2005, at 22:56, Nakajima, Jun wrote:
> 
>> These should not be necessary if pte is created correctly (w/ or w/o
>> NX bit depending on __supported_pte_mask) in the first place, as Keir
>> pointed out. That's what I meant by "We should fix the creator of the
>> pte (by __supported_pte_mask), not the consumer of it." And you
>> always cut off the NX bit in your patch.
>> 
>> So remove the changes to pci-dma.c and init.c. If that does not work,
>> move check_efer to right before pda_init(0) in x86_64_start_kernel()
>> in head64.c.
> 
> I think the changes in pci-dma.c *are* required: that code currently
> just does pte->pte >> PAGE_SHIFT, which certainly isn't right. That's
> code that doesn't exist in native Linux so it probably does need
> fixing up for PAE/NX.
Agree on that part. Should read like:

  pte = pte_offset_kernel(pmd, (vstart + (i*PAGE_SIZE)));
+ pte->pte &= __supported_pte_mask;
  pfn = pte->pte >> PAGE_SHIFT;

> 
> But 'fixes' to simple native functions like set_pud, set_pmd, etc.
> ought not to be necessary. We shouldn't have to fix ubiquitous
> functions like that to support nx bit on xenlinux. If we do, it's a
> sign that something is very wrong! :-)
> 
>   -- Keir
I found a bug in phys_pud_init(). I'll send a patch. 

Jun
---
Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 15+ messages in thread
* RE: [patch] nx bit shouldn't get set when disabled
@ 2005-06-08 23:29 Nakajima, Jun
  0 siblings, 0 replies; 15+ messages in thread
From: Nakajima, Jun @ 2005-06-08 23:29 UTC (permalink / raw)
  To: Scott Parish; +Cc: xen-devel

Scott Parish wrote:
> On Wed, Jun 08, 2005 at 03:47:01PM -0700, Nakajima, Jun wrote:
> 
>> Agree on that part. Should read like:
>> 
>>   pte = pte_offset_kernel(pmd, (vstart + (i*PAGE_SIZE)));
>> + pte->pte &= __supported_pte_mask;
>>   pfn = pte->pte >> PAGE_SHIFT;
> 
> I dissent. Same reason as yesterday: the above might work right now,
> but as soon as we enable NX the NX bit will end up in the pfn/mfn.
> 
> sRp

Your are correct. We always need to cut off the NX bit because that's
what HYPERVISOR_dom_mem_op expects. 

Jun
---
Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 15+ messages in thread
* RE: [patch] nx bit shouldn't get set when disabled
@ 2005-06-09 15:02 Nakajima, Jun
  0 siblings, 0 replies; 15+ messages in thread
From: Nakajima, Jun @ 2005-06-09 15:02 UTC (permalink / raw)
  To: Scott Parish, Keir Fraser; +Cc: xen-devel

Scott Parish wrote:
> On Wed, Jun 08, 2005 at 10:23:47PM +0100, Keir Fraser wrote:
> 
>> I'm not inclined to take patches for xen/x86_64/pci-dma.c anyway: I
>> think we can patch the xen/i386 one and share it with xen/x86_64.
>> Otherwise we're going to get unnecessary divergence between what
>> really ought to be two identical files. (I already did this for
>> arch/xen/i386/kernel/time.c, for example.)
> 
> The attached patch unifies pci-dma.c and adds the pte_mfn() macro.
> 
> The one thing that might need an explanation, there's 4 lines of
> changes (walking the page table) that add parentheses. The x86_64
> compiler doesn't seem to like doing unparenthesized math for function
> arguments. 
> 
> Boot tested dom0 on x86_64 and x86_32 (non-pae)
> 
> sRp

Looks good. That's the right thing.

BTW, I thought I found a bug in phys_pud_init(), but it's not.

Jun
---
Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2005-06-09 15:02 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-06-07 21:02 [patch] predicate NX flag Nakajima, Jun
2005-06-08 18:06 ` [patch] nx bit shouldn't get set when disabled Scott Parish
2005-06-08 19:56   ` Keir Fraser
2005-06-08 19:47     ` Scott Parish
2005-06-08 21:23       ` Keir Fraser
     [not found]         ` <20050608204727.GH3182@us.ibm.com>
     [not found]           ` <d246c2258805470a73cb0b357f3a6739@cl.cam.ac.uk>
2005-06-08 22:39             ` Scott Parish
2005-06-09 12:59         ` Scott Parish
  -- strict thread matches above, loose matches on Subject: below --
2005-06-08 21:30 Ian Pratt
2005-06-08 21:28 ` Keir Fraser
2005-06-08 21:56 Nakajima, Jun
2005-06-08 22:00 ` Keir Fraser
2005-06-08 22:47 Nakajima, Jun
2005-06-08 22:33 ` Scott Parish
2005-06-08 23:29 Nakajima, Jun
2005-06-09 15:02 Nakajima, Jun

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.