Re: [GIT PULL] x86/mm changes for v3.9-rc1

Re: [GIT PULL] x86/mm changes for v3.9-rc1

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 1201 bytes --]

[ trim cc list]

On Fri, Feb 22, 2013 at 5:27 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
>> >  static int xen_pgd_alloc(struct mm_struct *mm)
>> >  {
>> >         pgd_t *pgd = mm->pgd;
>> > @@ -2105,7 +2143,7 @@ static const struct pv_mmu_ops xen_mmu_ops __initconst = {
>> >  #ifdef CONFIG_X86_32
>> >         .write_cr3 = xen_write_cr3_init,
>> >  #else
>> > -       .write_cr3 = xen_write_cr3,
>> > +       .write_cr3 = xen_write_cr3_init,
>> >  #endif
>>
>> ah, why do you still keep the #ifdef here?
>>
>> how about if we call load_cr3 early several times?
>> assume you should make xen_write_cr3 more robust,
>> like bailing out early when cr3 writing same value.
>
> I would welcome such patch - but at this point I just want a
> patch for Linus so that I am not blocking him - and this
> one works.

come on, you produce

  #ifdef CONFIG_X86_32
         .write_cr3 = xen_write_cr3_init,
  #else
         .write_cr3 = xen_write_cr3_init,
  #endif

for the fix, Linus should just apply attached patch instead.

>>
>> Also you should check condition about calling xen_get_user_pgd().
>
> Could you elaborate please?

only call xen_get_user_pgd() when it should be called.

Yinghai

[-- Attachment #2: fix_xen_cr3.patch --]
[-- Type: application/octet-stream, Size: 537 bytes --]

---
 arch/x86/mm/init.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-2.6/arch/x86/mm/init.c
===================================================================
--- linux-2.6.orig/arch/x86/mm/init.c
+++ linux-2.6/arch/x86/mm/init.c
@@ -448,7 +448,8 @@ void __init init_mem_mapping(void)
 	early_ioremap_page_table_range_init();
 #endif
 
-	load_cr3(swapper_pg_dir);
+	if (read_cr3() != __pa(swapper_pg_dir))
+		load_cr3(swapper_pg_dir);
 	__flush_tlb_all();
 
 	early_memtest(0, max_pfn_mapped << PAGE_SHIFT);

Re: [GIT PULL] x86/mm changes for v3.9-rc1

[Konrad Rzeszutek Wilk]

> >> how about if we call load_cr3 early several times?
> >> assume you should make xen_write_cr3 more robust,
> >> like bailing out early when cr3 writing same value.
> >
> > I would welcome such patch - but at this point I just want a
> > patch for Linus so that I am not blocking him - and this
> > one works.
> 
> come on, you produce
> 
>   #ifdef CONFIG_X86_32
>          .write_cr3 = xen_write_cr3_init,
>   #else
>          .write_cr3 = xen_write_cr3_init,
>   #endif
> 
> for the fix, Linus should just apply attached patch instead.

I misunderstood you. I thought you were talking about
"how about if we call .. cr3 writing same value" - and that part
I hadn't thought completly through so I did not want to delay
Linus.

Naturally the fixes you pointed out should be in the patch - and
hpa graciously did the fixes and sent to Linus. Thank you Peter!
> 
> >>
> >> Also you should check condition about calling xen_get_user_pgd().
> >
> > Could you elaborate please?
> 
> only call xen_get_user_pgd() when it should be called.

I should have been more explicit. When I was saying "elaborate" I was
soliciting for advice on the 'check condition' and how to make it robust.
> 
> Yinghai

Re: [GIT PULL] x86/mm changes for v3.9-rc1

[Yinghai Lu]

On Sat, Feb 23, 2013 at 11:43 AM, Konrad Rzeszutek Wilk
<konrad@kernel.org> wrote:
>> >> Also you should check condition about calling xen_get_user_pgd().
>> >
>> > Could you elaborate please?
>>
>> only call xen_get_user_pgd() when it should be called.
>
> I should have been more explicit. When I was saying "elaborate" I was
> soliciting for advice on the 'check condition' and how to make it robust.

your 64bit xen_write_cr3_init, is just xen_write_cr3 without
xen_get_user_pgd calling.
plus set xen_write_cr3 to ops.write_cr3.

so if you could have way to find out when xen_get_user_pgd could be used,
new xen_write_cr3_init need to check that, then set ops.write_cr3 and
call ops.write_cr3
otherwise will still call slim version without update ops.write_cr3
and bail out early.

Thanks

Yinghai

Re: [GIT PULL] x86/mm changes for v3.9-rc1

[H. Peter Anvin]

On 02/23/2013 01:37 PM, Yinghai Lu wrote:
> On Sat, Feb 23, 2013 at 11:43 AM, Konrad Rzeszutek Wilk
> <konrad@kernel.org> wrote:
>>>>> Also you should check condition about calling xen_get_user_pgd().
>>>>
>>>> Could you elaborate please?
>>>
>>> only call xen_get_user_pgd() when it should be called.
>>
>> I should have been more explicit. When I was saying "elaborate" I was
>> soliciting for advice on the 'check condition' and how to make it robust.
>
> your 64bit xen_write_cr3_init, is just xen_write_cr3 without
> xen_get_user_pgd calling.
> plus set xen_write_cr3 to ops.write_cr3.
>
> so if you could have way to find out when xen_get_user_pgd could be used,
> new xen_write_cr3_init need to check that, then set ops.write_cr3 and
> call ops.write_cr3
> otherwise will still call slim version without update ops.write_cr3
> and bail out early.
>

Yes, and that's the right thing to do, *or* you can make the init 
version ask "do we have userspace yet" and if so switch the pointer and 
chain the other function.  The latter will save you from having to do a 
test every time.

However, the assumption that we will only set cr3 once before userspace 
is still too fragile.  It makes sense for a quick fix because 32 bits 
already had the same assumption, but this needs to be cleaned up in the 
future.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.