dmar: Disable PMR in driver attach routine

[kd]

Summary

Previously it was disabled only right before translation was enabled.
This way the disable logic is still executed even when translation is not be activated, e.g. with hw.iommu.dma=0 tunable set.
In order to do the disabling early a hw.dmar.pmr.disable tunable has to be set.
On some platforms we need to disable PMR in order for core dump to work.
At the same time it was observed that enabling translation has a significant impact on network performance.
With this patch PMR can be disabled, with IOMMU translation not being turned on by appending the following to the loader.conf:

hw.dmar.enable=1
hw.dmar.pmr.disable=1
hw.dmar.dma=0

[kib]

I am not sure that I like/agree with the patch. My main objection is that you are unconditionally tickle PMR despite DMAR state. In fact, it is quite far from the place where you put the call to dmar_disable_protected_regions(), to the actual enable of translation.

If your goal is to be able to just disable PMR, then add a knob that allows you to do just that: enable/disable PMR. IMO it is better suited both to the DMAR spec, and to your specific stated goal of just disabling PMR for one need.

[kd]

Hide the early disabling behind a hw.dmar.pmr.disable tunable. (Off by default.)
Leave the other places that call dmar_disable_protected_regions untouched.
Check the PMR state before disabling it. Without this a timeout is observed when we try to disable it the second time.

[kib]

As a second though, this control is global. Do you want it to be per-DMAR unit, in fact? [I am fine with this refine to be done later]

[kd]

In D34907#792838, @kib wrote:

As a second though, this control is global. Do you want it to be per-DMAR unit, in fact? [I am fine with this refine to be done later]

IMHO having one knob is better:

1. Other DMAR related tunables are "global", e.g. hw.dmar.enable, hw.dmar.dma.
2. From user perspective if they want to disable PMR it's probably on all units.

Otherwise we'd end up in a weird situation where some devices can access memory that was previously used by firmware and other don't.
I can't think of a scenario where that would be useful.

[kib]

In D34907#792865, @mindal_semihalf.com wrote:

In D34907#792838, @kib wrote:

As a second though, this control is global. Do you want it to be per-DMAR unit, in fact? [I am fine with this refine to be done later]

IMHO having one knob is better:

1. Other DMAR related tunables are "global", e.g. hw.dmar.enable, hw.dmar.dma.
2. From user perspective if they want to disable PMR it's probably on all units.

Otherwise we'd end up in a weird situation where some devices can access memory that was previously used by firmware and other don't.
I can't think of a scenario where that would be useful.

Global dmar.dma tunable exists for the purpose of enabling only interrupt remapping. IR is required on machines which have IOAPICs and more than 255 CPUs. In fact, this is perhaps the only purpose of this control. Idea is that dma mappings are enabled only for specific devices. You enable DMAR but disable dmar dma to have IR.

About the point that PMR only makes sense to disable globally, I am also not sure. For instance, if PMR was set up for something that can be externally plugged, it might be good idea to keep it enabled still.

[kd]

In D34907#792891, @kib wrote:

About the point that PMR only makes sense to disable globally, I am also not sure. For instance, if PMR was set up for something that can be externally plugged, it might be good idea to keep it enabled still.

The problem with this approach is that kernel doesn't know which memory regions are protected.
Because of that It can allocate a buffer from that region and try to use it for a DMA transaction with an externally plugged device.
Doing so will result in a bus abort, which has to be handled by a device driver and will lead to some cryptic errors.
For example trying to read from protected region during core dump results in the following:

 Dumping 707 out of 16133 MB:(ada0:ahcich1:0:0:0): WRITE_DMA. ACB: ca 00 7e 62 4d 42 00 00 00 00 80 00
(ada0:ahcich1:0:0:0): CAM status: CCB request was invalid
(ada0:ahcich1:0:0:0): Error 22, Unretryable error
Aborting dump due to I/O error.

Regardless I don't think that having PMR enabled during runtime gives us any security benefit.
It's meant to be used to protect memory occupied by firmware during early boot stages.
For example on my board PMR is applied over the first ~16MB of memory.

[kib]

If this is a laptop (judging by ahci/amount of memory), you most likely have two DMARs, one serving the GPU, and another covering all PCIe devices. In this configuration, whatever was protected by PMR for GPU, does not make sense to unprotect.