diff mbox series

[v2,6/6] drivers/pci/hyperv/arm64: vPCI MSI IRQ domain from DT

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

Commit Message

Roman Kisel May 14, 2024, 10:43 p.m. UTC
The hyperv-pci driver uses ACPI for MSI IRQ domain configuration
on arm64 thereby it won't be able to do that in the VTL mode where
only DeviceTree can be used.

Update the hyperv-pci driver to discover interrupt configuration
via DeviceTree.

Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
---
 drivers/pci/controller/pci-hyperv.c | 13 ++++++++++---
 include/linux/acpi.h                |  9 +++++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

Comments

Saurabh Singh Sengar May 15, 2024, 9:48 a.m. UTC | #1
On Tue, May 14, 2024 at 03:43:53PM -0700, Roman Kisel wrote:
> The hyperv-pci driver uses ACPI for MSI IRQ domain configuration
> on arm64 thereby it won't be able to do that in the VTL mode where
> only DeviceTree can be used.
> 
> Update the hyperv-pci driver to discover interrupt configuration
> via DeviceTree.

Subject prefix should be "PCI: hv:"

> 
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
>  drivers/pci/controller/pci-hyperv.c | 13 ++++++++++---
>  include/linux/acpi.h                |  9 +++++++++
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 1eaffff40b8d..ccc2b54206f4 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -906,9 +906,16 @@ static int hv_pci_irqchip_init(void)
>  	 * way to ensure that all the corresponding devices are also gone and
>  	 * no interrupts will be generated.
>  	 */
> -	hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
> -							  fn, &hv_pci_domain_ops,
> -							  chip_data);
> +	if (acpi_disabled)
> +		hv_msi_gic_irq_domain = irq_domain_create_hierarchy(
> +			irq_find_matching_fwnode(fn, DOMAIN_BUS_ANY),
> +			0, HV_PCI_MSI_SPI_NR,
> +			fn, &hv_pci_domain_ops,
> +			chip_data);
> +	else
> +		hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
> +			fn, &hv_pci_domain_ops,
> +			chip_data);

Upto 100 characters per line are supported now, we can have less
line breaks.

>  
>  	if (!hv_msi_gic_irq_domain) {
>  		pr_err("Failed to create Hyper-V arm64 vPCI MSI IRQ domain\n");
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index b7165e52b3c6..498cbb2c40a1 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1077,6 +1077,15 @@ static inline bool acpi_sleep_state_supported(u8 sleep_state)
>  	return false;
>  }
>  
> +static inline struct irq_domain *acpi_irq_create_hierarchy(unsigned int flags,
> +					     unsigned int size,
> +					     struct fwnode_handle *fwnode,
> +					     const struct irq_domain_ops *ops,
> +					     void *host_data)
> +{
> +	return NULL;
> +}
> +
>  #endif	/* !CONFIG_ACPI */
>  
>  extern void arch_post_acpi_subsys_init(void);
> -- 
> 2.45.0
>
Michael Kelley May 15, 2024, 1:47 p.m. UTC | #2
From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, May 14, 2024 3:44 PM
> 
> The hyperv-pci driver uses ACPI for MSI IRQ domain configuration
> on arm64 thereby it won't be able to do that in the VTL mode where
> only DeviceTree can be used.

That sentence seems a bit weird.  How about:

   The hyperv-pci driver uses ACPI for MSI IRQ domain configuration on arm64.
   It won't be able to do that in the VTL mode where only DeviceTree can be used.

> 
> Update the hyperv-pci driver to discover interrupt configuration
> via DeviceTree.

"discover interrupt configuration"?   I think that's a cut-and-paste error
from the previous patch.

> 
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
>  drivers/pci/controller/pci-hyperv.c | 13 ++++++++++---
>  include/linux/acpi.h                |  9 +++++++++
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 1eaffff40b8d..ccc2b54206f4 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -906,9 +906,16 @@ static int hv_pci_irqchip_init(void)
>  	 * way to ensure that all the corresponding devices are also gone and
>  	 * no interrupts will be generated.
>  	 */
> -	hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
> -							  fn, &hv_pci_domain_ops,
> -							  chip_data);
> +	if (acpi_disabled)
> +		hv_msi_gic_irq_domain = irq_domain_create_hierarchy(
> +			irq_find_matching_fwnode(fn, DOMAIN_BUS_ANY),
> +			0, HV_PCI_MSI_SPI_NR,
> +			fn, &hv_pci_domain_ops,
> +			chip_data);

Does the above really work?  It seems doubtful to me that irq_find_matching_fwnode()
always finds the parent domain that you want.  But I don't have a deep understanding
of how this works or is supposed to work, so I don't know for sure.

If the above *does* actually work for all cases, then should it also work for the ACPI
case?  Then you could avoid the messiness when acpi_irq_create_hierarchy() doesn't
exist.

> +	else
> +		hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
> +			fn, &hv_pci_domain_ops,
> +			chip_data);
> 
>  	if (!hv_msi_gic_irq_domain) {
>  		pr_err("Failed to create Hyper-V arm64 vPCI MSI IRQ domain\n");

I'm wondering if these are the only changes needed to make vPCI work on
arm64 with DeviceTree.  The DMA coherence issue I mentioned in the previous patch
definitely affects vPCI devices, so it needs to be fully understood and verified to work
correctly.

> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index b7165e52b3c6..498cbb2c40a1 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1077,6 +1077,15 @@ static inline bool acpi_sleep_state_supported(u8 sleep_state)
>  	return false;
>  }
> 
> +static inline struct irq_domain *acpi_irq_create_hierarchy(unsigned int flags,
> +					     unsigned int size,
> +					     struct fwnode_handle *fwnode,
> +					     const struct irq_domain_ops *ops,
> +					     void *host_data)
> +{
> +	return NULL;
> +}
> +
>  #endif	/* !CONFIG_ACPI */
> 
>  extern void arch_post_acpi_subsys_init(void);
> --
> 2.45.0
>
Roman Kisel May 15, 2024, 4:34 p.m. UTC | #3
On 5/15/2024 2:48 AM, Saurabh Singh Sengar wrote:
> On Tue, May 14, 2024 at 03:43:53PM -0700, Roman Kisel wrote:
>> The hyperv-pci driver uses ACPI for MSI IRQ domain configuration
>> on arm64 thereby it won't be able to do that in the VTL mode where
>> only DeviceTree can be used.
>>
>> Update the hyperv-pci driver to discover interrupt configuration
>> via DeviceTree.
> 
> Subject prefix should be "PCI: hv:"
> 
Thanks!

>>
>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
>> ---
>>   drivers/pci/controller/pci-hyperv.c | 13 ++++++++++---
>>   include/linux/acpi.h                |  9 +++++++++
>>   2 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
>> index 1eaffff40b8d..ccc2b54206f4 100644
>> --- a/drivers/pci/controller/pci-hyperv.c
>> +++ b/drivers/pci/controller/pci-hyperv.c
>> @@ -906,9 +906,16 @@ static int hv_pci_irqchip_init(void)
>>   	 * way to ensure that all the corresponding devices are also gone and
>>   	 * no interrupts will be generated.
>>   	 */
>> -	hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
>> -							  fn, &hv_pci_domain_ops,
>> -							  chip_data);
>> +	if (acpi_disabled)
>> +		hv_msi_gic_irq_domain = irq_domain_create_hierarchy(
>> +			irq_find_matching_fwnode(fn, DOMAIN_BUS_ANY),
>> +			0, HV_PCI_MSI_SPI_NR,
>> +			fn, &hv_pci_domain_ops,
>> +			chip_data);
>> +	else
>> +		hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
>> +			fn, &hv_pci_domain_ops,
>> +			chip_data);
> 
> Upto 100 characters per line are supported now, we can have less
> line breaks.
> 
Fewer line breaks would make this look nicer, let me know if you had any 
particular style in mind.

>>   
>>   	if (!hv_msi_gic_irq_domain) {
>>   		pr_err("Failed to create Hyper-V arm64 vPCI MSI IRQ domain\n");
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index b7165e52b3c6..498cbb2c40a1 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -1077,6 +1077,15 @@ static inline bool acpi_sleep_state_supported(u8 sleep_state)
>>   	return false;
>>   }
>>   
>> +static inline struct irq_domain *acpi_irq_create_hierarchy(unsigned int flags,
>> +					     unsigned int size,
>> +					     struct fwnode_handle *fwnode,
>> +					     const struct irq_domain_ops *ops,
>> +					     void *host_data)
>> +{
>> +	return NULL;
>> +}
>> +
>>   #endif	/* !CONFIG_ACPI */
>>   
>>   extern void arch_post_acpi_subsys_init(void);
>> -- 
>> 2.45.0
>>
Bjorn Helgaas May 15, 2024, 6:12 p.m. UTC | #4
On Wed, May 15, 2024 at 09:34:09AM -0700, Roman Kisel wrote:
> 
> 
> On 5/15/2024 2:48 AM, Saurabh Singh Sengar wrote:
> > On Tue, May 14, 2024 at 03:43:53PM -0700, Roman Kisel wrote:
> > > The hyperv-pci driver uses ACPI for MSI IRQ domain configuration
> > > on arm64 thereby it won't be able to do that in the VTL mode where
> > > only DeviceTree can be used.
> > > 
> > > Update the hyperv-pci driver to discover interrupt configuration
> > > via DeviceTree.
> > 
> > Subject prefix should be "PCI: hv:"
> > 
> Thanks!

"git log --oneline <file>" is a good guide in general and could be
used for other patches in this series as well.

> > > +		hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
> > > +			fn, &hv_pci_domain_ops,
> > > +			chip_data);
> > 
> > Upto 100 characters per line are supported now, we can have less
> > line breaks.
> > 
> Fewer line breaks would make this look nicer, let me know if you had any
> particular style in mind.

Let's not use the checkpatch "$max_line_length = 100" as a guide.

The pci-hyperv.c file as a whole is obviously formatted to fit in 80
columns with few exceptions.

IMO it would not be an improvement to scatter random 100-column lines
throughout.  That would just mean the file would look bad in an
80-column terminal and there would be a lot of wasted space in a
100-column terminal.

Bjorn
Roman Kisel May 15, 2024, 6:31 p.m. UTC | #5
On 5/15/2024 6:47 AM, Michael Kelley wrote:
> From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, May 14, 2024 3:44 PM
>>
>> The hyperv-pci driver uses ACPI for MSI IRQ domain configuration
>> on arm64 thereby it won't be able to do that in the VTL mode where
>> only DeviceTree can be used.
> 
> That sentence seems a bit weird.  How about:
> 
>     The hyperv-pci driver uses ACPI for MSI IRQ domain configuration on arm64.
>     It won't be able to do that in the VTL mode where only DeviceTree can be used.
> 
Agreed, appreciate your wordsmithing :)

>>
>> Update the hyperv-pci driver to discover interrupt configuration
>> via DeviceTree.
> 
> "discover interrupt configuration"?   I think that's a cut-and-paste error
> from the previous patch.
> 
Guilty as charged :) Will fix.

>>
>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
>> ---
>>   drivers/pci/controller/pci-hyperv.c | 13 ++++++++++---
>>   include/linux/acpi.h                |  9 +++++++++
>>   2 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
>> index 1eaffff40b8d..ccc2b54206f4 100644
>> --- a/drivers/pci/controller/pci-hyperv.c
>> +++ b/drivers/pci/controller/pci-hyperv.c
>> @@ -906,9 +906,16 @@ static int hv_pci_irqchip_init(void)
>>   	 * way to ensure that all the corresponding devices are also gone and
>>   	 * no interrupts will be generated.
>>   	 */
>> -	hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
>> -							  fn, &hv_pci_domain_ops,
>> -							  chip_data);
>> +	if (acpi_disabled)
>> +		hv_msi_gic_irq_domain = irq_domain_create_hierarchy(
>> +			irq_find_matching_fwnode(fn, DOMAIN_BUS_ANY),
>> +			0, HV_PCI_MSI_SPI_NR,
>> +			fn, &hv_pci_domain_ops,
>> +			chip_data);
> 
> Does the above really work?  It seems doubtful to me that irq_find_matching_fwnode()
> always finds the parent domain that you want.  But I don't have a deep understanding
> of how this works or is supposed to work, so I don't know for sure.
> 
> If the above *does* actually work for all cases, then should it also work for the ACPI
> case?  Then you could avoid the messiness when acpi_irq_create_hierarchy() doesn't
> exist.
> 
Have not got a system to validate this on. Conceptually (at my level of 
ignorance) didn't look very off... Will use the "collapsed" version 
you're suggesting as the litmus test.

>> +	else
>> +		hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
>> +			fn, &hv_pci_domain_ops,
>> +			chip_data);
>>
>>   	if (!hv_msi_gic_irq_domain) {
>>   		pr_err("Failed to create Hyper-V arm64 vPCI MSI IRQ domain\n");
> 
> I'm wondering if these are the only changes needed to make vPCI work on
> arm64 with DeviceTree.  The DMA coherence issue I mentioned in the previous patch
> definitely affects vPCI devices, so it needs to be fully understood and verified to work
> correctly.
> 
Likely not all as the code is venturing into the new territory composed 
of the pieces never snapped in place together before. Will work on the 
previous patch to resolve as many concerns as possible.

>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index b7165e52b3c6..498cbb2c40a1 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -1077,6 +1077,15 @@ static inline bool acpi_sleep_state_supported(u8 sleep_state)
>>   	return false;
>>   }
>>
>> +static inline struct irq_domain *acpi_irq_create_hierarchy(unsigned int flags,
>> +					     unsigned int size,
>> +					     struct fwnode_handle *fwnode,
>> +					     const struct irq_domain_ops *ops,
>> +					     void *host_data)
>> +{
>> +	return NULL;
>> +}
>> +
>>   #endif	/* !CONFIG_ACPI */
>>
>>   extern void arch_post_acpi_subsys_init(void);
>> --
>> 2.45.0
>>
Roman Kisel May 15, 2024, 6:34 p.m. UTC | #6
On 5/15/2024 11:12 AM, Bjorn Helgaas wrote:
> On Wed, May 15, 2024 at 09:34:09AM -0700, Roman Kisel wrote:
>>
>>
>> On 5/15/2024 2:48 AM, Saurabh Singh Sengar wrote:
>>> On Tue, May 14, 2024 at 03:43:53PM -0700, Roman Kisel wrote:
>>>> The hyperv-pci driver uses ACPI for MSI IRQ domain configuration
>>>> on arm64 thereby it won't be able to do that in the VTL mode where
>>>> only DeviceTree can be used.
>>>>
>>>> Update the hyperv-pci driver to discover interrupt configuration
>>>> via DeviceTree.
>>>
>>> Subject prefix should be "PCI: hv:"
>>>
>> Thanks!
> 
> "git log --oneline <file>" is a good guide in general and could be
> used for other patches in this series as well.
> 
Many thanks for suggesting that :)

>>>> +		hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
>>>> +			fn, &hv_pci_domain_ops,
>>>> +			chip_data);
>>>
>>> Upto 100 characters per line are supported now, we can have less
>>> line breaks.
>>>
>> Fewer line breaks would make this look nicer, let me know if you had any
>> particular style in mind.
> 
> Let's not use the checkpatch "$max_line_length = 100" as a guide.
> 
> The pci-hyperv.c file as a whole is obviously formatted to fit in 80
> columns with few exceptions.
> 
> IMO it would not be an improvement to scatter random 100-column lines
> throughout.  That would just mean the file would look bad in an
> 80-column terminal and there would be a lot of wasted space in a
> 100-column terminal.
> 
Appreciate showing me the data-driven way of reasoning about that very much!

