Re: [Xen-devel] [PATCH V6 4/4] x86/altp2m: fix display frozen when switching to a new view early

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.11.18 at 18:54, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 11/15/18 7:34 PM, George Dunlap wrote:
>>> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx> 
>>> wrote:
>>> @@ -1440,6 +1443,11 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned 
>>> int i)
>>>     struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>>     struct ept_data *ept;
>>>
>>> +    p2m->max_mapped_pfn = hostp2m->max_mapped_pfn;
>> 
>> Why we need to copy this value?
>> 
>> I’ve just spent the afternoon tracing around the source code, and while I’m 
> pretty sure it won’t hurt, I’m also not sure why it’s necessary.
> 
> I think I did that because I assumed that it would matter for
> ept_get_entry() and misconfigurations, when:
> 
>  954     /* This pfn is higher than the highest the p2m map currently
> holds */
>  955     if ( gfn > p2m->max_mapped_pfn )

The question is whether under any condition such checks require that
the accumulated value be in sync between the host and the various
alt p2m-s.

>  956     {
>  957         for ( i = ept->wl; i > 0; --i )
>  958             if ( (gfn & ~((1UL << (i * EPT_TABLE_ORDER)) - 1)) >
>  959                  p2m->max_mapped_pfn )
>  960                 break;
>  961         goto out;
>  962     }
> 
> but I am not certain it is required either. I can certainly remove this
> assignment and see if anything breaks.

I've seen you mention such or alike a couple of times now while
discussing this series, and I have to admit that I'm a little frightened:
Making a change just based on the observation that nothing breaks
is setting us up for breakage in some corner case. That is - seeing
something break is a good indication that a change was wrong, but
not seeing any breakage doesn't alone justify a change.

In the particular case here I think whether the copying of the field
is needed depends on what else is being copied (I'm sorry, I'm not
that familiar with altp2m): If mappings get cloned from the host p2m,
then this value ought to be cloned too. For any mappings that
might be kept in sync between p2m-s, I think this field also would
need to be updated in sync.

Jan


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