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

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

On Wed, Apr 23, 2025 at 10:43:56AM +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>
> ---
> Alternatively we could drop comment and assertion from
> subpage_mmio_write_emulate().

I think I prefer this as it fits better with my patch to unify the
open-coded MMIO accessors, which does have an ASSERT_UNREACHABLE() for
unhandled sizes.  The return there is anyway too late IMO, as we have
possibly already mapped the page when there's no need for it.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5195,8 +5195,9 @@ 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);

Should we print a debug message here saying the write is possibly
unhandled due to the access size if subpage r/o is enabled?

You might want to re-use the subpage_ro_active() I introduce to limit
the printing of the message.

Thanks, Roger.