page fault in xmem_pool_alloc w/ TXT

page fault in xmem_pool_alloc w/ TXT

[Cihula, Joseph]

In the latest changesets of xen-unstable, when Xen is booted with tboot/TXT, it gives a page fault in xmem_pool_alloc():
...
(XEN) [VT-D]dmar.c:585:   RMRR region: base_addr 8f305000 end_address 8f305fff
(XEN) PCI: MCFG configuration 0: base a0000000 segment 0 buses 0 - 255
(XEN) PCI: MCFG area at a0000000 reserved in E820
(XEN) Xen ERST support is initialized.
(XEN) Using ACPI (MADT) for SMP configuration information
(XEN) IRQ limits: 48 GSI, 4576 MSI/MSI-X
(XEN) Using scheduler: SMP Credit Scheduler (credit)
(XEN) Early fatal page fault at e008:ffff82c48012ac66 (cr2=000000000000ec18, ec)
(XEN) Stack dump: ffff83036fff9868 0000000000000060 ffff83036ffe6000 0000000000
...

We have bisected this to the following commit:
c/s 23013
ACPI: large cleanup

In some cases, entire files turned out unnecessary. Of what remains,
move whatever possible into .init.*, and some data items into
.data.read_mostly.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

Can someone take a look and see what got moved into .init.* that shouldn't have been?

Joe

Re: page fault in xmem_pool_alloc w/ TXT

[Keir Fraser]

Cc'ing the patch author.

 -- Keir

On 24/03/2011 15:41, "Cihula, Joseph" <joseph.cihula@intel.com> wrote:

> In the latest changesets of xen-unstable, when Xen is booted with tboot/TXT,
> it gives a page fault in xmem_pool_alloc():
> ...
> (XEN) [VT-D]dmar.c:585:   RMRR region: base_addr 8f305000 end_address 8f305fff
> (XEN) PCI: MCFG configuration 0: base a0000000 segment 0 buses 0 - 255
> (XEN) PCI: MCFG area at a0000000 reserved in E820
> (XEN) Xen ERST support is initialized.
> (XEN) Using ACPI (MADT) for SMP configuration information
> (XEN) IRQ limits: 48 GSI, 4576 MSI/MSI-X
> (XEN) Using scheduler: SMP Credit Scheduler (credit)
> (XEN) Early fatal page fault at e008:ffff82c48012ac66 (cr2=000000000000ec18,
> ec)
> (XEN) Stack dump: ffff83036fff9868 0000000000000060 ffff83036ffe6000
> 0000000000
> ...
> 
> We have bisected this to the following commit:
> c/s 23013
> ACPI: large cleanup
> 
> In some cases, entire files turned out unnecessary. Of what remains,
> move whatever possible into .init.*, and some data items into
> .data.read_mostly.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> 
> Can someone take a look and see what got moved into .init.* that shouldn't
> have been?
> 
> Joe
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: page fault in xmem_pool_alloc w/ TXT

[Jan Beulich]

>>> On 24.03.11 at 16:41, "Cihula, Joseph" <joseph.cihula@intel.com> wrote:
> In the latest changesets of xen-unstable, when Xen is booted with tboot/TXT, 
> it gives a page fault in xmem_pool_alloc():
> ...
> (XEN) [VT-D]dmar.c:585:   RMRR region: base_addr 8f305000 end_address 
> 8f305fff
> (XEN) PCI: MCFG configuration 0: base a0000000 segment 0 buses 0 - 255
> (XEN) PCI: MCFG area at a0000000 reserved in E820
> (XEN) Xen ERST support is initialized.
> (XEN) Using ACPI (MADT) for SMP configuration information
> (XEN) IRQ limits: 48 GSI, 4576 MSI/MSI-X
> (XEN) Using scheduler: SMP Credit Scheduler (credit)
> (XEN) Early fatal page fault at e008:ffff82c48012ac66 (cr2=000000000000ec18, 
> ec)
> (XEN) Stack dump: ffff83036fff9868 0000000000000060 ffff83036ffe6000 
> 0000000000
> ...
> 
> We have bisected this to the following commit:
> c/s 23013
> ACPI: large cleanup
> 
> In some cases, entire files turned out unnecessary. Of what remains,
> move whatever possible into .init.*, and some data items into
> .data.read_mostly.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> 
> Can someone take a look and see what got moved into .init.* that shouldn't 
> have been?

A crash that early can't be due to something having got moved
into .init.* - if you can get me the full log and the corresponding
xen-syms, I'll certainly check.

Jan

RE: page fault in xmem_pool_alloc w/ TXT

[Jan Beulich]

