diff mbox series

[v3,6/7] Drivers: hv: vmbus: Get the IRQ number from DT

Message ID 20240726225910.1912537-7-romank@linux.microsoft.com
State New
Headers show
Series arm64: hyperv: Support Virtual Trust Level Boot | expand

Commit Message

Roman Kisel July 26, 2024, 10:59 p.m. UTC
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(+)

Comments

Krzysztof Kozlowski July 27, 2024, 8:56 a.m. UTC | #1
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
Krzysztof Kozlowski July 27, 2024, 9:20 a.m. UTC | #2
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
Roman Kisel July 29, 2024, 4:36 p.m. UTC | #3
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
>
Michael Kelley Aug. 5, 2024, 3:03 a.m. UTC | #4
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
>
Roman Kisel Aug. 5, 2024, 4:26 p.m. UTC | #5
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 mbox series

Patch

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;
 }