[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 06/18] IOMMU/x86: restrict IO-APIC mappings for PV Dom0
On 02.12.2021 16:12, Roger Pau Monné wrote: > On Wed, Dec 01, 2021 at 12:45:12PM +0100, Jan Beulich wrote: >> On 01.12.2021 11:32, Roger Pau Monné wrote: >>> On Wed, Dec 01, 2021 at 10:27:21AM +0100, Jan Beulich wrote: >>>> On 01.12.2021 10:09, Roger Pau Monné wrote: >>>>> On Fri, Sep 24, 2021 at 11:46:57AM +0200, Jan Beulich wrote: >>>>>> @@ -267,44 +267,60 @@ static bool __hwdom_init hwdom_iommu_map >>>>>> * that fall in unusable ranges for PV Dom0. >>>>>> */ >>>>>> if ( (pfn > max_pfn && !mfn_valid(mfn)) || xen_in_range(pfn) ) >>>>>> - return false; >>>>>> + return 0; >>>>>> >>>>>> switch ( type = page_get_ram_type(mfn) ) >>>>>> { >>>>>> case RAM_TYPE_UNUSABLE: >>>>>> - return false; >>>>>> + return 0; >>>>>> >>>>>> case RAM_TYPE_CONVENTIONAL: >>>>>> if ( iommu_hwdom_strict ) >>>>>> - return false; >>>>>> + return 0; >>>>>> break; >>>>>> >>>>>> default: >>>>>> if ( type & RAM_TYPE_RESERVED ) >>>>>> { >>>>>> if ( !iommu_hwdom_inclusive && !iommu_hwdom_reserved ) >>>>>> - return false; >>>>>> + perms = 0; >>>>>> } >>>>>> - else if ( is_hvm_domain(d) || !iommu_hwdom_inclusive || pfn > >>>>>> max_pfn ) >>>>>> - return false; >>>>>> + else if ( is_hvm_domain(d) ) >>>>>> + return 0; >>>>>> + else if ( !iommu_hwdom_inclusive || pfn > max_pfn ) >>>>>> + perms = 0; >>>>> >>>>> I'm confused about the reason to set perms = 0 instead of just >>>>> returning here. AFAICT perms won't be set to any other value below, >>>>> so you might as well just return 0. >>>> >>>> This is so that ... >>>> >>>>>> } >>>>>> >>>>>> /* Check that it doesn't overlap with the Interrupt Address Range. >>>>>> */ >>>>>> if ( pfn >= 0xfee00 && pfn <= 0xfeeff ) >>>>>> - return false; >>>>>> + return 0; >>>>>> /* ... or the IO-APIC */ >>>>>> - for ( i = 0; has_vioapic(d) && i < d->arch.hvm.nr_vioapics; i++ ) >>>>>> - if ( pfn == PFN_DOWN(domain_vioapic(d, i)->base_address) ) >>>>>> - return false; >>>>>> + if ( has_vioapic(d) ) >>>>>> + { >>>>>> + for ( i = 0; i < d->arch.hvm.nr_vioapics; i++ ) >>>>>> + if ( pfn == PFN_DOWN(domain_vioapic(d, i)->base_address) ) >>>>>> + return 0; >>>>>> + } >>>>>> + else if ( is_pv_domain(d) ) >>>>>> + { >>>>>> + /* >>>>>> + * Be consistent with CPU mappings: Dom0 is permitted to >>>>>> establish r/o >>>>>> + * ones there, so it should also have such established for >>>>>> IOMMUs. >>>>>> + */ >>>>>> + for ( i = 0; i < nr_ioapics; i++ ) >>>>>> + if ( pfn == PFN_DOWN(mp_ioapics[i].mpc_apicaddr) ) >>>>>> + return rangeset_contains_singleton(mmio_ro_ranges, pfn) >>>>>> + ? IOMMUF_readable : 0; >>>>>> + } >>>> >>>> ... this return, as per the comment, takes precedence over returning >>>> zero. >>> >>> I see. This is because you want to map those in the IOMMU page tables >>> even if the IO-APIC ranges are outside of a reserved region. >>> >>> I have to admit this is kind of weird, because the purpose of this >>> function is to add mappings for all memory below 4G, and/or for all >>> reserved regions. >> >> Well, that was what it started out as. The purpose here is to be consistent >> about IO-APICs: Either have them all mapped, or none of them. Since we map >> them in the CPU page tables and since Andrew asked for the two mappings to >> be consistent, this is the only way to satisfy the requests. Personally I'd >> be okay with not mapping IO-APICs here (but then regardless of whether they >> are covered by a reserved region). > > I'm unsure of the best way to deal with this, it seems like both > the CPU and the IOMMU page tables would never be equal for PV dom0, > because we have no intention to map the MSI-X tables in RO mode in the > IOMMU page tables. > > I'm not really opposed to having the IO-APIC mapped RO in the IOMMU > page tables, but I also don't see much benefit of doing it unless we > have a user-case for it. The IO-APIC handling in PV is already > different from native, so I would be fine if we add a comment noting > that while the IO-APIC is mappable to the CPU page tables as RO it's > not present in the IOMMU page tables (and then adjust hwdom_iommu_map > to prevent it's mapping). Andrew, you did request both mappings to get in sync - thoughts? > I think we should also prevent mapping of the LAPIC, the HPET and the > HyperTransport range in case they fall into a reserved region? Probably. > TBH looks like we should be using iomem_access_permitted in > arch_iommu_hwdom_init? (not just for the IO-APIC, but for other device > ranges) In general - perhaps. Not sure though whether to switch to doing so right here. >>>>>> @@ -346,15 +362,19 @@ void __hwdom_init arch_iommu_hwdom_init( >>>>>> for ( ; i < top; i++ ) >>>>>> { >>>>>> unsigned long pfn = pdx_to_pfn(i); >>>>>> + unsigned int perms = hwdom_iommu_map(d, pfn, max_pfn); >>>>>> int rc; >>>>>> >>>>>> - if ( !hwdom_iommu_map(d, pfn, max_pfn) ) >>>>>> + if ( !perms ) >>>>>> rc = 0; >>>>>> else if ( paging_mode_translate(d) ) >>>>>> - rc = set_identity_p2m_entry(d, pfn, p2m_access_rw, 0); >>>>>> + rc = set_identity_p2m_entry(d, pfn, >>>>>> + perms & IOMMUF_writable ? >>>>>> p2m_access_rw >>>>>> + : >>>>>> p2m_access_r, >>>>>> + 0); >>>>>> else >>>>>> rc = iommu_map(d, _dfn(pfn), _mfn(pfn), 1ul << >>>>>> PAGE_ORDER_4K, >>>>>> - IOMMUF_readable | IOMMUF_writable, >>>>>> &flush_flags); >>>>>> + perms, &flush_flags); >>>>> >>>>> You could just call set_identity_p2m_entry uniformly here. It will >>>>> DTRT for non-translated guests also, and then hwdom_iommu_map could >>>>> perhaps return a p2m_access_t? >>>> >>>> That's an orthogonal change imo, i.e. could be done as a prereq change, >>>> but I'd prefer to leave it as is for now. Furthermore see "x86/mm: split >>>> set_identity_p2m_entry() into PV and HVM parts": In v2 I'm now also >>>> adjusting the code here >>> >>> I would rather adjust the code here to just call >>> set_identity_p2m_entry instead of differentiating between PV and >>> HVM. >> >> I'm a little hesitant, in particular considering your suggestion to >> then have hwdom_iommu_map() return p2m_access_t. Andrew has made quite >> clear that the use of p2m_access_* here and in a number of other places >> is actually an abuse. >> >> Plus - forgot about this in my earlier reply - there would also be a >> conflict with the next patch in this series, where larger orders will >> get passed to iommu_map(), while set_identity_p2m_entry() has no >> respective parameter yet (and imo it isn't urgent for it to gain one). > > I've seen now as the iommu_map path is modified to handle ranges > instead of single pages. Long term we also want to expand this range > handling to the HVM branch. Long (or maybe better mid) term, yes. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |