| Message ID | 20240726225910.1912537-7-romank@linux.microsoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: hyperv: Support Virtual Trust Level Boot | expand |
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? Best regards, Krzysztof
On 27/07/2024 11:17, 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. > > 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. > > 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. Kernel's task is not to validate the DT. Even if it was, above code works poor. What if someone adds 'dma-noncoherent'? The job of the bindings and DT schema is to validate and check DT if is well formed. Best regards, Krzysztof
On 7/27/2024 1:56 AM, 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? No support for non-coherent. Wanted to do a sanity check. Had better check for dma-noncoherent and warn about that I believe. > > Best regards, > Krzysztof >
From: Roman Kisel <romank@linux.microsoft.com> 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. 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 >
On 8/4/2024 8:03 PM, Michael Kelley wrote: > From: Roman Kisel <romank@linux.microsoft.com> 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 >>
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c index 12a707ab73f8..7eee7caff5f6 100644 --- a/drivers/hv/vmbus_drv.c +++ b/drivers/hv/vmbus_drv.c @@ -2306,6 +2306,34 @@ static int vmbus_acpi_add(struct platform_device *pdev) } #endif +static int __maybe_unused vmbus_set_irq(struct platform_device *pdev) +{ + struct irq_desc *desc; + int irq; + + irq = platform_get_irq(pdev, 0); + if (irq == 0) { + pr_err("VMBus interrupt mapping failure\n"); + return -EINVAL; + } + if (irq < 0) { + pr_err("VMBus interrupt data can't be read from DeviceTree, error %d\n", irq); + return irq; + } + + desc = irq_to_desc(irq); + if (!desc) { + pr_err("No interrupt descriptor for VMBus virq %d\n", irq); + return -ENODEV; + } + + vmbus_irq = irq; + vmbus_interrupt = desc->irq_data.hwirq; + pr_debug("VMBus virq %d, hwirq %d\n", vmbus_irq, vmbus_interrupt); + + return 0; +} + static int vmbus_device_add(struct platform_device *pdev) { struct resource **cur_res = &hyperv_mmio; @@ -2320,6 +2348,12 @@ static int vmbus_device_add(struct platform_device *pdev) if (ret) return ret; +#ifndef HYPERVISOR_CALLBACK_VECTOR + ret = vmbus_set_irq(pdev); + if (ret) + return ret; +#endif + for_each_of_range(&parser, &range) { struct resource *res; @@ -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"); + pdev->dev.dma_coherent = true; + +#endif + return ret; }
The VMBus driver uses ACPI for interrupt assignment on arm64 hence it won't function in the VTL mode where only DeviceTree can be used. Update the VMBus driver to discover interrupt configuration via DeviceTree and indicate DMA cache coherency. Signed-off-by: Roman Kisel <romank@linux.microsoft.com> --- drivers/hv/vmbus_drv.c | 49 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)