Re: [PATCH] delte PAGE_ORDER_1G in pod

[George Dunlap <george.dunlap@citrix.com>]

On 26/04/16 08:27, zhangcy wrote:
> PoD does not have cache list for 1GB pages.
> 
> Signed-off-by: zhangcy <zhangcy@cn.fujitsu.com>

Thanks for the patch.  FYI we normally tag the area in the title in a
structured way; I probably would have used something like the following:

xen/pod: Remove code handling PAGE_ORDER_1G from p2m_pod_cache_add

But with regards to the patch itself: The question isn't whether we have
a cache list for 1G pages; the question is whether p2m_pod_cache_add()
will ever be called with order == PAGE_ORDER_1G.

Taking a quick glance around, it looks like in theory if a guest called
decrease_reservation with order == PAGE_ORDER_1G, you could conceivably
get to p2m_pod_cache_add() with order == PAGE_ORDER_1G.

Even if the answer is "no", that may change in the future; which means
we need to at very least add an ASSERT(), and possibly add a more robust
failure case.  And at that point, since handling it properly only
requires 4 lines, you might as well just handle it.

Thanks,
 -George

> ---
>  xen/arch/x86/mm/p2m-pod.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m-pod.c b/xen/arch/x86/mm/p2m-pod.c
> index a931f2c..89a07ee 100644
> --- a/xen/arch/x86/mm/p2m-pod.c
> +++ b/xen/arch/x86/mm/p2m-pod.c
> @@ -122,10 +122,6 @@ p2m_pod_cache_add(struct p2m_domain *p2m,
>      /* Then add to the appropriate populate-on-demand list. */
>      switch ( order )
>      {
> -    case PAGE_ORDER_1G:
> -        for ( i = 0; i < (1UL << PAGE_ORDER_1G); i += 1UL << PAGE_ORDER_2M )
> -            page_list_add_tail(page + i, &p2m->pod.super);
> -        break;
>      case PAGE_ORDER_2M:
>          page_list_add_tail(page, &p2m->pod.super);
>          break;
> 


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