Xen devel - Jan 2010 - Changeset 20209 causes an issue in xen_in_range()

Currently PERCPU_SIZE is 2 4K-pages and only 1 page is used actually.
>From xen 20209 on, the second unused page is freed and returned to domheap
in debug=n build (MEMORY_GUARD is not defined):
percpu_free_unused_areas() -> free_xen_data() -> init_xenheap_pages().
Later the returned pages could be allocated to dom0 and dom0 could use them as
DMA buffer.

However, in iommu_set_dom0_mapping(), xen_in_range() is still True for the freed
pages above, so devices in Dom0 can meet with DMA fault.

We may have 2 options:
1) enhance xen_in_range() and check the freed pages. This means we should refer
to the logic of percpu_free_unused_areas().
2) don''t free the unused per_cpu pages: we will waste NR_CPUS pages.
e.g., if NR_CPUS is 256, we''ll waste 1M memory. Looks this is not a big
issue?

I personally perfer option 2 as it''s simple :-)
And actually long ago we also had frame_table checked in xen_in_range(), but 
gave up the checking later as it''s not easy to implement the exact
checking.

Any suggestions?

PS,  in percpu_free_unused_areas():
  data_size = (data_size + PAGE_SIZE + 1) & PAGE_MASK;
here "PAGE_SIZE + 1" should be "PAGE_SIZE - 1"?
 
Thanks,
-- Dexuan
 

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

>>> "Cui, Dexuan" <dexuan.cui@intel.com> 14.01.10 09:38
>>>
>We may have 2 options:
>1) enhance xen_in_range() and check the freed pages. This means we should
refer to the logic of percpu_free_unused_areas().
>2) don''t free the unused per_cpu pages: we will waste NR_CPUS
pages. e.g., if NR_CPUS is 256, we''ll waste 1M memory. Looks this is
not a big issue?
>
>I personally perfer option 2 as it''s simple :-)
I would definitely prefer option 1. Even more I would prefer a solution to avoid
allocating two pages of per-CPU data when only one is needed, though doing that
would seem tricky.
>And actually long ago we also had frame_table checked in xen_in_range(), but
gave up the checking later as it''s not easy to implement the exact
checking.
That''s certainly not the only set of ranges of concern. We either trust
Dom0 or need a general mechanism (perhaps using range sets) to track all memory
in use by Xen. Under tboot, the former may not be an option...

Jan


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

On 14/01/2010 08:38, "Cui, Dexuan" <dexuan.cui@intel.com> wrote:
> Currently PERCPU_SIZE is 2 4K-pages and only 1 page is used actually.
> 
> From xen 20209 on, the second unused page is freed and returned to domheap
in
> debug=n build (MEMORY_GUARD is not defined):
> percpu_free_unused_areas() -> free_xen_data() ->
init_xenheap_pages().
> Later the returned pages could be allocated to dom0 and dom0 could use them
as
> DMA buffer.
> 
> However, in iommu_set_dom0_mapping(), xen_in_range() is still True for the
> freed pages above, so devices in Dom0 can meet with DMA fault.
Should be fixed by c/s 20803.

 Thanks,
 Keir



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

>>> Keir Fraser <keir.fraser@eu.citrix.com> 14.01.10 11:04
>>>
>Should be fixed by c/s 20803.
How can you know the c/s number if not even the staging tree has it yet?

Jan


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

On 14/01/2010 10:09, "Jan Beulich" <JBeulich@novell.com> wrote:
>>>> Keir Fraser <keir.fraser@eu.citrix.com> 14.01.10 11:04
>>>
>> Should be fixed by c/s 20803.
> 
> How can you know the c/s number if not even the staging tree has it yet?
I can see it in my very own private tree. :-)

 -- Keir



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

>>> Keir Fraser <keir.fraser@eu.citrix.com> 14.01.10 11:04
>>>
>On 14/01/2010 08:38, "Cui, Dexuan" <dexuan.cui@intel.com>
wrote:
>
>> Currently PERCPU_SIZE is 2 4K-pages and only 1 page is used actually.
>> 
>> From xen 20209 on, the second unused page is freed and returned to
domheap in
>> debug=n build (MEMORY_GUARD is not defined):
>> percpu_free_unused_areas() -> free_xen_data() ->
init_xenheap_pages().
>> Later the returned pages could be allocated to dom0 and dom0 could use
them as
>> DMA buffer.
>> 
>> However, in iommu_set_dom0_mapping(), xen_in_range() is still True for
the
>> freed pages above, so devices in Dom0 can meet with DMA fault.
>
>Should be fixed by c/s 20803.
Do you really think so? Masking "start" with PERCPU_SIZE-1
doesn''t
make sense, as it may be arbitrarily before __percpu_start and
not aligned. Below my take on it.

