Re: [Xen-devel] [PATCH V6 3/4] x86/mm: allocate logdirty_ranges for altp2ms

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx> 
> wrote:
> 
> This patch is a pre-requisite for the one fixing VGA logdirty
> freezes when using altp2m. It only concerns itself with the
> ranges allocation / deallocation / initialization part. While
> touching the code, I've switched global_logdirty from bool_t
> to bool.
> 
> P2m_reset_altp2m() has been refactored to reduce code
> repetition, and it now takes the p2m lock. Similar
> refactoring has been done with p2m_activate_altp2m().

Thanks!  I think this is almost there.  I have a couple of comments about the 
code below; so since we have to do another round, let me comment on the 
changelog.

It doesn’t make much sense to say you’re “refactoring” a function that you are 
just now introducing in this patch.


I think I’d say something  more like this:

---
For now, only do allocation/deallocation; keeping them in sync will be done in 
subsequent patches.

Logdirty synchronization will only be done for active altp2ms; so allocate 
logdirty rangesets (copying the host logdirty rangeset) when an altp2m is 
activated, and free it when deactivated.

Write a helper function to do altp2m activiation (appropriately handling 
failures).  Also, refactor p2m_reset_altp2m() so that it can be used to remove 
redundant codepaths, fixing the locking while we’re at it.

While we’re here, switch global_logdirty from bool_t to bool.
---

> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 418ff85..abdf443 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2282,6 +2282,34 @@ bool_t p2m_altp2m_lazy_copy(struct vcpu *v, paddr_t 
> gpa,
>     return 1;
> }
> 
> +static void p2m_reset_altp2m(struct domain *d, unsigned int idx,
> +                             bool reset_remapped, bool free_logdirty_ranges)

As Jan says, passing in (…true, false) makes it more difficult to follow the 
code and see what’s going on.  You could use a ‘flags’ structure, as he 
mentions; or alternately, you could pass in an argument that was either 
DEACTIVATE or RESET, and then use that to decide which things to do.

> +{
> +    struct p2m_domain *p2m;
> +
> +    ASSERT(idx < MAX_ALTP2M);
> +    p2m = d->arch.altp2m_p2m[idx];
> +
> +    p2m_lock(p2m);
> +
> +    p2m_flush_table_locked(p2m);
> +    /* Uninit and reinit ept to force TLB shootdown */
> +
> +    if ( free_logdirty_ranges )
> +        p2m_free_logdirty(p2m);
> +
> +    ept_p2m_uninit(p2m);
> +    ept_p2m_init(p2m);

Nit: The comment about uninit/reinit should be just before the uninit/reinit. 
:-)

> +
> +    if ( reset_remapped )
> +    {
> +        p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
> +        p2m->max_remapped_gfn = 0;
> +    }

I don’t think there’s a need to do this conditionally.  In fact, the only 
reason it’s correct *not* to do this for the other two cases is because in 
those cases the p2m will go through p2m_init_altp2m_ept() before being used 
again.  Resetting these is redundant, but harmless, and not worth an extra 
function argument and conditional to avoid.

> +static int p2m_init_altp2m_logdirty(struct p2m_domain *p2m)
> +{
> +    struct p2m_domain *hostp2m = p2m_get_hostp2m(p2m->domain);
> +    int rc = p2m_init_logdirty(p2m);
> +
> +    if ( rc )
> +        return rc;
> +
> +    /* The following is really just a rangeset copy. */
> +    return rangeset_merge(p2m->logdirty_ranges, hostp2m->logdirty_ranges);
> +}
> +
> +static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
> +{
> +    int rc;
> +
> +    ASSERT(idx < MAX_ALTP2M);
> +    rc = p2m_init_altp2m_logdirty(d->arch.altp2m_p2m[idx]);
> +
> +    if ( rc )
> +        return rc;
> +
> +    p2m_init_altp2m_ept(d, idx);

Is there any reason to make these separate functions?  I had in mind a single 
p2m_activate_altp2m() function which would do all three things 
(p2m_init_logdirty, rangeset_merge, and init_altp2m_ept).

Also, I realize it’s _probably_ not necessary to grab the lock here for the p2m 
in question (since it shouldn’t be in use, because altp2m_eptp[] is 
INVALID_MFN); but since it’s not on the hot path, it seems like we might as 
well grab it just to be safe.

Everything else looks good, thanks.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel