Re: [PATCH v2] x86: constrain sub-page access length in mmio_ro_emulated_write()

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

On Fri, Apr 25, 2025 at 04:57:18PM +0200, Jan Beulich wrote:
> Without doing so we could trigger the ASSERT_UNREACHABLE() in
> subpage_mmio_write_emulate(). A comment there actually says this
> validation would already have been done ...
> 
> Fixes: 8847d6e23f97 ("x86/mm: add API for marking only part of a MMIO page 
> read only")
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> ---
> v2: Emit a warning message otherwise.
> 
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5195,8 +5195,13 @@ int cf_check mmio_ro_emulated_write(
>          return X86EMUL_UNHANDLEABLE;
>      }
>  
> -    subpage_mmio_write_emulate(mmio_ro_ctxt->mfn, PAGE_OFFSET(offset),
> -                               p_data, bytes);
> +    if ( bytes <= 8 )
> +        subpage_mmio_write_emulate(mmio_ro_ctxt->mfn, PAGE_OFFSET(offset),
> +                                   p_data, bytes);
> +    else if ( subpage_mmio_find_page(mmio_ro_ctxt->mfn) )
> +        gprintk(XENLOG_WARNING,
> +                "unsupported %u-byte write to R/O MMIO 0x%"PRI_mfn"%03lx\n",
> +                bytes, mfn_x(mmio_ro_ctxt->mfn), PAGE_OFFSET(offset));

It feels more natural to me to use:

"unsupported %u-byte write to r/o MMIO %#lx\n",
bytes, mfn_to_maddr(mmio_ro_ctxt->mfn) + PAGE_OFFSET(offset)

As it simplifies the printf format string, but that's just my taste.

Thanks, Roger.