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

[Andrew Cooper]

On 23/03/17 17:09, Tim Deegan wrote:
> At 16:31 + 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 
>>  #include 
>>  
>> -/* 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. [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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Tim Deegan]

At 16:31 + 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 
>  #include 
>  
> -/* 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. [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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 16.03.17 at 17:31,  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), >l4e, 0) )
> -paging_mark_dirty(d, gw->l4mfn);
> -if ( set_ad_bits(l3p + guest_l3_table_offset(va), >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), >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), >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), >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), >l3e,
> + (walk & PFEC_write_access) && leaf_level == 3) )
> +paging_mark_dirty(d, gw->l3mfn);
> +
> +if ( set_ad_bits(l4p + guest_l4_table_offset(va), >l4e, 0) )

false

Other than that,
Reviewed-by: Jan Beulich 

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