This appears to be caused by that changset's change to
xen/drivers/passthrough/vtd/dmar.c - I didn't notice that
tboot_parse_dmar_table() would call acpi_parse_dmar() on a copy of
the table rather than the original, and I also can't see why it needs to
do so. Just not freeing the table copy should at least get the crash
resolved, but the zapping then still wouldn't work. Me agreeing to
reverting that part of the change needs explanation why things need
to be done this way (i.e. why, if Dom0 can find the real table anyway,
Xen can't use it directly), and would perhaps force quite a bit of code
back out of .init.*, which I'd really like to avoid. Plus, this code in
tboot_parse_dmar_table() looks more like a hack currently.

Hmm, wait - an apparently simple alternative might be to have
acpi_parse_dmar() or acpi_dmar_init() do what get_dmar() did
before (rather than simply using acpi_parse_dmar()'s input to
store into dmar_table). Could you give the patch below a try
(I kept the disabling of IRQs, but I don't think that is necessary
anymore)?

Jan

--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -673,7 +673,6 @@ static int __init acpi_parse_dmar(struct
     u8 dmar_host_address_width;
     int ret = 0;
 
-    dmar_table = table;
     dmar = (struct acpi_table_dmar *)table;
 
     if ( !iommu_enabled )
@@ -762,6 +761,13 @@ out:
 
 int __init acpi_dmar_init(void)
 {
+    unsigned long flags;
+
+    /* Disabling IRQs avoids cross-CPU TLB flush in map_pages_to_xen(). */
+    local_irq_save(flags);
+    acpi_get_table(ACPI_SIG_DMAR, 0, &dmar_table);
+    local_irq_restore(flags);
+
     return parse_dmar_table(acpi_parse_dmar);
 }
 

RE: page fault in xmem_pool_alloc w/ TXT

[Wei, Gang]

Jan Beulich wrote on 2011-03-25:
> This appears to be caused by that changset's change to
> xen/drivers/passthrough/vtd/dmar.c - I didn't notice that
> tboot_parse_dmar_table() would call acpi_parse_dmar() on a copy of the
> table rather than the original, and I also can't see why it needs to
> do so. Just not freeing the table copy should at least get the crash
> resolved, but the zapping then still wouldn't work. Me agreeing to
> reverting that part of the change needs explanation why things need to
> be done this way (i.e. why, if Dom0 can find the real table anyway,
> Xen can't use it directly), and would perhaps force quite a bit of
> code back out of .init.*, which I'd really like to avoid. Plus, this
> code in
> tboot_parse_dmar_table() looks more like a hack currently.
> 
> Hmm, wait - an apparently simple alternative might be to have
> acpi_parse_dmar() or acpi_dmar_init() do what get_dmar() did before
> (rather than simply using acpi_parse_dmar()'s input to store into
> dmar_table). Could you give the patch below a try (I kept the
> disabling of IRQs, but I don't think that is necessary anymore)?

I applied this patch on c/s 23013 and tested, it makes things working again.

BTW, just looked into the code before 23013, the original acpi_dmar_reinstate() should never work because acpi_get_table(ACPI_SIG_DMAR, 0, &dmar_table) could not find the dmar table after it was zapped during the booting. That may be reason why tboot S3 don't work recently. I haven't verified whether tboot S3 is working after apply this patch yet, and may give it a try next week.

Jimmy

> 
> Jan
> 
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -673,7 +673,6 @@ static int __init acpi_parse_dmar(struct
>      u8 dmar_host_address_width;
>      int ret = 0;
> -    dmar_table = table;
>      dmar = (struct acpi_table_dmar *)table;
>      
>      if ( !iommu_enabled )
> @@ -762,6 +761,13 @@ out:
> 
>  int __init acpi_dmar_init(void)
>  {
> +    unsigned long flags;
> +
> +    /* Disabling IRQs avoids cross-CPU TLB flush in map_pages_to_xen(). */
> +    local_irq_save(flags);
> +    acpi_get_table(ACPI_SIG_DMAR, 0, &dmar_table);
> +    local_irq_restore(flags);
> +
>      return parse_dmar_table(acpi_parse_dmar);
>  }
>



Jimmy

RE: page fault in xmem_pool_alloc w/ TXT

[Cihula, Joseph]

> From: Jan Beulich [mailto:JBeulich@novell.com]
> Sent: Friday, March 25, 2011 4:49 AM
> 
> This appears to be caused by that changset's change to
> xen/drivers/passthrough/vtd/dmar.c - I didn't notice that
> tboot_parse_dmar_table() would call acpi_parse_dmar() on a copy of
> the table rather than the original, and I also can't see why it needs to
> do so. Just not freeing the table copy should at least get the crash

As part of the TXT launch process, the DMAR table will get copy to the TXT heap where SINIT will verify it for correctness.  It is copied to the TXT heap to prevent any malicious DMA modification of the table during and after verification.  Since it is this copy that is known to be good, if Xen is launched with tboot then it will use the verified copy.

> resolved, but the zapping then still wouldn't work. Me agreeing to
> reverting that part of the change needs explanation why things need
> to be done this way (i.e. why, if Dom0 can find the real table anyway,
> Xen can't use it directly), and would perhaps force quite a bit of code
> back out of .init.*, which I'd really like to avoid. Plus, this code in
> tboot_parse_dmar_table() looks more like a hack currently.
> 
> Hmm, wait - an apparently simple alternative might be to have
> acpi_parse_dmar() or acpi_dmar_init() do what get_dmar() did
> before (rather than simply using acpi_parse_dmar()'s input to
> store into dmar_table). Could you give the patch below a try
> (I kept the disabling of IRQs, but I don't think that is necessary
> anymore)?
> 
> Jan
> 
> --- a/xen/drivers/passthrough/vtd/dmar.c
> +++ b/xen/drivers/passthrough/vtd/dmar.c
> @@ -673,7 +673,6 @@ static int __init acpi_parse_dmar(struct
>      u8 dmar_host_address_width;
>      int ret = 0;
> 
> -    dmar_table = table;
>      dmar = (struct acpi_table_dmar *)table;
> 
>      if ( !iommu_enabled )
> @@ -762,6 +761,13 @@ out:
> 
>  int __init acpi_dmar_init(void)
>  {
> +    unsigned long flags;
> +
> +    /* Disabling IRQs avoids cross-CPU TLB flush in map_pages_to_xen(). */
> +    local_irq_save(flags);
> +    acpi_get_table(ACPI_SIG_DMAR, 0, &dmar_table);
> +    local_irq_restore(flags);
> +
>      return parse_dmar_table(acpi_parse_dmar);
>  }
> 
>