Re: [PATCH v2 8/9] x86/pagewalk: Improve the logic behind setting access and dirty bits

["Jan Beulich" <JBeulich@suse.com>]

>>> On 16.03.17 at 17:31, <andrew.cooper3@citrix.com> wrote:
> @@ -413,24 +417,33 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
>       * success.  Although the PRMs say higher-level _PAGE_ACCESSED bits
>       * get set whenever a lower-level PT is used, at least some hardware
>       * walkers behave this way. */
> -#if GUEST_PAGING_LEVELS == 4 /* 64-bit only... */
> -    if ( set_ad_bits(l4p + guest_l4_table_offset(va), &gw->l4e, 0) )
> -        paging_mark_dirty(d, gw->l4mfn);
> -    if ( set_ad_bits(l3p + guest_l3_table_offset(va), &gw->l3e,
> -                     (pse1G && (walk & PFEC_write_access))) )
> -        paging_mark_dirty(d, gw->l3mfn);
> -#endif
> -    if ( !pse1G )
> +    switch ( leaf_level )
>      {
> +    default:
> +        ASSERT_UNREACHABLE();
> +        break;
> +
> +    case 1:
> +        if ( set_ad_bits(l1p + guest_l1_table_offset(va), &gw->l1e,
> +                         (walk & PFEC_write_access) && leaf_level == 1) )

The comparison on leaf_level is pointless here (other than further
down).

> +            paging_mark_dirty(d, gw->l1mfn);
> +        /* Fallthrough */
> +
> +    case 2:
>          if ( set_ad_bits(l2p + guest_l2_table_offset(va), &gw->l2e,
> -                         (pse2M && (walk & PFEC_write_access))) )
> +                         (walk & PFEC_write_access) && leaf_level == 2) )
>              paging_mark_dirty(d, gw->l2mfn);
> -        if ( !pse2M )
> -        {
> -            if ( set_ad_bits(l1p + guest_l1_table_offset(va), &gw->l1e,
> -                             (walk & PFEC_write_access)) )
> -                paging_mark_dirty(d, gw->l1mfn);
> -        }
> +        /* Fallthrough */
> +
> +#if GUEST_PAGING_LEVELS == 4 /* 64-bit only... */
> +    case 3:
> +        if ( set_ad_bits(l3p + guest_l3_table_offset(va), &gw->l3e,
> +                         (walk & PFEC_write_access) && leaf_level == 3) )
> +            paging_mark_dirty(d, gw->l3mfn);
> +
> +        if ( set_ad_bits(l4p + guest_l4_table_offset(va), &gw->l4e, 0) )

false

Other than that,
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Personally I also think the fall through behavior would be more
immediately visible if you omitted the blank lines between the case
blocks.

Jan


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