Re: [Xen-devel] [PATCH V8 4/7] x86/p2m: refactor p2m_reset_altp2m()

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 11/21/18 10:49 AM, Jan Beulich wrote:
>>>> On 21.11.18 at 11:19, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> Refactor p2m_reset_altp2m() so that it can be used to remove
>> redundant codepaths, fixing the locking while we're at it.
> 
> Still no word about ...
> 
>> +static void p2m_reset_altp2m(struct domain *d, unsigned int idx,
>> +                             enum altp2m_reset_type reset_type)
>> +{
>> +    struct p2m_domain *p2m;
>> +
>> +    ASSERT(idx < MAX_ALTP2M);
>> +    p2m = d->arch.altp2m_p2m[idx];
>> +
>> +    p2m_lock(p2m);
>> +
>> +    p2m_flush_table_locked(p2m);
>> +
>> +    if ( reset_type == ALTP2M_DEACTIVATE )
>> +        p2m_free_logdirty(p2m);
>> +
>> +    /* Uninit and reinit ept to force TLB shootdown */
>> +    ept_p2m_uninit(p2m);
>> +    ept_p2m_init(p2m);
>> +
>> +    p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
>> +    p2m->max_remapped_gfn = 0;
> 
> ... these now done even when previously they weren't? In fact,
> having looked again, it seems as if their omission from e.g.
> p2m_flush_altp2m() was a mistake before, which you now fix.

As Razvan says, it wasn't a correctness issue; in the other two
"reset-like" bits of code, the altp2m idx was disabled; which guaranteed
that before being used again it would go through p2m_init_altp2m_ept(),
which resets them.

His first version of this patch actually had these set conditionally, so
they'd only be reset in the case where they were reset originally. I
asked him to take it out: saving two redundant memory writes isn't worth
the extra conditional and the extra argument.

So the code is the way it should be; but it should be mentioned in the
commit message, so that reviewers / archaeologists can simply verify it
rather than re-discovering it.

 -George



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