[patch] pae: tlbflush linear page table updates

[patch] pae: tlbflush linear page table updates

[Gerd Knorr]

  Hi,

On l3 page dir updates (and related linear page table updates)
we'll have to flush the tlb to make sure the linear page access
goes to the correct pages ...

  Gerd

--- xen/arch/x86/mm.c~	2005-08-08 12:50:32.000000000 +0200
+++ xen/arch/x86/mm.c	2005-08-08 16:24:30.814980475 +0200
@@ -737,6 +737,7 @@ static int create_pae_xen_mappings(l3_pg
             l2e_from_pfn(l3e_get_pfn(pl3e[i]), __PAGE_HYPERVISOR) :
             l2e_empty();
     unmap_domain_page(pl2e);
+    flush_tlb_all();
 
     return 1;
 }

Re: [patch] pae: tlbflush linear page table updates

[Keir Fraser]

On 8 Aug 2005, at 15:51, Gerd Knorr wrote:

> On l3 page dir updates (and related linear page table updates)
> we'll have to flush the tlb to make sure the linear page access
> goes to the correct pages ...

Have you actually observed problems without the flush?

Otherwise I think a bigger audit of linear mappings may be required and 
I'll hold off adding flushes until we do that.

  -- Keir

Re: [patch] pae: tlbflush linear page table updates

[Gerd Knorr]

On Mon, Aug 08, 2005 at 05:52:29PM +0100, Keir Fraser wrote:
> 
> On 8 Aug 2005, at 15:51, Gerd Knorr wrote:
> 
> >On l3 page dir updates (and related linear page table updates)
> >we'll have to flush the tlb to make sure the linear page access
> >goes to the correct pages ...
> 
> Have you actually observed problems without the flush?

Yes.  PAE xenlinux (UP) didn't boot up.  The first
make_page_readonly() faults because the first copy_from_user()
in ptwr_do_page_fault() fails (which dereferences stuff using
the linear page table ...).

I hoped the tlbflush also fixes the SMP issues, but it doesn't,
so there are more bugs.  I'm not sure the PAE/SMP crashes are
ptwr related, they also happen (although a bit less likely it
seems) without writable page tables.

  Gerd

-- 
panic("it works"); /* avoid being flooded with debug messages */

Re: [patch] pae: tlbflush linear page table updates

[Keir Fraser]

On 9 Aug 2005, at 08:54, Gerd Knorr wrote:

> Yes.  PAE xenlinux (UP) didn't boot up.  The first
> make_page_readonly() faults because the first copy_from_user()
> in ptwr_do_page_fault() fails (which dereferences stuff using
> the linear page table ...).
>
> I hoped the tlbflush also fixes the SMP issues, but it doesn't,
> so there are more bugs.  I'm not sure the PAE/SMP crashes are
> ptwr related, they also happen (although a bit less likely it
> seems) without writable page tables.

Out of interest, can you please try moving the flush_tlb_all() into 
mod_l3_entry(), at the following place:

         if ( unlikely(!UPDATE_ENTRY(l3, pl3e, ol3e, nl3e)) )
         {
             BUG_ON(!create_pae_xen_mappings(pl3e));
             flush_tlb_all(); /* <------- moved to here */
             put_page_from_l3e(nl3e, pfn);
             return 0;
         }

Hopefully that will work just as well.

  -- Keir

Re: [patch] pae: tlbflush linear page table updates

[Keir Fraser]

Actually, I spotted that create_pae_xen_mappings() was being called 
from completely the wrong paths in mod_l3_entry(). I've now fixed that 
-- it's possible that will fix your problems with no need for extra 
flushes. :-)

  -- Keir

On 9 Aug 2005, at 09:06, Keir Fraser wrote:

> Out of interest, can you please try moving the flush_tlb_all() into 
> mod_l3_entry(), at the following place:
>
>         if ( unlikely(!UPDATE_ENTRY(l3, pl3e, ol3e, nl3e)) )
>         {
>             BUG_ON(!create_pae_xen_mappings(pl3e));
>             flush_tlb_all(); /* <------- moved to here */
>             put_page_from_l3e(nl3e, pfn);
>             return 0;
>         }
>
> Hopefully that will work just as well.

