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

[Tim Deegan <tim@xxxxxxx>]

At 16:31 +0000 on 16 Mar (1489681902), Andrew Cooper wrote:
> --- a/xen/arch/x86/mm/guest_walk.c
> +++ b/xen/arch/x86/mm/guest_walk.c
> @@ -32,24 +32,28 @@ asm(".file \"" __OBJECT_FILE__ "\"");
>  #include <asm/page.h>
>  #include <asm/guest_pt.h>
>  
> -/* Modify a guest pagetable entry to set the Accessed and Dirty bits.
> - * Returns non-zero if it actually writes to guest memory. */
> -static uint32_t set_ad_bits(void *guest_p, void *walk_p, int set_dirty)
> +/*
> + * Modify a guest pagetable entry to set the Accessed and Dirty bits.
> + * Returns non-zero if it actually writes to guest memory.

s/non-zero/true/.

> + */
> +static bool set_ad_bits(void *_guest_p, void *_walk_p, bool set_dirty)

Would it work to have this take guest_intpte_t * and have the caller
pass e.g. &l4p[guest_l4_table_offset(va)].l4 ?  Not very much prettier, I
suppose. :)

>  {
> -    guest_intpte_t old, new;
> +    guest_intpte_t *guest_p = _guest_p, *walk_p = _walk_p;
> +    guest_intpte_t new, old = ACCESS_ONCE(*walk_p);

Why ACCESS_ONCE?  The walk is unlikely to change underfoot.

>  
> -    old = *(guest_intpte_t *)walk_p;
>      new = old | _PAGE_ACCESSED | (set_dirty ? _PAGE_DIRTY : 0);
> -    if ( old != new ) 
> +    if ( old != new )
>      {
> -        /* Write the new entry into the walk, and try to write it back
> +        /*
> +         * Write the new entry into the walk, and try to write it back
>           * into the guest table as well.  If the guest table has changed
> -         * under out feet then leave it alone. */
> -        *(guest_intpte_t *)walk_p = new;
> -        if ( cmpxchg(((guest_intpte_t *)guest_p), old, new) == old ) 
> -            return 1;
> +         * under out feet then leave it alone.

s/out/our/ while we're here. 

The rest of this LGTM, thanks.

Cheers,

Tim.

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