Re: [Xen-devel] [PATCH] x86/nested-hap: Fix handling of L0_ERROR

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/11/2019 15:23, Jan Beulich wrote:
> On 19.11.2019 15:58, Andrew Cooper wrote:
>> On 19/11/2019 11:13, Jan Beulich wrote:
>>> On 18.11.2019 19:15, Andrew Cooper wrote:
>>> I take it you imply that L0_ERROR would need raising (as per the
>>> auxiliary code fragment adding the "(access_x && *page_order)"
>>> check), but I wonder whether that would really be correct. This
>>> depends on what L0_ERROR really is supposed to mean: An error
>>> because of actual L0 settings (x=0 in our case), or an error
>>> because of intended L0 settings (x=1 in our case). After all a
>>> violation of just the p2m_access (which also affects r/w/x)
>>> doesn't get reported by nestedhap_walk_L0_p2m() as L0_ERROR
>>> either (and hence would, as it seems to me, lead to a similar
>>> live lock).
>>>
>>> Therefore I wonder whether your initial idea of doing the
>>> shattering right here wouldn't be the better course of action.
>>> nestedhap_fix_p2m() could either install the large page and then
>>> shatter it right away, or it could install just the individual
>>> small page. Together with the different npfec adjustment model
>>> suggested below (leading to npfec.present to also get updated in
>>> the DONE case) a similar "insn-fetch && present" conditional (to
>>> that introduced for XSA-304) could then be used there.
>>>
>>> Even better - by making the violation checking around the
>>> original XSA-304 addition a function (together with the 304
>>> addition), such a function might then be reusable here. This
>>> might then address the p2m_access related live lock as well.
>> This is all unrelated to the patch.
> I don't think so.

This patch is not a fix for the XSA-304 livelock.

It is a independent bug discovered while investigating the livelock.

It may, or may not, form part of the XSA-304 livelock bugfix, depending
on how the rest of the investigation goes.

>  At the very least defining what exactly L0_ERROR
> is intended to mean is pretty relevant here.

The intent of the code is clear (at least, to me).

It means #NPF/EPT_VIOLATION/EPT_MISCONFIG in the L01 part of the nested
walk.

>>>> @@ -181,6 +180,18 @@ nestedhap_walk_L0_p2m(struct p2m_domain *p2m, paddr_t 
>>>> L1_gpa, paddr_t *L0_gpa,
>>>>      *L0_gpa = (mfn_x(mfn) << PAGE_SHIFT) + (L1_gpa & ~PAGE_MASK);
>>>>  out:
>>>>      __put_gfn(p2m, L1_gpa >> PAGE_SHIFT);
>>>> +
>>>> +    /*
>>>> +     * When reporting L0_ERROR, rewrite nfpec to match what would have 
>>>> occured
>>>> +     * if hardware had walked the L0, rather than the combined L02.
>>>> +     */
>>>> +    if ( rc == NESTEDHVM_PAGEFAULT_L0_ERROR )
>>>> +    {
>>>> +        npfec->present = !mfn_eq(mfn, INVALID_MFN);
>>> To be in line with the conditional a few lines up from here,
>>> wouldn't this better be !mfn_valid(mfn)?
>> That's not how the return value from get_gfn_*() works, and would break
>> the MMIO case.
> How that (for the latter part of your reply)? The MMIO case produces
> NESTEDHVM_PAGEFAULT_DIRECT_MMIO, i.e. doesn't even enter this if().
> Hence my remark elsewhere that the MMIO cases isn't taken care of in
> the first place.
>
>>> Should there ever be a case to clear the flag when it was set? If
>>> a mapping has gone away between the time the exit condition was
>>> detected and the time we re-evaluate things here, I think it
>>> should still report "present" back to the caller.
>> No - absolutely not.  We must report the property of the L0 walk, as we
>> found it.
>>
>> Pretending it was present when it wasn't is a sure-fire way of leaving
>> further bugs lurking.
> But if npfec.present is set, it surely was set at the time of the
> hardware walk. And _that's_ what npfec is supposed to represent.
>
>>>  Taking both
>>> remarks together I'm thinking of
>>>
>>>         if ( mfn_valid(mfn) )
>>>             npfec->present = 1;
>>>
>>>> +        npfec->gla_valid = 0;
>>> For this, one the question is whose linear address is meant here.
>> The linear address (which was L2's) is nonsensical when we've taken an
>> L0 fault.  This is why it is clobbered unconditionally.
> And this is also why I was saying ...
>
>>> If it's L2's, then it shouldn't be cleared. If it's L1's, then
>>> it would seem to me that it should have been avoided to set the
>>> field, or at least it should have been cleared the moment we're
>>> past L12 handling.
> ... this. If it's nonsensical, it shouldn't have been set to begin
> with, or be squashed earlier than here.

There seems to be a lot of confusion here.

This is the correct place to discard it.

Hardware did a real walk of L02 and got a real gpa and npfec (optionally
with a real gla), that overall identified "something went wrong".

Upon interpreting "what went wrong", Xen may decide that it is a problem
in the L01 walk, rather than the L12 or combined L02.

A problem in the L01 walk is handled by returning L0_ERROR back to the
common code, discarding the current NPF/EPT_VIOLATION/MISCONFIG context,
and synthesizing the state that would have occurred if hardware were to
have performed the L01 walk instead of L02, so it can be correctly
interpreted by the common code on the hostp2m.

gpa gets adjusted.  npfec doesn't (and the subject of this patch).  gla
doesn't even get passed in for potential adjustments.

The gla isn't actually an interesting value, and Xen's use of it for
various cache maintenance purposes looks buggy.  Gla is specific to the
L2 guests' register state and virtual memory layout, and in particular,
has no bearing on anything where we've decided that we need a correction
to the L01 mapping.

~Andrew

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