Message-ID:
 <SN6PR02MB4157D5A2DA10658D506EA1B1D4BE2@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Mon, 5 Aug 2024 03:03:19 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Roman Kisel <romank@...ux.microsoft.com>, "arnd@...db.de" <arnd@...db.de>,
	"bhelgaas@...gle.com" <bhelgaas@...gle.com>, "bp@...en8.de" <bp@...en8.de>,
	"catalin.marinas@....com" <catalin.marinas@....com>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"decui@...rosoft.com" <decui@...rosoft.com>, "haiyangz@...rosoft.com"
	<haiyangz@...rosoft.com>, "hpa@...or.com" <hpa@...or.com>, "kw@...ux.com"
	<kw@...ux.com>, "kys@...rosoft.com" <kys@...rosoft.com>, "lenb@...nel.org"
	<lenb@...nel.org>, "lpieralisi@...nel.org" <lpieralisi@...nel.org>,
	"mingo@...hat.com" <mingo@...hat.com>, "rafael@...nel.org"
	<rafael@...nel.org>, "robh@...nel.org" <robh@...nel.org>,
	"tglx@...utronix.de" <tglx@...utronix.de>, "wei.liu@...nel.org"
	<wei.liu@...nel.org>, "will@...nel.org" <will@...nel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-hyperv@...r.kernel.org"
	<linux-hyperv@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-pci@...r.kernel.org"
	<linux-pci@...r.kernel.org>, "x86@...nel.org" <x86@...nel.org>
CC: "apais@...rosoft.com" <apais@...rosoft.com>, "benhill@...rosoft.com"
	<benhill@...rosoft.com>, "ssengar@...rosoft.com" <ssengar@...rosoft.com>,
	"sunilmut@...rosoft.com" <sunilmut@...rosoft.com>, "vdso@...bites.dev"
	<vdso@...bites.dev>
Subject: RE: [PATCH v3 7/7] PCI: hv: Get vPCI MSI IRQ domain from DT

From: Roman Kisel <romank@...ux.microsoft.com> Sent: Friday, July 26, 2024 3:59 PM
> 
> 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 get vPCI MSI IRQ domain in the DeviceTree
> case, too.
> 
> Signed-off-by: Roman Kisel <romank@...ux.microsoft.com>

Overall, this makes sense to me, and I think it works. As you noted in the cover
letter for the patch series, it's a bit messy.  But see my two comments below.

> ---
>  drivers/hv/vmbus_drv.c              | 23 +++++++-----
>  drivers/pci/controller/pci-hyperv.c | 55 +++++++++++++++++++++++++++--
>  include/linux/hyperv.h              |  2 ++
>  3 files changed, 69 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 7eee7caff5f6..038bd9be64b7 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -45,7 +45,8 @@ struct vmbus_dynid {
>  	struct hv_vmbus_device_id id;
>  };
> 
> -static struct device  *hv_dev;
> +/* VMBus Root Device */
> +static struct device  *vmbus_root_device;
> 
>  static int hyperv_cpuhp_online;
> 
> @@ -80,9 +81,15 @@ static struct resource *fb_mmio;
>  static struct resource *hyperv_mmio;
>  static DEFINE_MUTEX(hyperv_mmio_lock);
> 
> +struct device *get_vmbus_root_device(void)
> +{
> +	return vmbus_root_device;
> +}
> +EXPORT_SYMBOL_GPL(get_vmbus_root_device);
> +
>  static int vmbus_exists(void)
>  {
> -	if (hv_dev == NULL)
> +	if (vmbus_root_device == NULL)
>  		return -ENODEV;
> 
>  	return 0;
> @@ -861,7 +868,7 @@ static int vmbus_dma_configure(struct device *child_device)
>  	 * On x86/x64 coherence is assumed and these calls have no effect.
>  	 */
>  	hv_setup_dma_ops(child_device,
> -		device_get_dma_attr(hv_dev) == DEV_DMA_COHERENT);
> +		device_get_dma_attr(vmbus_root_device) == DEV_DMA_COHERENT);
>  	return 0;
>  }
> 
> @@ -1892,7 +1899,7 @@ int vmbus_device_register(struct hv_device *child_device_obj)
>  		     &child_device_obj->channel->offermsg.offer.if_instance);
> 
>  	child_device_obj->device.bus = &hv_bus;
> -	child_device_obj->device.parent = hv_dev;
> +	child_device_obj->device.parent = vmbus_root_device;
>  	child_device_obj->device.release = vmbus_device_release;
> 
>  	child_device_obj->device.dma_parms = &child_device_obj->dma_parms;
> @@ -2253,7 +2260,7 @@ static int vmbus_acpi_add(struct platform_device *pdev)
>  	struct acpi_device *ancestor;
>  	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
> 
> -	hv_dev = &device->dev;
> +	vmbus_root_device = &device->dev;
> 
>  	/*
>  	 * Older versions of Hyper-V for ARM64 fail to include the _CCA
> @@ -2342,7 +2349,7 @@ static int vmbus_device_add(struct platform_device *pdev)
>  	struct device_node *np = pdev->dev.of_node;
>  	int ret;
> 
> -	hv_dev = &pdev->dev;
> +	vmbus_root_device = &pdev->dev;
> 
>  	ret = of_range_parser_init(&parser, np);
>  	if (ret)
> @@ -2675,7 +2682,7 @@ static int __init hv_acpi_init(void)
>  	if (ret)
>  		return ret;
> 
> -	if (!hv_dev) {
> +	if (!vmbus_root_device) {
>  		ret = -ENODEV;
>  		goto cleanup;
>  	}
> @@ -2706,7 +2713,7 @@ static int __init hv_acpi_init(void)
> 
>  cleanup:
>  	platform_driver_unregister(&vmbus_platform_driver);
> -	hv_dev = NULL;
> +	vmbus_root_device = NULL;
>  	return ret;
>  }
> 
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 5992280e8110..cdecefd1f9bd 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -50,6 +50,7 @@
>  #include <linux/irqdomain.h>
>  #include <linux/acpi.h>
>  #include <linux/sizes.h>
> +#include <linux/of_irq.h>
>  #include <asm/mshyperv.h>
> 
>  /*
> @@ -887,6 +888,35 @@ static const struct irq_domain_ops hv_pci_domain_ops = {
>  	.activate = hv_pci_vec_irq_domain_activate,
>  };
> 
> +#ifdef CONFIG_OF
> +
> +static struct irq_domain *hv_pci_of_irq_domain_parent(void)
> +{
> +	struct device_node *parent;
> +	struct irq_domain *domain;
> +
> +	parent = of_irq_find_parent(to_platform_device(get_vmbus_root_device())->dev.of_node);

I think the above can be simplified to:

	parent = of_irq_find_parent(get_vmbus_root_device()->of_node);

Converting the vmbus_root_device to the platform device, and then accessing
the "dev" field of the platform device puts you back where you started with
the vmbus_root_device.  See the code in vmbus_device_add() where the
vmbus_root_device is set to the dev field of the platform device.

> +	domain = NULL;
> +	if (parent) {
> +		domain = irq_find_host(parent);
> +		of_node_put(parent);
> +	}
> +
> +	/*
> +	 * `domain == NULL` shouldn't happen.
> +	 *
> +	 * If somehow the code does end up in that state, treat this as a configuration
> +	 * issue rather than a hard error, emit a warning, and let the code proceed.
> +	 * The NULL parent domain is an acceptable option for the `irq_domain_create_hierarchy`
> +	 * function called later.
> +	 */
> +	if (!domain)
> +		WARN_ONCE(1, "No interrupt-parent found, check the DeviceTree data.\n");
> +	return domain;
> +}

