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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 23/03/17 17:09, Tim Deegan wrote:
> 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. :)

I tried that and came to the same conclusion.  This is a static local
helper, so I figured it was fine to stay untyped like this.

>
>>  {
>> -    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.

Oh yes.  (I was concerned about a double read, but that is via guest_p
not walk_p.)  I will drop.

>
>>  
>> -    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.

Will do.

~Andrew

>
> The rest of this LGTM, thanks.
>
> Cheers,
>
> Tim.


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