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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

Jan