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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew

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