Re: [patch] pae: tlbflush linear page table updates

[Gerd Knorr]

  Hi,

> Actually, I spotted that create_pae_xen_mappings() was being called 
> from completely the wrong paths in mod_l3_entry(). I've now fixed that 
> -- it's possible that will fix your problems with no need for extra 
> flushes. :-)

Machine crashes, below is the log with
"printk-via-hypercall-consoleio" hack.  It's not the first
make_page_readonly() call though, so it's some other bug.

It is hg95d2bbf6a273d053e8876944a803b80c086e0b3e.

  Gerd

 __  __            _____  ___         _                _ 
 \ \/ /___ _ __   |___ / / _ \     __| | _____   _____| |
  \  // _ \ '_ \    |_ \| | | |__ / _` |/ _ \ \ / / _ \ |
  /  \  __/ | | |  ___) | |_| |__| (_| |  __/\ V /  __/ |
 /_/\_\___|_| |_| |____(_)___/    \__,_|\___| \_/ \___|_|
                                                         
 http://www.cl.cam.ac.uk/netos/xen
 University of Cambridge Computer Laboratory

 Xen version 3.0-devel (kraxel@ber.suse.de) (gcc version 3.3.5 20050117 (prerelease) (SUSE Linux)) Tue Aug  9 15:36:24 CEST 2005
 Latest ChangeSet: Tue Aug  9 15:08:25 2005 95d2bbf6a273d053e8876944a803b80c086e0b3e

(XEN) Truncating memory map to 1007616kB
(XEN) Physical RAM map:
(XEN)  0000000000000000 - 000000000009fc00 (usable)
(XEN)  000000000009fc00 - 00000000000a0000 (reserved)
(XEN)  00000000000e6000 - 0000000000100000 (reserved)
(XEN)  0000000000100000 - 000000003d800000 (usable)
(XEN) System RAM: 983MB (1007228kB)
(XEN) Xen heap: 10MB (10452kB)
(XEN) PAE enabled, limit: 16 GB
(XEN) found SMP MP-table at 000ff780
(XEN) DMI 2.3 present.
(XEN) Using APIC driver default
(XEN) ACPI: RSDP (v000 ACPIAM                                ) @ 0x000f55c0
(XEN) ACPI: RSDT (v001 INTEL  @ÃS¸¨š
(XEN)  0x20050420 MSFT 0x00000097) @ 0x3f630000
(XEN) ACPI: FADT (v002 INTEL  @ÃS¸¨š
(XEN)  0x20050420 MSFT 0x00000097) @ 0x3f630200
(XEN) ACPI: MADT (v001 INTEL  @ÃS¸¨š
(XEN)  0x20050420 MSFT 0x00000097) @ 0x3f630390
(XEN) ACPI: MCFG (v001 INTEL  @ÃS¸¨š
(XEN)  0x20050420 MSFT 0x00000097) @ 0x3f630400
(XEN) ACPI: MSEG (v001 INTEL  @ÃS¸¨š
(XEN)  0x20050420 MSFT 0x00000097) @ 0x3f630440
(XEN) ACPI: WDDT (v001 INTEL  OEMWDDT  0x00000001 INTL 0x02002026) @ 0x3f6363c0
(XEN) ACPI: DSDT (v001 INTEL  @ÃS¸¨š
(XEN)  0x00000001 INTL 0x02002026) @ 0x00000000
(XEN) ACPI: Local APIC address 0xfee00000
(XEN) ACPI: LAPIC (acpi_id[0x01] lapic_id[0x00] enabled)
(XEN) Processor #0 15:4 APIC version 20
(XEN) ACPI: LAPIC (acpi_id[0x02] lapic_id[0x01] enabled)
(XEN) Processor #1 15:4 APIC version 20
(XEN) ACPI: LAPIC_NMI (acpi_id[0x01] dfl dfl lint[0x1])
(XEN) ACPI: LAPIC_NMI (acpi_id[0x02] dfl dfl lint[0x1])
(XEN) ACPI: IOAPIC (id[0x02] address[0xfec00000] gsi_base[0])
(XEN) IOAPIC[0]: apic_id 2, version 32, address 0xfec00000, GSI 0-23
(XEN) ACPI: INT_SRC_OVR (bus 0 bus_irq 0 global_irq 2 dfl dfl)
(XEN) ACPI: INT_SRC_OVR (bus 0 bus_irq 9 global_irq 9 high level)
(XEN) ACPI: IRQ0 used by override.
(XEN) ACPI: IRQ2 used by override.
(XEN) ACPI: IRQ9 used by override.
(XEN) Enabling APIC mode:  Flat.  Using 1 I/O APICs
(XEN) Using ACPI (MADT) for SMP configuration information
(XEN) Initializing CPU#0
(XEN) Detected 3793.084 MHz processor.
(XEN) Using scheduler: Simple EDF Scheduler (sedf)
(XEN) CPU: Trace cache: 12K uops, L1 D cache: 16K
(XEN) CPU: L2 cache: 2048K
(XEN) CPU: Physical Processor ID: 0
(XEN) VMXON is done
(XEN) CPU0: Intel Genuine Intel(R) CPU 3.80GHz stepping 06
(XEN) Booting processor 1/1 eip 90000
(XEN) Initializing CPU#1
(XEN) CPU: Trace cache: 12K uops, L1 D cache: 16K
(XEN) CPU: L2 cache: 2048K
(XEN) CPU: Physical Processor ID: 0
(XEN) VMXON is done
(XEN) CPU1: Intel Genuine Intel(R) CPU 3.80GHz stepping 06
(XEN) Total of 2 processors activated.
(XEN) ENABLING IO-APIC IRQs
(XEN) ..TIMER: vector=0x31 pin1=2 pin2=-1
(XEN) checking TSC synchronization across 2 CPUs: passed.
(XEN) Platform timer is 1.193MHz PIT
(XEN) Brought up 2 CPUs
(XEN) mtrr: v2.0 (20020519)
(XEN) *** LOADING DOMAIN 0 ***
(XEN) Xen-ELF header found: 'GUEST_OS=linux,GUEST_VER=2.6,XEN_VER=3.0,VIRT_BASE=0xC0000000,PAE=yes,LOADER=generic'
(XEN) PHYSICAL MEMORY ARRANGEMENT:
(XEN)  Dom0 alloc.:   10000000->20000000 (131072 pages to be allocated)
(XEN) VIRTUAL MEMORY ARRANGEMENT:
(XEN)  Loaded kernel: c0100000->c051d144
(XEN)  Init. ramdisk: c051e000->c051e000
(XEN)  Phys-Mach map: c051e000->c05de000
(XEN)  Page tables:   c05de000->c05e7000
(XEN)  Start info:    c05e7000->c05e8000
(XEN)  Boot stack:    c05e8000->c05e9000
(XEN)  TOTAL:         c0000000->c0800000
(XEN)  ENTRY ADDRESS: c0100000
(XEN) Scrubbing Free RAM: ...........done.
(XEN) *** Serial input -> DOM0 (type 'CTRL-a' three times to switch input to Xen).
<5>Linux version 2.6.12.3-xen0-hg95d2bbf6a273d053e8876944a803b80c086e0b3e (kraxel@eskarina) (gcc version 3.3.5 20050117 (prerelease) (SUSE Linux)) #2 Tue Aug 9 15:38:52 CEST 2005
<6>BIOS-provided physical RAM map:
 Xen: 0000000000100000 - 0000000030000000 (usable)
<5>40MB HIGHMEM available.
<5>728MB LOWMEM available.
NX (Execute Disable) protection: active
<7>On node 0 totalpages: 196608
<7>  DMA zone: 186368 pages, LIFO batch:31
<7>  Normal zone: 0 pages, LIFO batch:1
<7>  HighMem zone: 10240 pages, LIFO batch:3
<6>found SMP MP-table at 000ff780
<6>DMI 2.3 present.
<7>ACPI: RSDP (v000 ACPIAM                                ) @ 0x000f55c0
<7>ACPI: RSDT (v001 INTEL  @ÃS¸¨š
 0x20050420 MSFT 0x00000097) @ 0x3f630000
<7>ACPI: FADT (v002 INTEL  @ÃS¸¨š
 0x20050420 MSFT 0x00000097) @ 0x3f630200
<7>ACPI: MADT (v001 INTEL  @ÃS¸¨š
 0x20050420 MSFT 0x00000097) @ 0x3f630390
<7>ACPI: MCFG (v001 INTEL  @ÃS¸¨š
 0x20050420 MSFT 0x00000097) @ 0x3f630400
<7>ACPI: MSEG (v001 INTEL  @ÃS¸¨š
 0x20050420 MSFT 0x00000097) @ 0x3f630440
<7>ACPI: WDDT (v001 INTEL  OEMWDDT  0x00000001 INTL 0x02002026) @ 0x3f6363c0
<7>ACPI: DSDT (v001 INTEL  @ÃS¸¨š
 0x00000001 INTL 0x02002026) @ 0x00000000
<7>ACPI: Local APIC address 0xfee00000
<6>ACPI: LAPIC (acpi_id[0x01] lapic_id[0x00] enabled)
<6>ACPI: LAPIC (acpi_id[0x02] lapic_id[0x01] enabled)
<6>ACPI: LAPIC_NMI (acpi_id[0x01] dfl dfl lint[0x1])
<6>ACPI: LAPIC_NMI (acpi_id[0x02] dfl dfl lint[0x1])
<6>ACPI: IOAPIC (id[0x02] address[0xfec00000] gsi_base[0])
IOAPIC[0]: apic_id 2, version 32, address 0xfec00000, GSI 0-23
<6>ACPI: INT_SRC_OVR (bus 0 bus_irq 0 global_irq 2 dfl dfl)
<6>ACPI: INT_SRC_OVR (bus 0 bus_irq 9 global_irq 9 high level)
<7>ACPI: IRQ0 used by override.
<7>ACPI: IRQ2 used by override.
<7>ACPI: IRQ9 used by override.
Enabling APIC mode:  Flat.  Using 1 I/O APICs
<6>Using ACPI (MADT) for SMP configuration information
<6>IRQ lockup detection disabled
Allocating PCI resources starting at 30000000 (gap: 30000000:d0000000)
Built 1 zonelists
<5>Kernel command line: console=tty0 console=ttyS0 root=/dev/sda2 ro selinux=0 sysrq=yes
<6>Initializing CPU#0
(XEN) DOM0: (file=mm.c, line=1444) Bad type (saw f0000001!= exp a0000000) for pfn 10103
kernel BUG at arch/xen/i386/kernel/cpu/common.c:576 (cpu_gdt_init)!
 [<c04812a7>] cpu_gdt_init+0xa7/0xc0
 [<c021982f>] sort+0x11f/0x1f0
 [<c048137f>] cpu_init+0xbf/0x250
 [<c0213ee0>] cmp_ex+0x0/0x20
 [<c047878c>] start_kernel+0x9c/0x1e0
 [<c0478390>] unknown_bootoption+0x0/0x210
<0>Kernel panic - not syncing: BUG!
 (XEN) Domain 0 shutdown: rebooting machine.

Re: [patch] pae: tlbflush linear page table updates

[Gerd Knorr]

On Tue, Aug 09, 2005 at 03:52:23PM +0200, Gerd Knorr wrote:
>   Hi,
> 
> > Actually, I spotted that create_pae_xen_mappings() was being called 
> > from completely the wrong paths in mod_l3_entry(). I've now fixed that 
> > -- it's possible that will fix your problems with no need for extra 
> > flushes. :-)
> 
> Machine crashes, below is the log with
> "printk-via-hypercall-consoleio" hack.  It's not the first
> make_page_readonly() call though, so it's some other bug.

Hmm, that happens when xen is build with debug=y only, without
that it crashes much earlier ...

  Gerd

Re: [patch] pae: tlbflush linear page table updates

[Keir Fraser]

On 9 Aug 2005, at 16:44, Gerd Knorr wrote:

> Hmm, that happens when xen is build with debug=y only, without
> that it crashes much earlier ...

Weird. The calls to create_pae_xen_mappings were definitely on the 
error paths in mod_l3_entry(), which is obviously wrong. I'm surprised 
that fixing it would make things worse, unless some other patch in the 
meantime has screwed pae...

I'll try and find some time to look at pae myself and see if I can help 
out.

  -- Keir

Re: [patch] pae: tlbflush linear page table updates

[Gerd Knorr]

On Tue, Aug 09, 2005 at 05:14:14PM +0100, Keir Fraser wrote:
> 
> On 9 Aug 2005, at 16:44, Gerd Knorr wrote:
> 
> >Hmm, that happens when xen is build with debug=y only, without
> >that it crashes much earlier ...
> 
> Weird. The calls to create_pae_xen_mappings were definitely on the 
> error paths in mod_l3_entry(), which is obviously wrong. I'm surprised 
> that fixing it would make things worse, unless some other patch in the 
> meantime has screwed pae...

No, it's actually the changeset
6056:a1f7e01b0990a378584e718e6d48eac38824fdb9 which broke it.

