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.
