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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 11/16/18 2:10 PM, Razvan Cojocaru wrote:
> On 11/16/18 2:03 PM, George Dunlap wrote:
>> The code is definitely complicated enough, though, that I may have
>> missed something, which is why I asked Razvan if there was a reason he
>> changed it.
>>
>> For the purposes of this patch, I propose having p2m_altp2m_init_ept()
>> set max_mapped_pfn to 0 (if that works), and leaving "get rid of
>> max_remapped_pfn" for a future clean-up series.
> 
> I've retraced my previous analysis and re-ran some tests, and I now
> remember (sorry it took a while) why the p2m->max_mapped_pfn =
> hostp2m->max_mapped_pfn was both necessary and not accidental.
> 
> Let's say we set it to 0 in p2m_altp2m_init_ept(). Then,
> hap_track_dirty_vram() calls p2m_change_type_range(), which calls the
> newly added change_type_range().
> 
> Change_type_range() looks like this:
> 
> static void change_type_range(struct p2m_domain *p2m,
>                               unsigned long start, unsigned long end,
>                               p2m_type_t ot, p2m_type_t nt)
> {
>     unsigned long gfn = start;
>     struct domain *d = p2m->domain;
>     int rc = 0;
> 
>     p2m->defer_nested_flush = 1;
> 
>     if ( unlikely(end > p2m->max_mapped_pfn) )
>     {
>         if ( !gfn )
>         {
>             p2m->change_entry_type_global(p2m, ot, nt);
>             gfn = end;
>         }
>         end = p2m->max_mapped_pfn + 1;
>     }
>     if ( gfn < end )
>         rc = p2m->change_entry_type_range(p2m, ot, nt, gfn, end - 1);
>     if ( rc )
>     {
>         printk(XENLOG_G_ERR "Error %d changing Dom%d GFNs [%lx,%lx] from
> %d to %d\n",
>                rc, d->domain_id, start, end - 1, ot, nt);
>         domain_crash(d);
>     }
> 
>     switch ( nt )
>     {
>     case p2m_ram_rw:
>         if ( ot == p2m_ram_logdirty )
>             rc = rangeset_remove_range(p2m->logdirty_ranges, start, end
> - 1);
>         break;
>     case p2m_ram_logdirty:
>         if ( ot == p2m_ram_rw )
>             rc = rangeset_add_range(p2m->logdirty_ranges, start, end - 1);
>         break;
>     default:
>         break;
>     }
>     if ( rc )
>     {
>         printk(XENLOG_G_ERR "Error %d manipulating Dom%d's log-dirty
> ranges\n",
>                rc, d->domain_id);
>         domain_crash(d);
>     }
> 
>     p2m->defer_nested_flush = 0;
>     if ( nestedhvm_enabled(d) )
>         p2m_flush_nestedp2m(d);
> }
> 
> If we set p2m->max_mapped_pfn to 0, we're guaranteed to run into the if
> ( unlikely(end > p2m->max_mapped_pfn) ) body, where end =
> p2m->max_mapped_pfn + 1; will make end 1.
> 
> Then, we will crash the hypervisor in rangeset_add_range(), where
> there's an ASSERT() stating that start <= end.

Ah, right, this was the original crash that you ran into several months
ago, which flagged up the whole logdirty range synchronization issue.

But that's partly a logic hole in change_entry_type_range(), which
assumes that start < p2m->max_mapped_pfn.  It would be better to fix
that than to work around it by changing the meaning of max_mapped_pfn.

On the other hand, we want the logdirty rangesets to actually match the
host's rangesets; using altp2m->max_mapped_pfn for this is clearly
wrong. The easiest fix would be just to explicitly use the host's
max_mapped_pfn when calculating the clipping.  A more complete fix would
involve calculating two different ranges -- a "rangeset" range and a
"invalidate" range, the second of which would be clipped on altp2ms by
{min,max}_remapped_gfn.

Something like the attached (compile-tested only).  I'm partial to
having both patches applied, but I'd be open to arguments that we should
only use the first.

 -George

Attachment: 0001-p2m-Always-use-hostp2m-when-clipping-rangesets.patch
Description: Text Data

Attachment: 0002-p2m-change_range_type-Only-invalidate-remapped-gfns.patch
Description: Text Data

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