The create_pae_xen_mappings() call in the error path is broken
indead.  That must have sneaked in somewhen, I'm pretty sure I
wrote that initially as something like

         if ( unlikely(!UPDATE_ENTRY(l3, pl3e, ol3e, nl3e)) ||
	      !create_pae_xen_mappings(pl3e) )
         {
             put_page_from_l3e(nl3e, pfn);
             return 0;
         }

so create_pae_xen_mappings() failure (due to the guest OS trying
illegal things) will *trigger* the error path.  Strange it never
showed up, maybe linux never ever updates l3 entries after
creating them.

BUG_ON(!create_pae_xen_mappings()) is a bad idea, it _can_ fail,
the failure should just be propagated up (so in the end the
hypercall running into this returns some errror) or the domain
should simply be killed ...

  Gerd

-- 
panic("it works"); /* avoid being flooded with debug messages */

Re: [patch] pae: tlbflush linear page table updates

[Gerd Knorr]

> No, it's actually the changeset
> 6056:a1f7e01b0990a378584e718e6d48eac38824fdb9 which broke it.

And it isn't the mod_l3 update which broke PAE, it's the
__not_mapped() removal.  Applying the bits below reversed
makes PAE boot again.

  Gerd

diff -r 663f0fb1e444 -r a1f7e01b0990 xen/arch/x86/mm.c
--- a/xen/arch/x86/mm.c	Tue Aug  9 09:34:06 2005
+++ b/xen/arch/x86/mm.c	Tue Aug  9 10:42:51 2005
@@ -2902,42 +2887,13 @@
     .cmpxchg8b_emulated = ptwr_emulated_cmpxchg8b
 };
 