Jan

--- 2010-01-06.orig/xen/arch/x86/setup.c	2010-01-05 13:29:13.000000000 +0100
+++ 2010-01-06/xen/arch/x86/setup.c	2010-01-14 11:03:10.000000000 +0100
@@ -230,7 +230,7 @@ static void __init percpu_free_unused_ar
     /* Free all unused per-cpu data areas. */
     free_xen_data(&__per_cpu_start[first_unused << PERCPU_SHIFT],
__bss_start);
 
-    data_size = (data_size + PAGE_SIZE + 1) & PAGE_MASK;
+    BUG_ON(data_size & ~PAGE_MASK);
     if ( data_size != PERCPU_SIZE )
         for ( i = 0; i < first_unused; i++ )
             free_xen_data(&__per_cpu_start[(i << PERCPU_SHIFT) +
data_size],
@@ -1200,7 +1200,7 @@ int xen_in_range(paddr_t start, paddr_t 
     int i;
     static struct {
         paddr_t s, e;
-    } xen_regions[4];
+    } xen_regions[3];
 
     /* initialize first time */
     if ( !xen_regions[0].s )
@@ -1211,10 +1211,6 @@ int xen_in_range(paddr_t start, paddr_t 
         /* hypervisor code + data */
         xen_regions[1].s =__pa(&_stext);
         xen_regions[1].e = __pa(&__init_begin);
-        /* per-cpu data */
-        xen_regions[2].s = __pa(&__per_cpu_start);
-        xen_regions[2].e = xen_regions[2].s +
-            (((paddr_t)last_cpu(cpu_possible_map) + 1) << PERCPU_SHIFT);
         /* bss */
         xen_regions[3].s = __pa(&__bss_start);
         xen_regions[3].e = __pa(&_end);
@@ -1226,6 +1222,14 @@ int xen_in_range(paddr_t start, paddr_t 
             return 1;
     }
 
+    /* per-cpu data */
+    for_each_possible_cpu(i)
+    {
+        if ( (start < __pa(&__per_cpu_data_end[i <<
PERCPU_SHIFT])) &&
+             (end > __pa(&__per_cpu_start[i << PERCPU_SHIFT])) )
+            return 1;
+    }
+
     return 0;
 }
 
--- 2010-01-06.orig/xen/arch/x86/xen.lds.S	2009-10-15 11:42:12.000000000 +0200
+++ 2010-01-06/xen/arch/x86/xen.lds.S	2010-01-14 11:01:19.000000000 +0100
@@ -104,10 +104,10 @@ SECTIONS
        *(.data.percpu)
        . = ALIGN(SMP_CACHE_BYTES);
        *(.data.percpu.read_mostly)
+       . = ALIGN(PAGE_SIZE);
        __per_cpu_data_end = .;
   } :text
   . = __per_cpu_start + (NR_CPUS << PERCPU_SHIFT);
