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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.02.2025 11:51, Roger Pau Monné wrote:
> On Mon, Feb 17, 2025 at 11:27:52AM +0100, Jan Beulich wrote:
>> 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?
> 
> I've got no idea what's in those addresses.  It did survive and seems
> to work fine without those identity mappings in the p2m.  I assume
> that returning ~0 for reads and ignoring writes what good enough.

On one hand I find this concerning. Otoh this way we'll maybe learn what
issues were so far papered over by said read/write behavior. (Of course
there's also a small chance of this opening up new problem areas.)

>>> 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.
> 
> I didn't want to add yet another level of indentation, as it then
> becomes:
> 
>                     int inner_rc = hwdom_fixup_p2m(addr);
> 
>                     if ( !inner_rc || inner_rc == -EEXIST )
>                     {
>                         if ( !inner_rc )
>                             gdprintk(XENLOG_DEBUG,
>                                      "fixup p2m mapping for page %lx added\n",
>                                      paddr_to_pfn(addr));
>                         else
>                             gprintk(XENLOG_INFO,
>                                      "fixup p2m mapping for page %lx already 
> present\n",
>                                      paddr_to_pfn(addr));
> 
> Would you be OK with the above proposal then?

Yes (with off-by-1 indentation corrected). It's unfortunate that this can't
be written in a more compact form.

Jan