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

[Jan Beulich <jbeulich@xxxxxxxx>]

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. At the very least defining what exactly L0_ERROR
is intended to mean is pretty relevant here. The other two
paragraphs may or may not be considered related, depending how
things overall are supposed to work.

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

>> And then there is the question of overall flow here. On the basis
>> of npfec not being of any interest anymore to the caller's caller
>> if reporting back DONE (but as per far above it might help our
>> immediate caller) I wonder whether
> 
> That is far too subtle and complicated.  I'm not included to make the
> code any harder to follow than it already is.

I have to admit I don't see how this is any more complicated than
your variant. I'm merely suggesting adjustments to npfec to get done
at slightly different points in time, with the possible benefit of
an easier route to addressing the live lock(s). It possibly being
more subtle can be compensated by adequate commenting.

Jan

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