Re: [Xen-devel] [PATCH 1/2] xen/mm: fold PGC_broken into PGC_state bits

[Julien Grall <julien@xen.org>]

Hi David,

Could you please send the series in a separate thread? So we don't mix 
the two discussions (i.e merge and free on boot allocated page) together.

On 07/02/2020 15:57, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Only PGC_state_offlining and PGC_state_offlined are valid in conjunction
> with PGC_broken. The other two states (free and inuse) were never valid
> for a broken page.
> 
> By folding PGC_broken in, we can have three bits for PGC_state which
> allows up to 8 states, of which 6 are currently used and 2 are available
> for new use cases.

The idea looks good to me. I have a few  mostly nitpicking comment below.

> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   xen/arch/x86/domctl.c    |  2 +-
>   xen/common/page_alloc.c  | 67 ++++++++++++++++++++++++----------------
>   xen/include/asm-arm/mm.h | 26 +++++++++++-----
>   xen/include/asm-x86/mm.h | 33 +++++++++++++-------
>   4 files changed, 82 insertions(+), 46 deletions(-)
> 
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 4fa9c91140..17a318e16d 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -415,7 +415,7 @@ long arch_do_domctl(
>                   if ( page->u.inuse.type_info & PGT_pinned )
>                       type |= XEN_DOMCTL_PFINFO_LPINTAB;
>   
> -                if ( page->count_info & PGC_broken )
> +                if ( page_is_broken(page) )
>                       type = XEN_DOMCTL_PFINFO_BROKEN;
>               }
>   
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 97902d42c1..4084503554 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1093,7 +1093,7 @@ static int reserve_offlined_page(struct page_info *head)
>           struct page_info *pg;
>           int next_order;
>   
> -        if ( page_state_is(cur_head, offlined) )
> +        if ( page_is_offlined(cur_head) )
>           {
>               cur_head++;
>               if ( first_dirty != INVALID_DIRTY_IDX && first_dirty )
> @@ -1113,7 +1113,7 @@ static int reserve_offlined_page(struct page_info *head)
>               for ( i = (1 << cur_order), pg = cur_head + (1 << cur_order );
>                     i < (1 << next_order);
>                     i++, pg++ )
> -                if ( page_state_is(pg, offlined) )
> +                if ( page_is_offlined(pg) )
>                       break;
>               if ( i == ( 1 << next_order) )
>               {
> @@ -1145,16 +1145,19 @@ static int reserve_offlined_page(struct page_info *head)
>   
>       for ( cur_head = head; cur_head < head + ( 1UL << head_order); cur_head++ )
>       {
> -        if ( !page_state_is(cur_head, offlined) )
> +        struct page_list_head *list;

We tend to add a newline after a series of declaration.

> +        if ( page_state_is(cur_head, offlined) )
> +            list = &page_offlined_list;
> +        else if (page_state_is(cur_head, broken) )
> +            list = &page_broken_list;
> +        else
>               continue;
>   
>           avail[node][zone]--;
>           total_avail_pages--;
>           ASSERT(total_avail_pages >= 0);
>   
> -        page_list_add_tail(cur_head,
> -                           test_bit(_PGC_broken, &cur_head->count_info) ?
> -                           &page_broken_list : &page_offlined_list);
> +        page_list_add_tail(cur_head, list);
>   
>           count++;
>       }
> @@ -1404,13 +1407,16 @@ static void free_heap_pages(
>           switch ( pg[i].count_info & PGC_state )
>           {
>           case PGC_state_inuse:
> -            BUG_ON(pg[i].count_info & PGC_broken);
>               pg[i].count_info = PGC_state_free;
>               break;
>   
>           case PGC_state_offlining:
> -            pg[i].count_info = (pg[i].count_info & PGC_broken) |
> -                               PGC_state_offlined;
> +            pg[i].count_info = PGC_state_offlined;
> +            tainted = 1;
> +            break;
> +
> +        case PGC_state_broken_offlining:
> +            pg[i].count_info = PGC_state_broken;
>               tainted = 1;
>               break;
>   
> @@ -1527,16 +1533,25 @@ static unsigned long mark_page_offline(struct page_info *pg, int broken)
>       do {
>           nx = x = y;
>   
> -        if ( ((x & PGC_state) != PGC_state_offlined) &&
> -             ((x & PGC_state) != PGC_state_offlining) )
> +        nx &= ~PGC_state;
> +
> +        switch ( x & PGC_state )
>           {
> -            nx &= ~PGC_state;
> -            nx |= (((x & PGC_state) == PGC_state_free)
> -                   ? PGC_state_offlined : PGC_state_offlining);
> -        }
> +        case PGC_state_inuse:
> +        case PGC_state_offlining:
> +            nx |= broken ? PGC_state_offlining : PGC_state_broken_offlining;
> +            break;
> +
> +        case PGC_state_free:
> +            nx |= broken ? PGC_state_broken : PGC_state_offlined;
>   
> -        if ( broken )
> -            nx |= PGC_broken;
> +        case PGC_state_broken_offlining:
> +            nx |= PGC_state_broken_offlining;
> +
> +        case PGC_state_offlined:
> +        case PGC_state_broken:
> +            nx |= PGC_state_broken;

Shouldn't this be:

case PGC_state_offlined:
     nx |= broken ? PGC_state_offlined : PGC_state_broken;

case PGC_state_broken:
     nx |= PGC_state_broken;

There are also quite a difference with the default case now. Without 
this patch, if you were to add a new state but not handling it here, you 
would transition to PGC_state_offlining. With this patch, we will 
transtion to 0 (i.e PGC_state_inuse for now).

PGC_state_* are not an enum, the compiler can't help to catch new state 
that doesn't have a corresponding case. So I would suggest to add a 
default matching the current behavior and adding an 
ASSERT_UNREACHABLE(). Note that I am open to a different kind of 
handling here.

> +        }
>   
>           if ( x == nx )
>               break;
> @@ -1609,7 +1624,7 @@ int offline_page(mfn_t mfn, int broken, uint32_t *status)
>        * need to prevent malicious guest access the broken page again.
>        * Under such case, hypervisor shutdown guest, preventing recursive mce.
>        */
> -    if ( (pg->count_info & PGC_broken) && (owner = page_get_owner(pg)) )
> +    if ( page_is_broken(pg) && (owner = page_get_owner(pg)) )
>       {
>           *status = PG_OFFLINE_AGAIN;
>           domain_crash(owner);
> @@ -1620,7 +1635,7 @@ int offline_page(mfn_t mfn, int broken, uint32_t *status)
>   
>       old_info = mark_page_offline(pg, broken);
>   
> -    if ( page_state_is(pg, offlined) )
> +    if ( page_is_offlined(pg) )
>       {
>           reserve_heap_page(pg);
>   
> @@ -1699,14 +1714,14 @@ unsigned int online_page(mfn_t mfn, uint32_t *status)
>       do {
>           ret = *status = 0;
>   
> -        if ( y & PGC_broken )
> +        if ( (y & PGC_state) == PGC_state_broken ||
> +             (y & PGC_state) == PGC_state_broken_offlining )

This is a bit a shame we can't use page_is_broken. Would it make sense 
to introduce an helper that just take a count_info?

>           {
>               ret = -EINVAL;
>               *status = PG_ONLINE_FAILED |PG_ONLINE_BROKEN;
>               break;
>           }
> -
> -        if ( (y & PGC_state) == PGC_state_offlined )
> +        else if ( (y & PGC_state) == PGC_state_offlined )
>           {
>               page_list_del(pg, &page_offlined_list);
>               *status = PG_ONLINE_ONLINED;
> @@ -1747,11 +1762,11 @@ int query_page_offline(mfn_t mfn, uint32_t *status)
>   
>       pg = mfn_to_page(mfn);
>   
> -    if ( page_state_is(pg, offlining) )
> +    if ( page_is_offlining(pg) )
>           *status |= PG_OFFLINE_STATUS_OFFLINE_PENDING;
> -    if ( pg->count_info & PGC_broken )
> +    if ( page_is_broken(pg) )
>           *status |= PG_OFFLINE_STATUS_BROKEN;
> -    if ( page_state_is(pg, offlined) )
> +    if ( page_is_offlined(pg) )
>           *status |= PG_OFFLINE_STATUS_OFFLINED;
>   
>       spin_unlock(&heap_lock);
> @@ -2483,7 +2498,7 @@ __initcall(pagealloc_keyhandler_init);
>   
>   void scrub_one_page(struct page_info *pg)
>   {
> -    if ( unlikely(pg->count_info & PGC_broken) )
> +    if ( unlikely(page_is_broken(pg)) )
>           return;
>   
>   #ifndef NDEBUG
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index 333efd3a60..c9466c8ca0 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -112,13 +112,25 @@ struct page_info
>   /* Page is broken? */
>   #define _PGC_broken       PG_shift(7)
>   #define PGC_broken        PG_mask(1, 7)

Shouldn't this be dropped now?

> - /* Mutually-exclusive page states: { inuse, offlining, offlined, free }. */
> -#define PGC_state         PG_mask(3, 9)
> -#define PGC_state_inuse   PG_mask(0, 9)
> -#define PGC_state_offlining PG_mask(1, 9)
> -#define PGC_state_offlined PG_mask(2, 9)
> -#define PGC_state_free    PG_mask(3, 9)
> -#define page_state_is(pg, st) (((pg)->count_info&PGC_state) == PGC_state_##st)
> + /*
> +  * Mutually-exclusive page states:
> +  * { inuse, offlining, offlined, free, broken_offlining, broken }
> +  */
> +#define PGC_state                  PG_mask(7, 9)
> +#define PGC_state_inuse            PG_mask(0, 9)
> +#define PGC_state_offlining        PG_mask(1, 9)
> +#define PGC_state_offlined         PG_mask(2, 9)
> +#define PGC_state_free             PG_mask(3, 9)
> +#define PGC_state_broken_offlining PG_mask(4, 9)
> +#define PGC_state_broken           PG_mask(5, 9)

I agree that making all the value aligned make it nicer to read, but 
this is increasing number of "unrelated" changes and makes the review 
more difficult.

I would prefer if we leave the indentation alone for the current 
#define. But I am not going to push for it :).

> +
> +#define page_state_is(pg, st)      (((pg)->count_info&PGC_state) == PGC_state_##st)
> +#define page_is_broken(pg)         (page_state_is((pg), broken_offlining) ||  \
> +                                    page_state_is((pg), broken))
> +#define page_is_offlined(pg)       (page_state_is((pg), broken) ||    \
> +                                    page_state_is((pg), offlined))
> +#define page_is_offlining(pg)      (page_state_is((pg), broken_offlining) || \
> +                                    page_state_is((pg), offlining))
>   
>   /* Count of references to this frame. */
>   #define PGC_count_width   PG_shift(9)
> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> index 2ca8882ad0..3edadf7a7c 100644
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -67,18 +67,27 @@
>    /* 3-bit PAT/PCD/PWT cache-attribute hint. */
>   #define PGC_cacheattr_base PG_shift(6)
>   #define PGC_cacheattr_mask PG_mask(7, 6)
> - /* Page is broken? */
> -#define _PGC_broken       PG_shift(7)
> -#define PGC_broken        PG_mask(1, 7)
> - /* Mutually-exclusive page states: { inuse, offlining, offlined, free }. */
> -#define PGC_state         PG_mask(3, 9)
> -#define PGC_state_inuse   PG_mask(0, 9)
> -#define PGC_state_offlining PG_mask(1, 9)
> -#define PGC_state_offlined PG_mask(2, 9)
> -#define PGC_state_free    PG_mask(3, 9)
> -#define page_state_is(pg, st) (((pg)->count_info&PGC_state) == PGC_state_##st)
> -
> - /* Count of references to this frame. */
> + /*
> +  * Mutually-exclusive page states:
> +  * { inuse, offlining, offlined, free, broken_offlining, broken }
> +  */
> +#define PGC_state                  PG_mask(7, 9)
> +#define PGC_state_inuse            PG_mask(0, 9)
> +#define PGC_state_offlining        PG_mask(1, 9)
> +#define PGC_state_offlined         PG_mask(2, 9)
> +#define PGC_state_free             PG_mask(3, 9)
> +#define PGC_state_broken_offlining PG_mask(4, 9)
> +#define PGC_state_broken           PG_mask(5, 9)
> +
> +#define page_state_is(pg, st)      (((pg)->count_info&PGC_state) == PGC_state_##st)
> +#define page_is_broken(pg)         (page_state_is((pg), broken_offlining) ||  \
> +                                    page_state_is((pg), broken))
> +#define page_is_offlined(pg)       (page_state_is((pg), broken) ||    \
> +                                    page_state_is((pg), offlined))
> +#define page_is_offlining(pg)      (page_state_is((pg), broken_offlining) || \
> +                                    page_state_is((pg), offlining))
> +
> +/* Count of references to this frame. */
>   #define PGC_count_width   PG_shift(9)
>   #define PGC_count_mask    ((1UL<<PGC_count_width)-1)
>   
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel