On 01.09.2021 15:48, Roger Pau Monné wrote: > On Wed, Sep 01, 2021 at 11:53:03AM +0200, Jan Beulich wrote: >> The issue isn't just with execution, though, and as a result I may >> need to change the logic here to also include at least W. As of >> one of the XSA-378 changes we may now pass just p2m_access_r to >> iommu_identity_mapping(), if the ACPI tables on an AMD system were >> saying so. (We may also pass p2m_access_w, but I sincerely hope no >> firmware would specify write but no read access.) >> >> Similarly in "IOMMU/x86: restrict IO-APIC mappings for PV Dom0" I >> now pass p2m_access_r to set_identity_p2m_entry(). > > Not really I think, as PVH dom0 is the only user of the > set_identity_p2m_entry call in arch_iommu_hwdom_init, and we should > never identity map the IO-APIC range in that case because a set of > emulated IO-APIC replacements are provided and those require ranges to > be unmapped so that accesses can be trapped.
Correct, this would be only a latent issue for now. The code passes that value, but that path is not going to be taken (until, perhaps, a future change). >> I suppose an underlying issue is the mixed purpose of using >> p2m_access_*, which possibly has been against the intentions in the >> first place. We cannot, for example, express r/o access to an MMIO >> page without using p2m_access_r (or p2m_access_rx), as there's no >> suitable p2m type to express this via type alone. We may need to >> split p2m_mmio_direct into multiple types (up to 7), I guess, if >> we wanted to remove this (ab)use of p2m_access_*. > > My main complaint is mostly with the fact that some MMIO ranges are > mapped without execute permissions when mapped by > set_identity_p2m_entry vs map_mmio_regions that will map them with the > default permissions and that has execution set. Right - as said in a reply to Andrew, we may need to drop some p2m_access_t parameters in favor of using default permissions. But at least for AMD's IOMMU-unity-mapped ranges we will need to have a way to express at least r/o. Jan
