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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 10/18/18 1:57 PM, Andrew Cooper wrote:
> On 18/10/18 11:07, Razvan Cojocaru wrote:
>> When an new altp2m view is created very early on guest boot, the
>> display will freeze (although the guest will run normally). This
>> may also happen on resizing the display. The reason is the way
>> Xen currently (mis)handles logdirty VGA: it intentionally
>> misconfigures VGA pages so that they will fault.
>>
>> The problem is that it only does this in the host p2m. Once we
>> switch to a new altp2m, the misconfigured entries will no longer
>> fault, so the display will not be updated.
>>
>> This patch:
>> * updates ept_handle_misconfig() to use the active altp2m instead
>>   of the hostp2m;
>> * allocates new logdirty ranges for each altp2m;
>> * has p2m_init_altp2m_ept() copy over max_mapped_pfn,
>>   and global_logdirty, and merges the logdirty ranges of the
>>   hostp2m into the logdirty range of the altp2m;
>> * modifies p2m_change_entry_type_global(), p2m_memory_type_changed
>>   and p2m_change_type_range() to propagate their changes to all
>>   valid altp2ms.
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> Suggested-by: George Dunlap <george.dunlap@citrix.com>
> 
>>From the looks of this patch, it would be cleaner to split out the patch
> for allocating/freeing resources from the patch which implements the
> logdirty merging.
> 
> On the allocating/freeing side of things specifically, ...

Thanks for the review! I was poking the patch around to see how I might
split it as indicated, and there's a small problem: I've moved
p2m->logdirty_ranges and p2m->global_logdirty under
p2m->sync.logdirty_ranges and p2m->sync.global_logdirty respectively.

This was recommended by George, but as it has turned out while coding it
doesn't make a huge difference (beyond being useful to really identify
all places where those two were used because after the move code that
wasn't updated stopped compiling).

But the point is that if I move them under "sync" and update all the
code that uses them, the allocate / free part of the patch grows to
quite a lot of the original patch.

Should I drop the move under "sync"?

>> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
>> index fabcd06..28790bf 100644
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -1434,18 +1437,44 @@ void setup_ept_dump(void)
>>      register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0);
>>  }
>>  
>> -void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>> +int p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>>  {
>>      struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
>>      struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>      struct ept_data *ept;
>> +    int rc;
>> +
>> +    ASSERT(!p2m->sync.logdirty_ranges);
>> +    p2m->sync.logdirty_ranges = rangeset_new(d, "log-dirty",
>> +                                             RANGESETF_prettyprint_hex);
>> +    if ( !p2m->sync.logdirty_ranges )
>> +        return -ENOMEM;
>> +
>> +    rc = rangeset_merge(p2m->sync.logdirty_ranges,
>> +                        hostp2m->sync.logdirty_ranges);
>> +    if ( !rc )
>> +        return rc;
>>  
>>      p2m->ept.ad = hostp2m->ept.ad;
>> +    p2m->max_mapped_pfn = hostp2m->max_mapped_pfn;
>> +    p2m->default_access = hostp2m->default_access;
>> +    p2m->domain = hostp2m->domain;
>> +
>> +    p2m->sync.global_logdirty = hostp2m->sync.global_logdirty;
>>      p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
>>      p2m->max_remapped_gfn = 0;
>>      ept = &p2m->ept;
>>      ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
>>      d->arch.altp2m_eptp[i] = ept->eptp;
>> +
>> +    return 0;
>> +}
>> +
>> +void p2m_uninit_altp2m_ept(struct p2m_domain *p2m)
> 
> For naming consistency, this should be p2m_destroy_altp2m_ept()
> 
> [EDIT] It looks like the rest of the code has poor consistency.  /sigh

I'll do my best to rename touched functions for consistency in the alloc
/ free patch as well then.

>> +{
>> +    ASSERT(p2m->sync.logdirty_ranges);
>> +    rangeset_destroy(p2m->sync.logdirty_ranges);
>> +    p2m->sync.logdirty_ranges = NULL;
> 
> Please make all destroy functions idempotent.  i.e.
> 
> if ( p2m->sync.logdirty_ranges )
> {
>     rangeset_destroy(p2m->sync.logdirty_ranges);
>     p2m->sync.logdirty_ranges = NULL;
> }
> 
> and use this destroy function in the cleanup path of init().

I'll do that.

>>  unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index 42b9ef4..e9f8385 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -119,9 +120,9 @@ static int p2m_init_hostp2m(struct domain *d)
>>  
>>      if ( p2m )
>>      {
>> -        p2m->logdirty_ranges = rangeset_new(d, "log-dirty",
>> -                                            RANGESETF_prettyprint_hex);
>> -        if ( p2m->logdirty_ranges )
>> +        p2m->sync.logdirty_ranges = rangeset_new(d, "log-dirty",
>> +                                                 RANGESETF_prettyprint_hex);
> 
> This looks to be common with bits of p2m_init_altp2m_ept().  Why doesn't
> that get reused?

I'll see about that as well.

>> +        if ( p2m->sync.logdirty_ranges )
>>          {
>>              d->arch.p2m = p2m;
>>              return 0;
>> @@ -2341,6 +2396,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
>>          {
>>              p2m_flush_table(d->arch.altp2m_p2m[idx]);
>>              /* Uninit and reinit ept to force TLB shootdown */
>> +            p2m_uninit_altp2m_ept(d->arch.altp2m_p2m[idx]);
> 
> Shouldn't this ideally be called from
> ept_p2m_uninit(d->arch.altp2m_p2m[idx]) instead?

I'll check and update accordingly.


Thanks,
Razvan

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