-#if defined(__x86_64__)
-/*
- * Returns zero on if mapped, or -1 otherwise
- */
-static int __not_mapped(l2_pgentry_t *pl2e)
-{
-    unsigned long page = read_cr3();
-
-    page &= PAGE_MASK;
-    page = ((unsigned long *) __va(page))[l4_table_offset((unsigned long)pl2e)];
-    if ( !(page & _PAGE_PRESENT) ) 
-        return -1;        
-        
-    page &= PAGE_MASK;
-    page = ((unsigned long *) __va(page))[l3_table_offset((unsigned long)pl2e)];
-    if ( !(page & _PAGE_PRESENT) ) 
-        return -1;
-
-    page &= PAGE_MASK;
-    page = ((unsigned long *) __va(page))[l2_table_offset((unsigned long)pl2e)];
-    if ( !(page & _PAGE_PRESENT) )
-        return -1;
-
-    return 0;
-}
-#else
-#define __not_mapped(p) (0)
-#endif
-
 /* Write page fault handler: check if guest is trying to modify a PTE. */
 int ptwr_do_page_fault(struct domain *d, unsigned long addr)
 {
     unsigned long    pfn;
     struct pfn_info *page;
     l1_pgentry_t     pte;
-    l2_pgentry_t    *pl2e;
+    l2_pgentry_t    *pl2e, l2e;
     int              which;
     unsigned long    l2_idx;
 
@@ -2984,10 +2940,7 @@
     pl2e = &__linear_l2_table[l2_idx];
     which = PTWR_PT_INACTIVE;
 
-    if ( unlikely(__not_mapped(pl2e)) )
-        goto inactive;
-
-    if ( (l2e_get_pfn(*pl2e)) == pfn )
+    if ( (__get_user(l2e.l2, &pl2e->l2) == 0) && (l2e_get_pfn(l2e) == pfn) )
     {
         /*
          * Check the PRESENT bit to set ACTIVE mode.
@@ -2995,13 +2948,11 @@
          * ACTIVE p.t. (it may be the same p.t. mapped at another virt addr).
          * The ptwr_flush call below will restore the PRESENT bit.
          */
-        if ( likely(l2e_get_flags(*pl2e) & _PAGE_PRESENT) ||
+        if ( likely(l2e_get_flags(l2e) & _PAGE_PRESENT) ||
              (d->arch.ptwr[PTWR_PT_ACTIVE].l1va &&
               (l2_idx == d->arch.ptwr[PTWR_PT_ACTIVE].l2_idx)) )
             which = PTWR_PT_ACTIVE;
     }
-
-  inactive:
 
     /*
      * If this is a multi-processor guest then ensure that the page is hooked

Re: [patch] pae: tlbflush linear page table updates

[Keir Fraser]

On 10 Aug 2005, at 11:22, Gerd Knorr wrote:

> so create_pae_xen_mappings() failure (due to the guest OS trying
> illegal things) will *trigger* the error path.  Strange it never
> showed up, maybe linux never ever updates l3 entries after
> creating them.

I think it preallocates all the L2's so mod_l3_entry probably isn't 
used. But yes: it looks like I broke it. :-)

> BUG_ON(!create_pae_xen_mappings()) is a bad idea, it _can_ fail,
> the failure should just be propagated up (so in the end the
> hypercall running into this returns some errror) or the domain
> should simply be killed ...

The checks that can fail only really belong in alloc_l3_table. An 
already typed l3 should not need those checks on its highest l3e 
(mod_l3_entry explicitly disallows meddling with that entry).

  -- Keir

Re: [patch] pae: tlbflush linear page table updates

[Keir Fraser]

On 10 Aug 2005, at 11:28, Gerd Knorr wrote:

> And it isn't the mod_l3 update which broke PAE, it's the
> __not_mapped() removal.  Applying the bits below reversed
> makes PAE boot again.

afaics the only change for pae is use of __get_user() rather than 
directly reading from the pl2e. Perhaps __get_user() on 8-byte 
quantities is broken, although I'm sure that must be used elsewhere and 
so is tested....

  -- Keir

Re: [patch] pae: tlbflush linear page table updates

[Gerd Knorr]

On Wed, Aug 10, 2005 at 11:54:22AM +0100, Keir Fraser wrote:
> 
> On 10 Aug 2005, at 11:28, Gerd Knorr wrote:
> 
> >And it isn't the mod_l3 update which broke PAE, it's the
> >__not_mapped() removal.  Applying the bits below reversed
> >makes PAE boot again.
> 
> afaics the only change for pae is use of __get_user() rather than 
> directly reading from the pl2e. Perhaps __get_user() on 8-byte 
> quantities is broken, although I'm sure that must be used elsewhere and 
> so is tested....

Sure?  The patch below fixes it for me.

The tlbflush stuff is red herring btw, the real difference is
optimization.  Build with "optimize=n" boot fine, others don't.

  Gerd

Index: xen/arch/x86/mm.c
===================================================================
--- xen.orig/arch/x86/mm.c	2005-08-10 14:09:59.476430318 +0200
+++ xen/arch/x86/mm.c	2005-08-10 14:13:50.906986616 +0200
@@ -2940,7 +2940,7 @@ int ptwr_do_page_fault(struct domain *d,
     pl2e = &__linear_l2_table[l2_idx];
     which = PTWR_PT_INACTIVE;
 
-    if ( (__get_user(l2e.l2, &pl2e->l2) == 0) && (l2e_get_pfn(l2e) == pfn) )
+    if ( (__copy_from_user(&l2e, pl2e, sizeof(l2e)) == 0) && (l2e_get_pfn(l2e) == pfn) )
     {
         /*
          * Check the PRESENT bit to set ACTIVE mode.

Re: [patch] pae: tlbflush linear page table updates

[Gerd Knorr]

  Hi,

> Sure?  The patch below fixes it for me.

Uhm, well, it doesn't really fix it.

> The tlbflush stuff is red herring btw, the real difference is
> optimization.  Build with "optimize=n" boot fine, others don't.

While looking into this I've noticed the bug comes and goes away
with the optimization level.  Building with -O1 works fine,
building with -O2 breaks (both with the patch mailed applied).

There might be something wrong with the inline assembler ...

  Gerd

-- 
panic("it works"); /* avoid being flooded with debug messages */

Re: [patch] pae: tlbflush linear page table updates

[Keir Fraser]

On 10 Aug 2005, at 14:01, Gerd Knorr wrote:

>> Sure?  The patch below fixes it for me.
>
> Uhm, well, it doesn't really fix it.
>
>> The tlbflush stuff is red herring btw, the real difference is
>> optimization.  Build with "optimize=n" boot fine, others don't.
>
> While looking into this I've noticed the bug comes and goes away
> with the optimization level.  Building with -O1 works fine,
> building with -O2 breaks (both with the patch mailed applied).
>
> There might be something wrong with the inline assembler ...

See the fix I just this minute checked in :-)

Hopefully it should fix all these weirdnesses....

  -- Keir

Re: [patch] pae: tlbflush linear page table updates

[Gerd Knorr]

> >There might be something wrong with the inline assembler ...
> 
> See the fix I just this minute checked in :-)
> 
> Hopefully it should fix all these weirdnesses....

