Re: [Xen-devel] [PATCH 1/5] x86/traps: Drop paging_mode_external() handling from the PV pagefault path

[Tim Deegan <tim@xxxxxxx>]

At 13:05 +0000 on 12 Dec (1481547944), Andrew Cooper wrote:
> On 12/12/16 11:57, Tim Deegan wrote:
> > At 11:45 +0000 on 12 Dec (1481543145), Andrew Cooper wrote:
> >> On 12/12/16 11:27, Tim Deegan wrote:
> >>> At 10:43 +0000 on 12 Dec (1481539421), Andrew Cooper wrote:
> >>>> PV guests necessarily can't be external, as Xen must steal address space 
> >>>> from
> >>>> them.  Pagefaults for HVM guests are handled by 
> >>>> {vmx,svm}_vmexit_handler() and
> >>>> don't enter the PV fixup_page_fault() path.
> >>>>
> >>>> This paging_fault() callsite is therefore dead code, so drop it.
> >>>>
> >>>> Clarify the comment at the other paging_fault() callsite to indicate 
> >>>> that it
> >>>> covers the logdirty case only.
> >>>>
> >>>> No functional change.
> >>> IMO this is a change, just not on any supported config.
> >> Really?  I'd agree if the content of the clause was reachable (and we
> >> didn't support the configuration required to make it reachable), but my
> >> argument here is that it is genuinely dead code and cannot be reached.
> > Ah, you're right - I was thinking of refcounts mode.  External mode
> > guests can't get here.  In which case...
> >
> >>>> +    /*
> >>>> +     * For non-external shadowed guests (i.e. PV guests with logdirty
> >>>> +     * active), we fix up both their own pagefaults and Xen's, since
> >>>> +     * they share the pagetables.
> >>>> +     */
> >>>>      if ( paging_mode_enabled(d) && !paging_mode_external(d) )
> >>> Here we can drop the check of !paging_mode_external(d), or maybe turn
> >>> it into an assertion somewhere.
> >>>
> >>> With that,
> >>>
> >>> Acked-by: Tim Deegan <tim@xxxxxxx>
> >> Seeing as both you and Jan have asked, I will see about addition an
> >> assertion.  However, it will have to be in patch 3 for bisectability,
> >> when we rule out the use of PV refcount guests.  Are you ok with that?
> > ...I don't follow you here -- since external-mode guests can't reach this
> > code we can surely switch from test to assert.
> 
> My plan was to add something like this:
> 
> if ( paging_mode_enabled(d) )
>     ASSERT(paging_mode_only_logdirty(d));
> 
> for a suitable new predicate.

That sounds fine.  Order it whatever way is most convenient, so long
as this !paging_mode_external(d) disappears at some point. :)

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel