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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.11.18 at 12:48, <george.dunlap@xxxxxxxxxx> wrote:
> 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.

Oh. That looks like a layering violation. I certainly didn't expect
these fields to be reset outside of p2m.c + altp2m.c, and the
resets missing from p2m_flush_altp2m() therefore looked like
a mistake.

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

Right, that's what I was trying to get at with the first sentence
(question).

Jan



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