Looks good, the odd bugs seem to be gone now.
Small change, big effect ;)

  Gerd

RE: [patch] pae: tlbflush linear page table updates

[Tian, Kevin]

>From: xen-devel-bounces@lists.xensource.com
>[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser
>Sent: Wednesday, August 10, 2005 9:12 PM
>On 10 Aug 2005, at 14:01, Gerd Knorr wrote:
>
>>> Sure?  The patch below fixes it for me.
>>
>> Uhm, well, it doesn't really fix it.
>>
>>> The tlbflush stuff is red herring btw, the real difference is
>>> optimization.  Build with "optimize=n" boot fine, others don't.
>>
>> While looking into this I've noticed the bug comes and goes away
>> with the optimization level.  Building with -O1 works fine,
>> building with -O2 breaks (both with the patch mailed applied).
>>
>> There might be something wrong with the inline assembler ...
>
>See the fix I just this minute checked in :-)
>
>Hopefully it should fix all these weirdnesses....
>
>  -- Keir

Is similar fix also required by __put_user_64?

#define __put_user_u64(x, addr, retval, errret)                 \
	...
	: "=r"(retval)                                  \
    : "A" (x), "r" (addr), "i"(errret), "0"(retval))

I'm not sure exact rules for compiler to choose registers. But once
compiler optimizes retval or addr to reuse eax/edx under some condition,
user will also see either incorrect return value, or incorrect content
written to incorrect address. Can we assume such optimization never
happen?

