On 8/4/2024 8:03 PM, Michael Kelley wrote:
From: Roman Kisel <romank@xxxxxxxxxxxxxxxxxxx> Sent: Monday, July 29, 2024 9:51 AM
On 7/27/2024 2:17 AM, Arnd Bergmann wrote:
On Sat, Jul 27, 2024, at 10:56, Krzysztof Kozlowski wrote:
On 27/07/2024 00:59, Roman Kisel wrote:
@@ -2338,6 +2372,21 @@ static int vmbus_device_add(struct platform_device *pdev)
cur_res = &res->sibling;
}
+ /*
+ * Hyper-V always assumes DMA cache coherency, and the DMA subsystem
+ * might default to 'not coherent' on some architectures.
+ * Avoid high-cost cache coherency maintenance done by the CPU.
+ */
+#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
+ defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
+ defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
+
+ if (!of_property_read_bool(np, "dma-coherent"))
+ pr_warn("Assuming cache coherent DMA transactions, no 'dma-coherent' node supplied\n");
Why do you need this property at all, if it is allways dma-coherent? Are
you supporting dma-noncoherent somewhere?
It's just a sanity check that the DT is well-formed.
In my view, this chunk of code can be dropped entirely. The guest
should believe what the Hyper-V host tells it via DT, and that includes
operating in non-coherent mode. There might be some future case
where non-coherent DMA is correct. In such a case, we don't want to
have to come back and remove an overly aggressive sanity test from
Linux kernel code.
As Arnd noted, the dma-coherent (or dma-noncoherent) property should
be interpreted and applied to the device by common code. If that's not
working for some reason in this case, we should investigate why not.
Note that the ACPI code for VMBus does the same thing -- it believes and
uses whatever the _CCA property says. The exception is that there
are deployed version of Hyper-V that don't set _CCA at all, contrary to the
ACPI spec. So there's a hack in vmbus_acpi_add() to work around this case
and force coherent_dma. But that's the only place where the current
Hyper-V assumption of coherence comes into play. I sincerely hope Hyper-V
ensures that the DT correctly includes dma-coherent from the start, and
that we don't have to replicate the hack on the DT side.
I was replicating the _CCA hack diligently a bit much too much, agreed.
This great conversation really gives me reassurance that the code
doesn't have to be paranoid, and I can happily remove this if statement.
Michael
Since the dma-coherent property is interpreted by common code, it's
not up to hv to change the default for the platform. I'm not sure
if the presence of CONFIG_ARCH_HAS_SYNC_DMA_* options is the correct
check to determine that an architecture defaults to noncoherent
though, as the function may be needed to do something else.
I used the ifdef as the dma_coherent field is declared under these macros:
#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU) || \
defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU_ALL)
extern bool dma_default_coherent;
static inline bool dev_is_dma_coherent(struct device *dev)
{
return dev->dma_coherent;
}
#else
#define dma_default_coherent true
static inline bool dev_is_dma_coherent(struct device *dev)
{
return true;
}
i.e., there is no API to set dma_coherent. As I see it, the options
are either warn the user if they forgot to add `dma-coherent`
if (!dev_is_dma_coherent(dev)) pr_warn("add dma-coherent to be faster\n"),
or warn and force the flag to true. Maybe just warn
the user I think now... The code will be cleaner (no need to emulate
a-would-be set_dma_coherent) , and the user will
know how to make the system perform at its best.
Appreciate sharing the reservations about that piece!
The global "dma_default_coherent' may be a better thing to check
for. This is e.g. set on powerpc64, riscv and on specific mips
platforms, but it's never set on arm64 as far as I can tell.
Arnd
--
Thank you,
Roman
--
Thank you,
Roman
