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@suse.com>
---
Alternatively we could drop comment and assertion from
subpage_mmio_write_emulate().

--- 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);
 
     return X86EMUL_OKAY;
 }

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@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.

> --- 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.

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 <jbeulich@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

On Wed, Apr 23, 2025 at 02:58:28PM +0200, Jan Beulich wrote:
> 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 <jbeulich@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?

Yes, sorry, "this" => "this patch".

> >> --- 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()

Hm, yes, that's likely more expensive, but certainly more accurate.
Given the context here the extra logic doesn't matter much.

Thanks, Roger.