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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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?

Thanks, Roger.