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

From: George Dunlap <George.Dunlap@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Kevin Tian <kevin.tian@intel.com>, Wei Liu <wei.liu2@citrix.com>,
	Razvan Cojocaru <rcojocaru@bitdefender.com>,
	Andrew Cooper <Andrew.Cooper3@citrix.com>,
	George Dunlap <George.Dunlap@citrix.com>,
	Jun Nakajima <jun.nakajima@intel.com>,
	xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH V7 3/5] x86/altp2m: fix display frozen when switching to a new view early
Date: Tue, 20 Nov 2018 09:43:31 +0000	[thread overview]
Message-ID: <36228A8C-086E-4A48-AF13-9E9EFE23555C@citrix.com> (raw)
In-Reply-To: <5BF3CFF802000078001FDD90@prv1-mh.provo.novell.com>

> On Nov 20, 2018, at 9:12 AM, Jan Beulich <JBeulich@suse.com> wrote:
> 
>>>> On 19.11.18 at 18:26, <rcojocaru@bitdefender.com> 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;
>> * modifies p2m_change_entry_type_global(),
>>  p2m_memory_type_changed(), p2m_change_type_range() and
>>  p2m_finish_type_change() to propagate their changes to all
>>  valid altp2ms.
>> 
>> With the introduction of altp2m fields in p2m_memory_type_changed()
>> the whole function has been put under CONFIG_HVM.
>> 
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> Suggested-by: George Dunlap <george.dunlap@citrix.com>
> 
> Judging from George's earlier analysis I wonder whether the patch
> ordering is correct: I've got the impression that the patch here should
> be last in the series, for it to be correct and efficient in all cases.

My patches back would require significant rework — both of my patches to rebase on an earlier tree, and of Razvan’s patches to be rebased later.  I don’t think this kind of thing should be required unless there is a compelling benefit to doing so.

Normally the reason for such an ordering is “no regressions in the middle of the series”, primarily in order to avoid breaking bisection; and of course there’s also something  much more aesthetically satisfying about doing a bunch of prep work behind the scenes and then flipping one switch to enable it at the end of the series.

In this case, altp2m + logdirty was already broken; so I didn’t think this patch could be considered to introduce a regression.  Thus the only reason to have this patch be the final patch would be for aesthetic purposes, which I didn’t consider enough value to justify requesting a patch re-ordering.

Did you have a compelling reason in mind for doing the reordering?

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