-  . = ALIGN(PAGE_SIZE);
 
   /*
    * Do not insert anything here - the unused portion of .data.percpu



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

On 14/01/2010 11:21, "Jan Beulich" <JBeulich@novell.com> wrote:
>>> However, in iommu_set_dom0_mapping(), xen_in_range() is still True
for the
>>> freed pages above, so devices in Dom0 can meet with DMA fault.
>> 
>> Should be fixed by c/s 20803.
> 
> Do you really think so? Masking "start" with PERCPU_SIZE-1
doesn''t
> make sense, as it may be arbitrarily before __percpu_start and
> not aligned. Below my take on it.
How about c/s 20806?

 -- Keir



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

I am curious that all bits of cpu_possible_map are all set to 1, so the related
checks in percpu_free_unused_areas are unnecessary ?
Xiantao


Keir Fraser wrote:
> On 14/01/2010 11:21, "Jan Beulich" <JBeulich@novell.com>
wrote:
> 
>>>> However, in iommu_set_dom0_mapping(), xen_in_range() is still
True
>>>> for the freed pages above, so devices in Dom0 can meet with DMA
>>>> fault. 
>>> 
>>> Should be fixed by c/s 20803.
>> 
>> Do you really think so? Masking "start" with PERCPU_SIZE-1
doesn''t
>> make sense, as it may be arbitrarily before __percpu_start and
>> not aligned. Below my take on it.
> 
> How about c/s 20806?
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

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

At some point we will introduce hotplug notifiers and have per-cpu areas set
up only for present CPUs. The possible_map is basically deprecated/obsolete.

 -- keir

On 14/01/2010 12:19, "Zhang, Xiantao" <xiantao.zhang@intel.com>
wrote:
> I am curious that all bits of cpu_possible_map are all set to 1, so the
> related checks in percpu_free_unused_areas are unnecessary ?
> Xiantao
> 
> 
> Keir Fraser wrote:
>> On 14/01/2010 11:21, "Jan Beulich"
<JBeulich@novell.com> wrote:
>> 
>>>>> However, in iommu_set_dom0_mapping(), xen_in_range() is
still True
>>>>> for the freed pages above, so devices in Dom0 can meet with
DMA
>>>>> fault. 
>>>> 
>>>> Should be fixed by c/s 20803.
>>> 
>>> Do you really think so? Masking "start" with
PERCPU_SIZE-1 doesn''t
>>> make sense, as it may be arbitrarily before __percpu_start and
>>> not aligned. Below my take on it.
>> 
>> How about c/s 20806?
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
> 


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

>>> Keir Fraser <keir.fraser@eu.citrix.com> 14.01.10 12:47
>>>
>How about c/s 20806?
Yes, that one looks fine to me.

Jan


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

Okay, seems a cleanup patch for that is needed, otherwise the related logic
maybe misleading...
Xiantao
Keir Fraser wrote:
> At some point we will introduce hotplug notifiers and have per-cpu
> areas set up only for present CPUs. The possible_map is basically
> deprecated/obsolete. 
> 
>  -- keir
> 
> On 14/01/2010 12:19, "Zhang, Xiantao"
<xiantao.zhang@intel.com> wrote:
> 
>> I am curious that all bits of cpu_possible_map are all set to 1, so
>> the related checks in percpu_free_unused_areas are unnecessary ?
>> Xiantao
>> 
>> 
>> Keir Fraser wrote:
>>> On 14/01/2010 11:21, "Jan Beulich"
<JBeulich@novell.com> wrote:
>>> 
>>>>>> However, in iommu_set_dom0_mapping(), xen_in_range() is
still
>>>>>> True for the freed pages above, so devices in Dom0 can
meet with
>>>>>> DMA fault.
>>>>> 
>>>>> Should be fixed by c/s 20803.
>>>> 
>>>> Do you really think so? Masking "start" with
PERCPU_SIZE-1 doesn''t
>>>> make sense, as it may be arbitrarily before __percpu_start and
>>>> not aligned. Below my take on it.
>>> 
>>> How about c/s 20806?
>>> 
>>> 
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xensource.com
>>> http://lists.xensource.com/xen-devel

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

Well, there maybe a bit of unneeded code hanging around, but I don''t
think
it''s that bad or particularly misleading.

 -- Keir


On 14/01/2010 13:13, "Zhang, Xiantao" <xiantao.zhang@intel.com>
wrote:
> Okay, seems a cleanup patch for that is needed, otherwise the related logic
> maybe misleading...
> Xiantao
> Keir Fraser wrote:
>> At some point we will introduce hotplug notifiers and have per-cpu
>> areas set up only for present CPUs. The possible_map is basically
>> deprecated/obsolete.
>> 
>>  -- keir
>> 
>> On 14/01/2010 12:19, "Zhang, Xiantao"
<xiantao.zhang@intel.com> wrote:
>> 
>>> I am curious that all bits of cpu_possible_map are all set to 1, so
>>> the related checks in percpu_free_unused_areas are unnecessary ?
>>> Xiantao
>>> 
>>> 
>>> Keir Fraser wrote:
>>>> On 14/01/2010 11:21, "Jan Beulich"
<JBeulich@novell.com> wrote:
>>>> 
>>>>>>> However, in iommu_set_dom0_mapping(),
xen_in_range() is still
>>>>>>> True for the freed pages above, so devices in Dom0
can meet with
>>>>>>> DMA fault.
>>>>>> 
>>>>>> Should be fixed by c/s 20803.
>>>>> 
>>>>> Do you really think so? Masking "start" with
PERCPU_SIZE-1 doesn''t
>>>>> make sense, as it may be arbitrarily before __percpu_start
and
>>>>> not aligned. Below my take on it.
>>>> 
>>>> How about c/s 20806?
>>>> 
>>>> 
>>>> _______________________________________________
>>>> Xen-devel mailing list
>>>> Xen-devel@lists.xensource.com
>>>> http://lists.xensource.com/xen-devel
> 


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