| Message ID | 20240201170159.66330-5-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iommu/x86: fixes/improvements for unity range checks | expand |
On 01.02.2024 18:01, Roger Pau Monne wrote: > Currently when a unity range overlaps with memory being used as RAM by the > hypervisor the result would be that the IOMMU gets disabled. However that's > not enough, as even with the IOMMU disabled the device will still access the > affected RAM areas. Hmm, no, I think this is going too far. Not the least because it is s/will/may/. But also because if we really wanted such behavior, we ought to also parse the respective ACPI tables when the "iommu=off". > Note that IVMD or RMRR ranges being placed over RAM is a firmware bug. As written this is wrong: They're typically in RAM, just that the E820 type for that range should not be RAM_TYPE_CONVENTIONAL. > Doing so also allows to simplify the code and use a switch over the reported > memory type(s). I'm afraid this isn't right either: page_get_ram_type() can set multiple bits in its output. Jan
On Tue, Feb 06, 2024 at 12:49:08PM +0100, Jan Beulich wrote: > On 01.02.2024 18:01, Roger Pau Monne wrote: > > Currently when a unity range overlaps with memory being used as RAM by the > > hypervisor the result would be that the IOMMU gets disabled. However that's > > not enough, as even with the IOMMU disabled the device will still access the > > affected RAM areas. > > Hmm, no, I think this is going too far. Not the least because it is > s/will/may/. But also because if we really wanted such behavior, we > ought to also parse the respective ACPI tables when the "iommu=off". I guessed so, hence why it's the last patch in the series. TBH I think it's very unlikely that such system exist. > > Note that IVMD or RMRR ranges being placed over RAM is a firmware bug. > > As written this is wrong: They're typically in RAM, just that the E820 > type for that range should not be RAM_TYPE_CONVENTIONAL. Hm, yes, ÇI should have written 'over a RAM range in the memory map' or similar. > > Doing so also allows to simplify the code and use a switch over the reported > > memory type(s). > > I'm afraid this isn't right either: page_get_ram_type() can set > multiple bits in its output. It can indeed. But if the only bit set is RAM_TYPE_CONVENTIONAL then the page will be handled as RAM, and that's where Xen would be in trouble if a device is also using such page as a unity map. If the page however is RAM_TYPE_CONVENTIONAL | RAM_TYPE_RESERVED then the RESERVED type will take over the whole page, and it's no longer an issue to have a unity range covering it. Thanks, Roger.
On 07.02.2024 10:14, Roger Pau Monné wrote: > On Tue, Feb 06, 2024 at 12:49:08PM +0100, Jan Beulich wrote: >> On 01.02.2024 18:01, Roger Pau Monne wrote: >>> Currently when a unity range overlaps with memory being used as RAM by the >>> hypervisor the result would be that the IOMMU gets disabled. However that's >>> not enough, as even with the IOMMU disabled the device will still access the >>> affected RAM areas. >> >> Hmm, no, I think this is going too far. Not the least because it is >> s/will/may/. But also because if we really wanted such behavior, we >> ought to also parse the respective ACPI tables when the "iommu=off". > > I guessed so, hence why it's the last patch in the series. TBH I > think it's very unlikely that such system exist. And you think so despite knowing that on some systems one needs to manually specify RMRR regions? >>> Doing so also allows to simplify the code and use a switch over the reported >>> memory type(s). >> >> I'm afraid this isn't right either: page_get_ram_type() can set >> multiple bits in its output. > > It can indeed. But if the only bit set is RAM_TYPE_CONVENTIONAL then > the page will be handled as RAM, and that's where Xen would be in > trouble if a device is also using such page as a unity map. > > If the page however is RAM_TYPE_CONVENTIONAL | RAM_TYPE_RESERVED then > the RESERVED type will take over the whole page, and it's no longer an > issue to have a unity range covering it. Okay, if this is intentional, than saying so in a code comment would imo help quite a bit. Jan
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c index 63d4cb898218..9b977f84582f 100644 --- a/xen/drivers/passthrough/x86/iommu.c +++ b/xen/drivers/passthrough/x86/iommu.c @@ -806,10 +806,14 @@ bool __init iommu_unity_region_ok(mfn_t start, mfn_t end) for ( addr = start; !mfn_eq(addr, end); mfn_add(addr, 1) ) { - unsigned int type = page_get_ram_type(addr); - - if ( type == RAM_TYPE_UNKNOWN ) + /* + * Any page that's at least partially of type RESERVED, UNUSABLE or + * ACPI will be considered by Xen of being all of that type, and hence + * the problematic pages are those that are fully holes or RAM. + */ + switch ( page_get_ram_type(addr) ) { + case RAM_TYPE_UNKNOWN: if ( e820_add_range(mfn_to_maddr(addr), mfn_to_maddr(addr) + PAGE_SIZE, E820_RESERVED) ) continue; @@ -817,7 +821,10 @@ bool __init iommu_unity_region_ok(mfn_t start, mfn_t end) "IOMMU: page at %#" PRI_mfn " couldn't be reserved\n", mfn_x(addr)); return false; - } + + case RAM_TYPE_CONVENTIONAL: + panic("IOMMU: page at %#" PRI_mfn " overlaps RAM range\n", + mfn_x(addr)); /* * Types which aren't RAM are considered good enough. @@ -825,14 +832,7 @@ bool __init iommu_unity_region_ok(mfn_t start, mfn_t end) * force Xen into assuming the whole page as having that type in * practice. */ - if ( type & (RAM_TYPE_RESERVED | RAM_TYPE_ACPI | - RAM_TYPE_UNUSABLE) ) - continue; - - printk(XENLOG_WARNING - "IOMMU: page at %#" PRI_mfn " can't be converted\n", - mfn_x(addr)); - return false; + } } return true;
Currently when a unity range overlaps with memory being used as RAM by the hypervisor the result would be that the IOMMU gets disabled. However that's not enough, as even with the IOMMU disabled the device will still access the affected RAM areas. Note that IVMD or RMRR ranges being placed over RAM is a firmware bug. Doing so also allows to simplify the code and use a switch over the reported memory type(s). Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/drivers/passthrough/x86/iommu.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)