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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

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.

So my reasoning was that, since setting p2m->max_mapped_pfn =
hostp2m->max_mapped_pfn is both harmless (as both your and Jan's
analyses appear to confirm) and makes this code correct, that was the
way to go.


Thanks,
Razvan

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