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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.11.2019 21:45, Andrew Cooper wrote:
> 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.

But this is contrary to the code deriving L0_ERROR exclusively from
the p2m type, ignoring p2m access and anything else that may have lead
to e.g. r/w/x settings different from the ones implied by the type.
Using just the p2m type to me makes it more like a "would be error"
than an actual walk result.

What you imply would require an approach much more similar to that of
nestedhap_walk_L1_p2m(), looking at the actual EPT/NPT entries rather
than the abstract (MFN,order,type,access) tuple derived from them.

>>>>> @@ -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.

Indeed. The question though is who it is that is confused. It's
likely me, but I dislike you putting it as if there's not even
the remote chance of it not being (just) me.

> 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.

Well, we appear to agree on what is to happen, but disagree on what
"if hardware were to have performed the L01 walk instead of L02" means:
You suggest this ought to be the state of the tables as we see them
when we do the software walk. This allows for a 1 -> 0 transition of
npfec.present when updating it. Otoh I think that a 1 -> 0 transition
shouldn't be possible, as the hardware having found the L02 entry to
be present implies that both the L12 and the L01 entries also had
their respective entries present at the time the hardware walk was
performed, and npfec (including any updates we do to it) should
reflect (as much as possible) the state of things at the time of the
hardware walk, because that's what it is describing.

Note that what I describe above as my interpretation of your replies
seems (to me) contrary to your patch, in a comment, having "if
hardware had walked the L0". But yes, the comment isn't entirely
unambiguous either, as it also leaves open at which point in time
you mean this hypothetical walk to have occurred.

> 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.

And hence my conclusion that gla_valid should be cleared the latest
by nestedhvm_hap_nested_page_fault() before even calling
nestedhap_walk_L0_p2m(), i.e. in particular independent of anything
the latter function does.

In the end it's not my ack that would matter for this patch, so if
you and George can come to an agreement, I'll go silent. For me to
give an R-b the minimum requirements are
- more extensive description (or comments) eliminating some of the
  ambiguities (and thus having firm common grounds as to what the
  behavior ultimately ought to be, allowing to judge that this
  change is, in all its parts, at least not a step away from the
  targeted behavior),
- the MMIO case taken care of (or making it explicit that the case
  is left unaddressed),
- at least a comment regarding the XSA-304 / mem-access related
  shortcomings of nestedhap_walk_L0_p2m().

Jan

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