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

To deal with the MMIO case the if() condition needs extending
(or a separate code block needs adding) and then I would still
see this assignment become

       npfec->present = mfn_valid(mfn) || rc == NESTEDHVM_PAGEFAULT_DIRECT_MMIO;

After all we never write non-present direct MMIO entries. (The
above is ignoring the question of whether 1 -> 0 transitions of
->present are to be permitted, as per the other sub-thread.)
This is to cope with (current or potential future) p2m entries
that get a special MFN stored, e.g. along the lines of
SHARED_P2M_ENTRY. I don't think we have any such right now, but
I also don't see why code shouldn't be prepared for ones to
appear.

Jan

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