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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.03.2020 22:21, David Woodhouse wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -422,7 +422,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;

Coming back to an earlier request of mine: There are no locks being
held here afaics, so with

#define page_is_broken(pg)         (pgc_is_broken((pg)->count_info))

and

#define pgc_is_broken(pgc)         (pgc_is(pgc, broken) || \
                                    pgc_is(pgc, broken_offlining))

there's a chance that the page gets transitioned from
broken_offlining to broken (by another CPU) between these two
checks, resulting in wrong returned state. Either the latter macro
uses an intermediate variable and ACCESS_ONCE() or, as suggested
before, enumerators get arranged such that the check can be done
(e.g. using binary masking operations) with a single evaluation of
pgc.

This may or may not also be an issue for the other two pgc_is_*(),
but I think at least for symmetry they should then follow suit. At
the very least all three macros need to be immune to uses like
page_is_offlined(pg++) or similar argument expressions with side
effects.

> @@ -1699,19 +1706,18 @@ unsigned int online_page(mfn_t mfn, uint32_t *status)
>      do {
>          ret = *status = 0;
>  
> -        if ( y & PGC_broken )
> +        if ( pgc_is_broken(y) )
>          {
>              ret = -EINVAL;
> -            *status = PG_ONLINE_FAILED |PG_ONLINE_BROKEN;
> +            *status = PG_ONLINE_FAILED | PG_ONLINE_BROKEN;
>              break;
>          }
> -
> -        if ( (y & PGC_state) == PGC_state_offlined )
> +        else if ( pgc_is(y, offlined) )

At the risk of getting flamed again: Even if it was a matter of
taste in new code whether to use "else" in a case like this one,
this isn't new code, and it is in no way necessary to change what
we have for the purpose of this patch. I.e. without even having
to resort to the question of whether personal taste decisions are
to be accepted, this simply falls under "no unrelated /
unnecessary changes please". (FAOD this includes the deletion of
the blank line then as well.)

> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -67,16 +67,32 @@
>   /* 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)
> + /*
> +  * 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) /* Broken and offlining */
> +#define PGC_state_broken           PG_mask(5, 9) /* Broken and offlined */
> +
> +#define pgc_is(pgc, st)            (((pgc) & PGC_state) == PGC_state_##st)
> +#define page_state_is(pg, st)       pgc_is((pg)->count_info, st)
> +
> +#define pgc_is_broken(pgc)         (pgc_is(pgc, broken) || \
> +                                    pgc_is(pgc, broken_offlining))
> +#define pgc_is_offlined(pgc)       (pgc_is(pgc, offlined) || \
> +                                    pgc_is(pgc, broken))
> +#define pgc_is_offlining(pgc)      (pgc_is(pgc, offlining) || \
> +                                    pgc_is(pgc, broken_offlining))
> +
> +#define page_is_broken(pg)         (pgc_is_broken((pg)->count_info))
> +#define page_is_offlined(pg)       (pgc_is_broken((pg)->count_info))
> +#define page_is_offlining(pg)      (pgc_is_broken((pg)->count_info))

Copy-and-paste mistake (rhs is the same for all three; same for Arm)?
Also there's no need here for the outer pairs of parentheses.

Also, for the next version, may I ask that you number versions in
the subject's tag and that you provide a brief description of
changes from the previous version (if any, but there ought to be
some in a series for there to be a point to send out)? Thanks.

Jan