Thanks,
Kevin

RE: [patch] pae: tlbflush linear page table updates

[Tian, Kevin]

>From: Keir Fraser [mailto:Keir.Fraser@cl.cam.ac.uk]
>
>The compiler will never alias inputs. In this case it is forced to
>alias the output constraint with input retval, and so the output cannot
>alias with any other input (which would be an error).
>
>  -- Keir

Yes, this is reasonable policy. ;-)
Thanks,
Kevin

Re: [patch] pae: tlbflush linear page table updates

[Keir Fraser]

On 12 Aug 2005, at 10:02, Tian, Kevin wrote:

> Is similar fix also required by __put_user_64?

No.

> #define __put_user_u64(x, addr, retval, errret)                 \
> 	...
> 	: "=r"(retval)                                  \
>     : "A" (x), "r" (addr), "i"(errret), "0"(retval))
>
> I'm not sure exact rules for compiler to choose registers. But once
> compiler optimizes retval or addr to reuse eax/edx under some 
> condition,
> user will also see either incorrect return value, or incorrect content
> written to incorrect address. Can we assume such optimization never
> happen?

The compiler will never alias inputs. In this case it is forced to 
alias the output constraint with input retval, and so the output cannot 
alias with any other input (which would be an error).

  -- Keir

Re: [patch] pae: tlbflush linear page table updates

[Andi Kleen]

Keir Fraser <Keir.Fraser@cl.cam.ac.uk> writes:
> 
> The compiler will never alias inputs. In this case it is forced to
> alias the output constraint with input retval, and so the output
> cannot alias with any other input (which would be an error).

Actually it will sometimes. That is what the "early clobber" (&) 
prevents. Perhaps it should be used here.

-Andi 

RE: [patch] pae: tlbflush linear page table updates

[Tian, Kevin]

>-----Original Message-----
>From: ak@suse.de [mailto:ak@suse.de]
>Sent: Friday, August 12, 2005 5:21 PM
>Keir Fraser <Keir.Fraser@cl.cam.ac.uk> writes:
>>
>> The compiler will never alias inputs. In this case it is forced to
>> alias the output constraint with input retval, and so the output
>> cannot alias with any other input (which would be an error).
>
>Actually it will sometimes. That is what the "early clobber" (&)
>prevents. Perhaps it should be used here.
>
>-Andi

"early clobber" is good to prevent output alias as input, if that output
may be clobbered before input is used. However there seems no point to
simply alias among inputs. Or else, the only way I can see is that
compiler insert some extra lines in the middle of inline asm... Any
benefit for this likelihood?

Thanks,
Kevin

Re: [patch] pae: tlbflush linear page table updates

[Keir Fraser]

On 12 Aug 2005, at 10:20, Andi Kleen wrote:

>> The compiler will never alias inputs. In this case it is forced to
>> alias the output constraint with input retval, and so the output
>> cannot alias with any other input (which would be an error).
>
> Actually it will sometimes. That is what the "early clobber" (&)
> prevents. Perhaps it should be used here.

Do you have an example?

  -- Keir

Re: [patch] pae: tlbflush linear page table updates

[Andi Kleen]

> "early clobber" is good to prevent output alias as input, if that output
> may be clobbered before input is used. However there seems no point to
> simply alias among inputs. Or else, the only way I can see is that
> compiler insert some extra lines in the middle of inline asm... Any
> benefit for this likelihood?

There can be cases e.g. when the inputs are dependent and you use
general enough constraints. e.g. one input can be %reg and the other
offset(%reg). In this case they will essentially alias. 

-Andi

Re: [patch] pae: tlbflush linear page table updates

[Keir Fraser]

On 12 Aug 2005, at 10:56, Andi Kleen wrote:

>> "early clobber" is good to prevent output alias as input, if that 
>> output
>> may be clobbered before input is used. However there seems no point to
>> simply alias among inputs. Or else, the only way I can see is that
>> compiler insert some extra lines in the middle of inline asm... Any
>> benefit for this likelihood?
>
> There can be cases e.g. when the inputs are dependent and you use
> general enough constraints. e.g. one input can be %reg and the other
> offset(%reg). In this case they will essentially alias.

That would be a nasty one to track down. :-)

It seems the general policy in Linux also is not to bother with '&' on 
outputs that have a forced alias in the input list. I'd be happy to see 
a cleanup patch that adds more '&'s to Xen though -- in cases where we 
are relying on the forced alias then it's unlikely to change the 
generated assembly code.

  -- Keir