Re: [PATCH 3/5] x86/hvm: fix handling of accesses to partial r/o MMIO pages

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.04.2025 12:54, Roger Pau Monne wrote:
> The current logic to handle accesses to MMIO pages partially read-only is
> based on the (now removed) logic used to handle accesses to the r/o MMCFG
> region(s) for PVH v1 dom0.  However that has issues when running on AMD
> hardware, as in that case the guest linear address that triggered the fault
> is not provided as part of the VM exit.  This caused
> mmio_ro_emulated_write() to always fail before calling
> subpage_mmio_write_emulate() when running on AMD and called from an HVM
> context.
> 
> Take a different approach and convert the handling of partial read-only
> MMIO page accesses into an HVM MMIO ops handler, as that's the more natural
> way to handle this kind of emulation for HVM domains.
> 
> This allows getting rid of hvm_emulate_one_mmio() and it's single cal site
> in hvm_hap_nested_page_fault().
> 
> Note a small adjustment is needed to the `pf-fixup` dom0 PVH logic: avoid
> attempting to fixup faults resulting from accesses to read-only MMIO
> regions, as handling of those accesses is now done by handle_mmio().
> 
> Fixes: 33c19df9a5a0 ('x86/PCI: intercept accesses to RO MMIO from dom0s in 
> HVM containers')
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> The fixes tag is maybe a bit wonky, it's either this or:
> 
> 8847d6e23f97 ('x86/mm: add API for marking only part of a MMIO page read 
> only')
> 
> However the addition of subpage r/o access handling to the existing
> mmio_ro_emulated_write() function was done based on the assumption that the
> current code was working - which turned out to not be the case for AMD,
> hence my preference for blaming the commit that actually introduced the
> broken logic.

I agree.

> ---
>  xen/arch/x86/hvm/emulate.c             | 47 +-------------
>  xen/arch/x86/hvm/hvm.c                 | 89 +++++++++++++++++++++++---

It's quite a bit of rather special code you add to this catch-all file. How
about introducing a small new one, e.g. mmio.c (to later maybe become home
to some further stuff)?

> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -370,7 +370,8 @@ static int hvmemul_do_io(
>          /* If there is no suitable backing DM, just ignore accesses */
>          if ( !s )
>          {
> -            if ( is_mmio && is_hardware_domain(currd) )
> +            if ( is_mmio && is_hardware_domain(currd) &&
> +                 !rangeset_contains_singleton(mmio_ro_ranges, 
> PFN_DOWN(addr)) )

I think this needs a brief comment - it otherwise looks misplaced here.

> @@ -585,9 +585,81 @@ static int cf_check hvm_print_line(
>      return X86EMUL_OKAY;
>  }
>  
> +static int cf_check subpage_mmio_accept(struct vcpu *v, unsigned long addr)
> +{
> +    p2m_type_t t;
> +    mfn_t mfn = get_gfn_query_unlocked(v->domain, addr, &t);
> +
> +    return !mfn_eq(mfn, INVALID_MFN) && t == p2m_mmio_direct &&
> +           !!subpage_mmio_find_page(mfn);

The !! isn't needed here, is it?

> +}
> +
> +static int cf_check subpage_mmio_read(
> +    struct vcpu *v, unsigned long addr, unsigned int len, unsigned long 
> *data)
> +{
> +    struct domain *d = v->domain;
> +    p2m_type_t t;
> +    mfn_t mfn = get_gfn_query(d, addr, &t);
> +    struct subpage_ro_range *entry;
> +    volatile void __iomem *mem;
> +
> +    *data = ~0UL;
> +
> +    if ( mfn_eq(mfn, INVALID_MFN) || t != p2m_mmio_direct )
> +    {
> +        put_gfn(d, addr);
> +        return X86EMUL_RETRY;
> +    }
> +
> +    entry = subpage_mmio_find_page(mfn);
> +    if ( !entry )
> +    {
> +        put_gfn(d, addr);
> +        return X86EMUL_RETRY;
> +    }
> +
> +    mem = subpage_mmio_map_page(entry);
> +    if ( !mem )
> +    {
> +        put_gfn(d, addr);
> +        gprintk(XENLOG_ERR, "Failed to map page for MMIO read at %#lx\n",
> +                mfn_to_maddr(mfn) + PAGE_OFFSET(addr));

Makes me wonder whether the function parameter wouldn't better be named
"curr" and the local variable "currd", to reflect that this log message
will report appropriate context.

Would also logging the guest physical address perhaps be worthwhile here?

> +        return X86EMUL_OKAY;
> +    }
> +
> +    *data = read_mmio(mem + PAGE_OFFSET(addr), len);
> +
> +    put_gfn(d, addr);
> +    return X86EMUL_OKAY;
> +}
> +
> +static int cf_check subpage_mmio_write(
> +    struct vcpu *v, unsigned long addr, unsigned int len, unsigned long data)
> +{
> +    struct domain *d = v->domain;
> +    p2m_type_t t;
> +    mfn_t mfn = get_gfn_query(d, addr, &t);
> +
> +    if ( mfn_eq(mfn, INVALID_MFN) || t != p2m_mmio_direct )
> +    {
> +        put_gfn(d, addr);
> +        return X86EMUL_RETRY;
> +    }
> +
> +    subpage_mmio_write_emulate(mfn, PAGE_OFFSET(addr), data, len);
> +
> +    put_gfn(d, addr);
> +    return X86EMUL_OKAY;
> +}

Unlike the read path this doesn't return RETRY when subpage_mmio_find_page()
fails (indicating something must have changed after "accept" said yes).

> @@ -1981,7 +2056,9 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>       */
>      if ( (p2mt == p2m_mmio_dm) ||
>           (npfec.write_access &&
> -          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
> +          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server) ||
> +           /* MMIO entries can be r/o if the target mfn is in 
> mmio_ro_ranges. */
> +           (p2mt == p2m_mmio_direct))) )
>      {
>          if ( !handle_mmio_with_translation(gla, gfn, npfec) )
>              hvm_inject_hw_exception(X86_EXC_GP, 0);

Aren't we handing too many things to handle_mmio_with_translation() this
way? At the very least you're losing ...

> @@ -2033,14 +2110,6 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>          goto out_put_gfn;
>      }
>  
> -    if ( (p2mt == p2m_mmio_direct) && npfec.write_access && npfec.present &&

... the .present check.

I'm also concerned of e.g. VT-x'es APIC access MFN, which is
p2m_mmio_direct.

Jan