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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

>>>> 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.
> 
> Well, we also need to consider dom0 here (either PV or PVH), which
> will get those devices passed through.  I assume those are mostly
> system devices, and hence there's usually no interaction of the OS
> with them.
> 
> I'm thinking that our definition of cache_flush_permitted() is not
> fully accurate then, we would need to also account for any PCI devices
> being assigned to the guest, even if those have no IO resources?

I think so, yes.

Jan