Re: [PATCH 2/4] x86/P2M: relax guarding of MMIO entries

[Jan Beulich <jbeulich@xxxxxxxx>]

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