Re: [PATCH 2/2] x86/dom0: attempt to fixup p2m page-faults for PVH dom0

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.02.2025 11:20, Roger Pau Monné wrote:
> On Mon, Feb 17, 2025 at 09:44:28AM +0100, Jan Beulich wrote:
>> On 17.02.2025 09:25, Roger Pau Monné wrote:
>>> On Fri, Feb 14, 2025 at 02:07:05PM +0100, Jan Beulich wrote:
>>>> On 14.02.2025 13:38, Roger Pau Monné wrote:
>>>>> On Fri, Feb 14, 2025 at 12:53:01PM +0100, Jan Beulich wrote:
>>>>>> On 14.02.2025 10:29, Roger Pau Monne wrote:
>>>>>>> +{
>>>>>>> +    unsigned long gfn = paddr_to_pfn(addr);
>>>>>>> +    struct domain *currd = current->domain;
>>>>>>> +    p2m_type_t type;
>>>>>>> +    mfn_t mfn;
>>>>>>> +    int rc;
>>>>>>> +
>>>>>>> +    ASSERT(is_hardware_domain(currd));
>>>>>>> +    ASSERT(!altp2m_active(currd));
>>>>>>> +
>>>>>>> +    /*
>>>>>>> +     * Fixups are only applied for MMIO holes, and rely on the 
>>>>>>> hardware domain
>>>>>>> +     * having identity mappings for non RAM regions (gfn == mfn).
>>>>>>> +     */
>>>>>>> +    if ( !iomem_access_permitted(currd, gfn, gfn) ||
>>>>>>> +         !is_memory_hole(_mfn(gfn), _mfn(gfn)) )
>>>>>>> +        return -EPERM;
>>>>>>> +
>>>>>>> +    mfn = get_gfn(currd, gfn, &type);
>>>>>>> +    if ( !mfn_eq(mfn, INVALID_MFN) || !p2m_is_hole(type) )
>>>>>>> +        rc = mfn_eq(mfn, _mfn(gfn)) ? 0 : -EEXIST;
>>>>>>
>>>>>> I understand this is to cover the case where two vCPU-s access the same 
>>>>>> GFN
>>>>>> at about the same time. However, the "success" log message at the call 
>>>>>> site
>>>>>> being debug-only means we may be silently hiding bugs in release builds, 
>>>>>> if
>>>>>> e.g. we get here despite the GFN having had an identity mapping already 
>>>>>> for
>>>>>> ages.
>>>>>
>>>>> Possibly, but what would be your suggestion to fix this?  I will think
>>>>> about it, but I can't immediately see a solution that's not simply to
>>>>> make the message printed by the caller to be gprintk() instead of
>>>>> gdprintk() so catch such bugs.  Would you agree to that?
>>>>
>>>> My thinking was that it might be best to propagate a distinguishable error
>>>> code (perhaps -EEXIST, with its present use then replaced) out of the 
>>>> function,
>>>> and make the choice of gprintk() vs gdprintk() depend on that. Accompanied 
>>>> by a
>>>> comment explaining things a little.
>>>
>>> I think it would be easier if I just made those gprintk() instead of
>>> gdprintk(), all with severity XENLOG_DEBUG except for the one that
>>> reports the failure of the fixup function that is XENLOG_WARNING.
>>> Would you be OK with that?
>>
>> Hmm. Okay-ish at best. Even if debug+guest-level messages are suppressed by
>> default, I think it wouldn't be nice if many of them might appear in release
>> builds with guest_loglevel=all. What I find difficult is to predict how high
>> the chances are to see any of them (and then possibly multiple times).
> 
> I think getting those messages even in non-debug builds might be
> helpful for debugging purposes.  Sometimes it's difficult for users to
> switch to a debug build of Xen if not provided by their upstream.
> 
> FWIW, on my Intel NUC I see three of those:
> 
> (XEN) [    5.418855] arch/x86/hvm/emulate.c:391:d0v0 fixup p2m mapping for 
> page fedc7 added
> (XEN) [    5.474574] arch/x86/hvm/emulate.c:391:d0v0 fixup p2m mapping for 
> page fd6a0 added
> (XEN) [    8.712784] arch/x86/hvm/emulate.c:391:d0v2 fixup p2m mapping for 
> page fe410 added

For my understanding: Did Xen with a PVH Dom0 not work on the NUC before? Or
else how come it survived without this fixing up of mappings?

> Would you be fine with this approach:
> 
> bool __ro_after_init opt_dom0_pf_fixup;
> static int hwdom_fixup_p2m(paddr_t addr)
> {
>     unsigned long gfn = paddr_to_pfn(addr);
>     struct domain *currd = current->domain;
>     p2m_type_t type;
>     mfn_t mfn;
>     int rc;
> 
>     ASSERT(is_hardware_domain(currd));
>     ASSERT(!altp2m_active(currd));
> 
>     /*
>      * Fixups are only applied for MMIO holes, and rely on the hardware domain
>      * having identity mappings for non RAM regions (gfn == mfn).
>      */
>     if ( !iomem_access_permitted(currd, gfn, gfn) ||
>          !is_memory_hole(_mfn(gfn), _mfn(gfn)) )
>         return -EPERM;
> 
>     mfn = get_gfn(currd, gfn, &type);
>     if ( !mfn_eq(mfn, INVALID_MFN) || !p2m_is_hole(type) )
>         rc = mfn_eq(mfn, _mfn(gfn)) ? -EEXIST : -ENOTEMPTY;
>     else
>         rc = set_mmio_p2m_entry(currd, _gfn(gfn), _mfn(gfn), 0);
>     put_gfn(currd, gfn);
> 
>     return rc;
> }
> [...]
>                     int inner_rc = hwdom_fixup_p2m(addr);
> 
>                     if ( !inner_rc || inner_rc == -EEXIST )
>                     {
>                         gdprintk(XENLOG_DEBUG,
>                                  "fixup p2m mapping for page %lx %s\n",
>                                  paddr_to_pfn(addr),
>                                  !inner_rc ? "added" : "already present");

As before, I think the "already present" message wants to be present also in
release build logs. As opposed to the "added" one. Yet at the same time, if
e.g. you and Andrew agree on the shape above, I won't stand in the way.

Jan