Re: [PATCH 7/9] xen/x86: rename cache_flush_permitted() to has_arch_io_resources()
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.
Re: [PATCH 7/9] xen/x86: rename cache_flush_permitted() to has_arch_io_resources()
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? Jan
Re: [PATCH 7/9] xen/x86: rename cache_flush_permitted() to has_arch_io_resources()
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. Thanks, Roger.
Re: [PATCH 7/9] xen/x86: rename cache_flush_permitted() to has_arch_io_resources()
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
Re: [PATCH 7/9] xen/x86: rename cache_flush_permitted() to has_arch_io_resources()
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. > >> 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? Thanks, Roger.
Re: [PATCH 7/9] xen/x86: rename cache_flush_permitted() to has_arch_io_resources()
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
Re: [PATCH 7/9] xen/x86: rename cache_flush_permitted() to has_arch_io_resources()
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. > 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? I'm happy to just use cache_flush_permitted() which is likely more correct given the context here. Thanks, Roger.
Re: [PATCH 7/9] xen/x86: rename cache_flush_permitted() to has_arch_io_resources()
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. 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 ... Jan