Here's a thought, which may or may not be a good one:  Push some or all
the functionality of hv_pci_of_irq_domain_parent() into vmbus_device_add().
In the simplest case, have vmbus_device_add() store the of_node (which is
already the "np" local variable) in a global static variable, and provide
hv_get_vmbus_of_node() instead of get_vmbus_root_device().

The next step to consider would be to compute the parent in
vmbus_device_add(), and provide hv_get_vmbus_parent_of_node() instead
of hv_get_vmbus_of_node(). One difference is that vmbus_device_add()
runs for both x86 and arm64, while hv_pci_of_irq_domain_parent() is for
arm64 only.  The parent node may not exist on x86, but maybe that isn't
really a problem.

Pushing everything into vmbus_device_add() would entail doing the
irq_find_host(parent) as well, and the accessor function would just
return the IRQ domain. In that case, hv_pci_of_irq_domain_parent()
can go away. The domain might be NULL on x86, but that's OK
because the accessor function won't be called on x86.

Maybe there's a snag that prevents this idea from working well,
particularly on x86 where the domain will be NULL. But if it works,
it seems slightly less messy to me, though that is a judgment call.
I'll leave it to you to decide, and I'm OK either way. :-)

Michael

> +
> +#endif
> +
>  static int hv_pci_irqchip_init(void)
>  {
>  	static struct hv_pci_chip_data *chip_data;
> @@ -906,10 +936,29 @@ static int hv_pci_irqchip_init(void)
>  	 * IRQ domain once enabled, should not be removed since there is no
>  	 * way to ensure that all the corresponding devices are also gone and
>  	 * no interrupts will be generated.
> +	 *
> +	 * In the ACPI case, the parent IRQ domain is supplied by the ACPI
> +	 * subsystem, and it is the default GSI domain pointing to the GIC.
> +	 * Neither is available outside of the ACPI subsystem, cannot avoid
> +	 * the messy ifdef below.
> +	 * There is apparently no such default in the OF subsystem, and
> +	 * `hv_pci_of_irq_domain_parent` finds the parent IRQ domain that
> +	 * points to the GIC as well.
> +	 * None of these two cases reaches for the MSI parent domain.
>  	 */
> -	hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
> -							  fn, &hv_pci_domain_ops,
> -							  chip_data);
> +#ifdef CONFIG_ACPI
> +	if (!acpi_disabled)
> +		hv_msi_gic_irq_domain = acpi_irq_create_hierarchy(0, HV_PCI_MSI_SPI_NR,
> +			fn, &hv_pci_domain_ops,
> +			chip_data);
> +#endif
> +#if defined(CONFIG_OF)
> +	if (!hv_msi_gic_irq_domain)
> +		hv_msi_gic_irq_domain = irq_domain_create_hierarchy(
> +			hv_pci_of_irq_domain_parent(), 0, HV_PCI_MSI_SPI_NR,
> +			fn, &hv_pci_domain_ops,
> +			chip_data);
> +#endif
> 
>  	if (!hv_msi_gic_irq_domain) {
>  		pr_err("Failed to create Hyper-V arm64 vPCI MSI IRQ domain\n");
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 5e39baa7f6cb..b4aa1f579a97 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1346,6 +1346,8 @@ static inline void *hv_get_drvdata(struct hv_device *dev)
>  	return dev_get_drvdata(&dev->device);
>  }
> 
> +struct device *get_vmbus_root_device(void);
> +
>  struct hv_ring_buffer_debug_info {
>  	u32 current_interrupt_mask;
>  	u32 current_read_index;
> --
> 2.34.1
> 

Powered by blists - more mailing lists