> Bjorn
Bjorn Helgaas June 7, 2024, 7:55 p.m. UTC | #7
On Wed, May 15, 2024 at 01:12:38PM -0500, Bjorn Helgaas wrote:
> On Wed, May 15, 2024 at 09:34:09AM -0700, Roman Kisel wrote:
> > 
> > 
> > On 5/15/2024 2:48 AM, Saurabh Singh Sengar wrote:
> > > On Tue, May 14, 2024 at 03:43:53PM -0700, Roman Kisel wrote:
> > > > The hyperv-pci driver uses ACPI for MSI IRQ domain configuration
> > > > on arm64 thereby it won't be able to do that in the VTL mode where
> > > > only DeviceTree can be used.
> > > > 
> > > > Update the hyperv-pci driver to discover interrupt configuration
> > > > via DeviceTree.
> > > 
> > > Subject prefix should be "PCI: hv:"

I forgot to also suggest that the subject line begin with a verb,
e.g., "Get vPCI MSI IRQ domain from DT" or similar, again so it reads
consistently with previous commits.

Oh, I see patch 5/6, "Get the irq number from DeviceTree" is also very
similar.  It would be nice if they matched, e.g., both used "IRQ" and
"DT".

Bjorn
Roman Kisel June 11, 2024, 2:40 p.m. UTC | #8
On 6/7/2024 12:55 PM, Bjorn Helgaas wrote:
> On Wed, May 15, 2024 at 01:12:38PM -0500, Bjorn Helgaas wrote:
>> On Wed, May 15, 2024 at 09:34:09AM -0700, Roman Kisel wrote:
>>>
>>>
>>> On 5/15/2024 2:48 AM, Saurabh Singh Sengar wrote:
>>>> On Tue, May 14, 2024 at 03:43:53PM -0700, Roman Kisel wrote:
>>>>> The hyperv-pci driver uses ACPI for MSI IRQ domain configuration
>>>>> on arm64 thereby it won't be able to do that in the VTL mode where
>>>>> only DeviceTree can be used.
>>>>>
>>>>> Update the hyperv-pci driver to discover interrupt configuration
>>>>> via DeviceTree.
>>>>
>>>> Subject prefix should be "PCI: hv:"
> 
> I forgot to also suggest that the subject line begin with a verb,
> e.g., "Get vPCI MSI IRQ domain from DT" or similar, again so it reads
> consistently with previous commits.
> 
> Oh, I see patch 5/6, "Get the irq number from DeviceTree" is also very
> similar.  It would be nice if they matched, e.g., both used "IRQ" and
> "DT".
> 
> Bjorn

Will update, thanks! Going to send another version during the next week 
most likely.
diff mbox series

Patch

diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 1eaffff40b8d..ccc2b54206f4 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -906,9 +906,16 @@  static int hv_pci_irqchip_init(void)
 	 * way to ensure that all the corresponding devices are also gone and
 	 * no interrupts will be generated.
 	 */
-	hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
-							  fn, &hv_pci_domain_ops,
-							  chip_data);
+	if (acpi_disabled)
+		hv_msi_gic_irq_domain = irq_domain_create_hierarchy(
+			irq_find_matching_fwnode(fn, DOMAIN_BUS_ANY),
+			0, HV_PCI_MSI_SPI_NR,
+			fn, &hv_pci_domain_ops,
+			chip_data);
+	else
+		hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
+			fn, &hv_pci_domain_ops,
+			chip_data);
 
 	if (!hv_msi_gic_irq_domain) {
 		pr_err("Failed to create Hyper-V arm64 vPCI MSI IRQ domain\n");
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index b7165e52b3c6..498cbb2c40a1 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1077,6 +1077,15 @@  static inline bool acpi_sleep_state_supported(u8 sleep_state)
 	return false;
 }
 
+static inline struct irq_domain *acpi_irq_create_hierarchy(unsigned int flags,
+					     unsigned int size,
+					     struct fwnode_handle *fwnode,
+					     const struct irq_domain_ops *ops,
+					     void *host_data)
+{
+	return NULL;
+}
+
 #endif	/* !CONFIG_ACPI */
 
 extern void arch_post_acpi_subsys_init(void);