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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>> Tangentially - is it plausible for has_arch_io_resources() to return
>> false when has_arch_pdevs() returns true? Perhaps there are exotic
>> PCI devices (but non-bridges) which work with no BARs at all ...
> 
> I guess it's technically possible, albeit very unlikely?  How would
> the OS interact with such device then, exclusively with PCI config
> space accesses?

Yes, that's what I'd expect for such devices. Looking around, there
are numerous such devices (leaving aside bridges). Just that it looks
implausible to me that one would want to pass those through to a guest.

Jan