On 23.04.2025 11:07, Roger Pau Monné wrote:
> 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 <jbeul...@suse.com>
>> ---
>> 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.
FTAOD with "this" you mean the patch as is, not the alternative?
>> --- 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.
That would be too broad for my taste. I've used subpage_mmio_find_page()
instead.
Jan
