Re: [PATCH 06/16] x86/shadow: purge {write,cmpxchg}_guest_entry() hooks

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 22/03/2023 9:32 am, Jan Beulich wrote:
> --- a/xen/arch/x86/pv/mm.h
> +++ b/xen/arch/x86/pv/mm.h
> @@ -32,6 +34,35 @@ static inline l1_pgentry_t guest_get_eff
>  }
>  
>  /*
> + * Write a new value into the guest pagetable, and update the
> + * paging-assistance state appropriately.  Returns false if we page-faulted,
> + * true for success.
> + */

I know you're just moving the comments as-are, but more than half of
this is definitely wrong now, and another part is wholly redundant.

"Write a new value into the guest pagetable" is about the best I can
think of, but it is borderline whether it even needs a comment.

> +static inline void paging_write_guest_entry(
> +    struct vcpu *v, intpte_t *p, intpte_t new, mfn_t gmfn)
> +{
> +    if ( unlikely(paging_mode_shadow(v->domain)) )
> +        shadow_write_guest_entry(v, p, new, gmfn);
> +    else
> +        write_atomic(p, new);
> +}
> +
> +
> +/*
> + * Cmpxchg a new value into the guest pagetable, and update the
> + * paging-assistance state appropriately.  Returns false if we page-faulted,
> + * true if not.  N.B. caller should check the value of "old" to see if the
> + * cmpxchg itself was successful.
> + */

"Compare and exchange a guest pagetable entry.  Returns the old value." 
We don't need to teach people how to use cmpxchg as a primitive here...

The comment next to shadow_cmpxchg_guest_entry() ideally wants the
grammar fix in the first clause too, and this is probably the right
patch to do it in.

For the content of the change, definitely an improvement.  With the
comments suitably adjusted, Reviewed-by: Andrew Cooper
<andrew.cooper3@xxxxxxxxxx>