Re: [Xen-devel] [PATCH v3 9/9] mm: Make sure pages are scrubbed

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.04.17 at 17:37, <boris.ostrovsky@xxxxxxxxxx> wrote:
> --- a/xen/Kconfig.debug
> +++ b/xen/Kconfig.debug
> @@ -114,6 +114,13 @@ config DEVICE_TREE_DEBUG
>         logged in the Xen ring buffer.
>         If unsure, say N here.
>  
> +config SCRUB_DEBUG
> +    bool "Page scrubbing test"
> +    default DEBUG
> +    ---help---
> +      Verify that pages that need to be scrubbed before being allocated to
> +      a guest are indeed scrubbed.

Indentation.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -694,6 +694,31 @@ static void page_list_add_scrub(struct page_info *pg, 
> unsigned int node,
>          page_list_add(pg, &heap(node, zone, order));
>  }
>  
> +#define SCRUB_BYTE_PATTERN 0xc2c2c2c2c2c2c2c2

This likely needs a ULL suffix at least for the ARM32 build.

> @@ -912,6 +937,11 @@ static struct page_info *alloc_heap_pages(
>           * guest can control its own visibility of/through the cache.
>           */
>          flush_page_to_ram(page_to_mfn(&pg[i]));
> +
> +#ifdef CONFIG_SCRUB_DEBUG
> +        if ( d && !is_idle_domain(d) )
> +            check_one_page(&pg[i]);
> +#endif

I don't see the need for the is_idle_domain() check.

Also it would be nice for readability if the #ifdef-s were limited to
the function definitions - if their bodies are empty, I'd expect the
compiler to eliminate altogether both the if() here and ...

> @@ -1341,6 +1371,11 @@ static void free_heap_pages(
>              pg[i].count_info |= PGC_need_scrub;
>  
>          node_need_scrub[node] += (1UL << order);
> +
> +#ifdef CONFIG_SCRUB_DEBUG
> +        for ( i = 0; i < (1 << order); i++ )
> +            poison_one_page(&pg[i]);
> +#endif

... the loop here.

> @@ -1637,6 +1672,14 @@ static void init_heap_pages(
>              nr_pages -= n;
>          }
>  
> +#ifdef CONFIG_SCRUB_DEBUG
> +        /*
> +         * These pages get into heap and are allocated to dom0 before
> +         * boot scrub happens.
> +         * Not scrubbing them here will cause failure in check_one_page().
> +         */
> +        scrub_one_page(pg + i);
> +#endif

Isn't this introducing unacceptable overhead on bigger systems,
the more that this is then redundant with boot time scrubbing? To
me, such overhead would be discouraging to ever enable this
config option...

> @@ -2170,6 +2213,9 @@ void free_domheap_pages(struct page_info *pg, unsigned 
> int order)
>              {
>                  BUG_ON((pg[i].u.inuse.type_info & PGT_count_mask) != 0);
>                  arch_free_heap_page(d, &pg[i]);
> +#ifdef CONFIG_SCRUB_DEBUG
> +                scrub_one_page(&pg[i]);
> +#endif

This too looks to be redundant in the important case of reclaiming
memory from a dying domain (i.e. when scrubbing is to happen
anyway).

> @@ -2273,7 +2319,8 @@ void scrub_one_page(struct page_info *pg)
>  
>  #ifndef NDEBUG
>      /* Avoid callers relying on allocations returning zeroed pages. */
> -    unmap_domain_page(memset(__map_domain_page(pg), 0xc2, PAGE_SIZE));
> +    unmap_domain_page(memset(__map_domain_page(pg),
> +                             SCRUB_BYTE_PATTERN & 0xff, PAGE_SIZE));

This makes it a requirement for SCRUB_BYTE_PATTERN to consist of
all identical bytes. There should at least be a respective comment
next to its definition; even better would be if that #define made
sure this requirement is fulfilled by producing a suitably wide value
from just an initial 1-byte one.

Jan


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