[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 05/21] IOMMU/x86: restrict IO-APIC mappings for PV Dom0
On 04.05.2022 15:46, Roger Pau Monné wrote: > On Wed, May 04, 2022 at 03:19:16PM +0200, Jan Beulich wrote: >> On 04.05.2022 15:00, Roger Pau Monné wrote: >>> On Wed, May 04, 2022 at 02:12:58PM +0200, Jan Beulich wrote: >>>> On 04.05.2022 14:01, Roger Pau Monné wrote: >>>>> On Wed, May 04, 2022 at 12:51:25PM +0200, Jan Beulich wrote: >>>>>> On 04.05.2022 12:30, Roger Pau Monné wrote: >>>>>>> Right, ->iomem_caps is indeed too wide for our purpose. What >>>>>>> about using something like: >>>>>>> >>>>>>> else if ( is_pv_domain(d) ) >>>>>>> { >>>>>>> if ( !iomem_access_permitted(d, pfn, pfn) ) >>>>>>> return 0; >>>>>> >>>>>> We can't return 0 here (as RAM pages also make it here when >>>>>> !iommu_hwdom_strict), so I can at best take this as a vague outline >>>>>> of what you really mean. And I don't want to rely on RAM pages being >>>>>> (imo wrongly) represented by set bits in Dom0's iomem_caps. >>>>> >>>>> Well, yes, my suggestion was taking into account that ->iomem_caps for >>>>> dom0 has mostly holes for things that shouldn't be mapped, but >>>>> otherwise contains everything else as allowed (including RAM). >>>>> >>>>> We could instead do: >>>>> >>>>> else if ( is_pv_domain(d) && type != RAM_TYPE_CONVENTIONAL ) >>>>> { >>>>> ... >>>>> >>>>> So that we don't rely on RAM being 'allowed' in ->iomem_caps? >>>> >>>> This would feel to me like excess special casing. >>> >>> What about placing this in the 'default:' label on the type switch a >>> bit above? >> >> I'd really like to stick to the present layout of where the special >> casing is done, with PV and PVH logic at least next to each other. I >> continue to think the construct I suggested (still visible below) >> would do. >> >>>>>>> if ( rangeset_contains_singleton(mmio_ro_ranges, pfn) ) >>>>>>> return IOMMUF_readable; >>>>>>> } >>>>>>> >>>>>>> That would get us a bit closer to allowed CPU side mappings, and we >>>>>>> don't need to special case IO-APIC or HPET addresses as those are >>>>>>> already added to ->iomem_caps or mmio_ro_ranges respectively by >>>>>>> dom0_setup_permissions(). >>>>>> >>>>>> This won't fit in a region of code framed by a (split) comment >>>>>> saying "Check that it doesn't overlap with ...". Hence if anything >>>>>> I could put something like this further down. Yet even then the >>>>>> question remains what to do with ranges which pass >>>>>> iomem_access_permitted() but >>>>>> - aren't really MMIO, >>>>>> - are inside MMCFG, >>>>>> - are otherwise special. >>>>>> >>>>>> Or did you perhaps mean to suggest something like >>>>>> >>>>>> else if ( is_pv_domain(d) && iomem_access_permitted(d, pfn, pfn) && >>>>>> rangeset_contains_singleton(mmio_ro_ranges, pfn) ) >>>>>> return IOMMUF_readable; >>>>> >>>>> I don't think this would be fully correct, as we would still allow >>>>> mappings of IO-APIC pages explicitly banned in ->iomem_caps by not >>>>> handling those? >>>> >>>> CPU side mappings don't deal with the IO-APICs specifically. They only >>>> care about iomem_caps and mmio_ro_ranges. Hence explicitly banned >>>> IO-APIC pages cannot be mapped there either. (Of course we only do >>>> such banning if IO-APIC pages weren't possible to represent in >>>> mmio_ro_ranges, which should effectively be never.) >>> >>> I think I haven't expressed myself correctly. >>> >>> This construct won't return 0 for pfns not in iomem_caps, and hence >>> could allow mapping of addresses not in iomem_caps? >> >> I'm afraid I don't understand: There's an iomem_access_permitted() >> in the conditional. How would this allow mapping pages outside of >> iomem_caps? The default case higher up has already forced perms to >> zero for any non-RAM page (unless iommu_hwdom_inclusive). > > It was my understanding that when using iommu_hwdom_inclusive (or > iommu_hwdom_reserved if the IO-APIC page is a reserved region) we > still want to deny access to the IO-APIC page if it's not in > iomem_caps, and the proposed conditional won't do that. > > So I guess the discussion is really whether > iommu_hwdom_{inclusive,reserved} take precedence over ->iomem_caps? I think the intended interaction is not spelled out anywhere. I also think that it is to be expected for such interaction to be quirky; after all the options themselves are quirks. > It seems a bit inconsistent IMO to enforce mmio_ro_ranges but not > ->iomem_caps when using iommu_hwdom_{inclusive,reserved}. In a way, yes. But as said before - it's highly theoretical for IO-APIC pages to not be in ->iomem_caps (and this case also won't go silently). Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |