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

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

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

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");
                        rc = X86EMUL_RETRY;
                        vio->req.state = STATE_IOREQ_NONE;
                        break;
                    }

                    gprintk(XENLOG_WARNING,
                            "unable to fixup memory %s to %#lx size %u: %d\n",
                            dir ? "read" : "write", addr, size, inner_rc);

Thanks, Roger.