Re: [PATCH 7/9] xen/x86: rename cache_flush_permitted() to has_arch_io_resources()

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

On Fri, May 16, 2025 at 10:36:19AM +0200, Jan Beulich wrote:
> On 16.05.2025 10:27, Roger Pau Monné wrote:
> > On Fri, May 16, 2025 at 10:08:35AM +0200, Jan Beulich wrote:
> >> On 16.05.2025 10:02, Roger Pau Monné wrote:
> >>> On Fri, May 16, 2025 at 09:07:43AM +0200, Jan Beulich wrote:
> >>>> On 15.05.2025 12:28, Roger Pau Monné wrote:
> >>>>> On Mon, May 12, 2025 at 05:16:02PM +0200, Jan Beulich wrote:
> >>>>>> On 06.05.2025 10:31, Roger Pau Monne wrote:
> >>>>>>> To better describe the underlying implementation.  Define
> >>>>>>> cache_flush_permitted() as an alias of has_arch_io_resources(), so 
> >>>>>>> that
> >>>>>>> current users of cache_flush_permitted() are not effectively modified.
> >>>>>>>
> >>>>>>> With the introduction of the new handler, change some of the call 
> >>>>>>> sites of
> >>>>>>> cache_flush_permitted() to instead use has_arch_io_resources() as such
> >>>>>>> callers are not after whether cache flush is enabled, but rather 
> >>>>>>> whether
> >>>>>>> the domain has any IO resources assigned.
> >>>>>>>
> >>>>>>> Take the opportunity to adjust l1_disallow_mask() to use the newly
> >>>>>>> introduced has_arch_io_resources() macro.
> >>>>>>
> >>>>>> While I'm happy with everything else here, to me it's at least on the
> >>>>>> edge whether cache_flush_permitted() wouldn't be the better predicate
> >>>>>> to use there, for this being about ...
> >>>>>>
> >>>>>>> --- a/xen/arch/x86/mm.c
> >>>>>>> +++ b/xen/arch/x86/mm.c
> >>>>>>> @@ -172,8 +172,7 @@ static DEFINE_SPINLOCK(subpage_ro_lock);
> >>>>>>>  
> >>>>>>>  #define l1_disallow_mask(d)                                     \
> >>>>>>>      (((d) != dom_io) &&                                         \
> >>>>>>> -     (rangeset_is_empty((d)->iomem_caps) &&                     \
> >>>>>>> -      rangeset_is_empty((d)->arch.ioport_caps) &&               \
> >>>>>>> +     (!has_arch_io_resources(d) &&                              \
> >>>>>>>        !has_arch_pdevs(d) &&                                     \
> >>>>>>>        is_pv_domain(d)) ?                                        \
> >>>>>>>       L1_DISALLOW_MASK : (L1_DISALLOW_MASK & ~PAGE_CACHE_ATTRS))
> >>>>>>
> >>>>>> ... cachability, which goes hand in hand with the ability to also
> >>>>>> flush cache contents.
> >>>>>
> >>>>> Hm, I was on the edge here, in fact I've previously coded this using
> >>>>> cache_flush_permitted(), just to the change back to
> >>>>> has_arch_io_resources().  If you think cache_flush_permitted() is
> >>>>> better I'm fine with that.
> >>>>
> >>>> I think that would be better here, yet as you say - it's not entirely
> >>>> clear cut either way.
> >>>
> >>> I've reverted this chunk of the change and left the code as-is for the
> >>> time being.
> >>
> >> Didn't we agree to use cache_flush_permitted() here instead?
> > 
> > I think it would be a bit weird, if we want this to be a
> > non-functional change we would need to keep the has_arch_pdevs()
> > condition because cache_flush_permitted() doesn't take that into
> > account.  Or we need to adjust cache_flush_permitted() to also take
> > has_arch_pdevs() into consideration.
> 
> Which is what you suggested elsewhere, or did I misunderstand that?

Yes, I missed that you agreed to that then, sorry.  To many messages
on the thread I'm afraid.

Thanks, Roger.