| Message ID | 20250212014321.1108840-7-romank@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | arm64: hyperv: Support Virtual Trust Level Boot | expand |
On Tue, Feb 11, 2025 at 05:43:21PM -0800, Roman Kisel wrote: > 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@linux.microsoft.com> > --- > drivers/hv/vmbus_drv.c | 23 ++++++---- > drivers/pci/controller/pci-hyperv.c | 69 ++++++++++++++++++++++++++--- > include/linux/hyperv.h | 2 + > 3 files changed, 80 insertions(+), 14 deletions(-) > > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c > index 9d0c2dbd2a69..3f0f9f01b520 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 *hv_get_vmbus_root_device(void) > +{ > + return vmbus_root_device; > +} > +EXPORT_SYMBOL_GPL(hv_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; > } > > @@ -1920,7 +1927,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; > @@ -2282,7 +2289,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 > @@ -2373,7 +2380,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) > @@ -2692,7 +2699,7 @@ static int __init hv_acpi_init(void) > if (ret) > return ret; > > - if (!hv_dev) { > + if (!vmbus_root_device) { > ret = -ENODEV; > goto cleanup; > } > @@ -2723,7 +2730,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 cdd5be16021d..24725bea9ef1 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> > > /* > @@ -817,9 +818,17 @@ static int hv_pci_vec_irq_gic_domain_alloc(struct irq_domain *domain, > int ret; > > fwspec.fwnode = domain->parent->fwnode; > - fwspec.param_count = 2; > - fwspec.param[0] = hwirq; > - fwspec.param[1] = IRQ_TYPE_EDGE_RISING; > + if (is_of_node(fwspec.fwnode)) { > + /* SPI lines for OF translations start at offset 32 */ > + fwspec.param_count = 3; > + fwspec.param[0] = 0; > + fwspec.param[1] = hwirq - 32; > + fwspec.param[2] = IRQ_TYPE_EDGE_RISING; > + } else { > + fwspec.param_count = 2; > + fwspec.param[0] = hwirq; > + fwspec.param[1] = IRQ_TYPE_EDGE_RISING; > + } > > ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec); > if (ret) > @@ -887,6 +896,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(hv_get_vmbus_root_device()->of_node); > + 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. The rest of this file fits in 80 columns; please wrap this to match. > + */ > + if (!domain) > + WARN_ONCE(1, "No interrupt-parent found, check the DeviceTree data.\n"); Is there a way to include a hint about what specific part of the devicetree to look at, e.g., the node that lacks a parent? > + return domain; > +} > + > +#endif > + > static int hv_pci_irqchip_init(void) > { > static struct hv_pci_chip_data *chip_data; > @@ -906,10 +944,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. Add a blank line if you intend a new paragraph here. Otherwise, wrap to fill 78 columns or so. > + * 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. And here. > + * None of these two cases reaches for the MSI parent domain. I don't know what "reaches for the MSI parent domain" means. Neither "searches for"? > */ > - 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 I don't know if acpi_irq_create_hierarchy() is helping or hurting here. It obscures the fact that the only difference is the first argument to irq_domain_create_hierarchy(). If we could open-code or have a helper to figure out that irq_domain "parent" argument for the ACPI case, then we'd only have one call of irq_domain_create_hierarchy() here and it seems like it might be simpler. > 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 4179add2864b..2be4dd83b0e1 100644 > --- a/include/linux/hyperv.h > +++ b/include/linux/hyperv.h > @@ -1333,6 +1333,8 @@ static inline void *hv_get_drvdata(struct hv_device *dev) > return dev_get_drvdata(&dev->device); > } > > +struct device *hv_get_vmbus_root_device(void); > + > struct hv_ring_buffer_debug_info { > u32 current_interrupt_mask; > u32 current_read_index; > -- > 2.43.0 >
On 2/12/2025 9:42 AM, Bjorn Helgaas wrote: > On Tue, Feb 11, 2025 at 05:43:21PM -0800, Roman Kisel wrote: [...] >> + * function called later. > > The rest of this file fits in 80 columns; please wrap this to match. > Will fix, thank you for taking the time to review that! >> + */ >> + if (!domain) >> + WARN_ONCE(1, "No interrupt-parent found, check the DeviceTree data.\n"); > > Is there a way to include a hint about what specific part of the > devicetree to look at, e.g., the node that lacks a parent? I'll improve this, will mention the bus, thanks! [...] >> + * the messy ifdef below. > > Add a blank line if you intend a new paragraph here. Otherwise, wrap > to fill 78 columns or so. Will fix this, appreciate noticing that! > >> + * 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. > > And here. Will fix, thanks! >> + * None of these two cases reaches for the MSI parent domain. > > I don't know what "reaches for the MSI parent domain" means. Neither > "searches for"? > My bad, sorry about the incomprehensible phrasing! Will fix this, thank you! >> */ >> - 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 > > I don't know if acpi_irq_create_hierarchy() is helping or hurting > here. It obscures the fact that the only difference is the first > argument to irq_domain_create_hierarchy(). If we could open-code or > have a helper to figure out that irq_domain "parent" argument for the > ACPI case, then we'd only have one call of > irq_domain_create_hierarchy() here and it seems like it might be > simpler. > That looks quite dirty, no dispute over that... The root device was static/provate for the ACPI case, and I didn't go for changing the ACPI subsystem code to improve this patch, thought the only user wouldn't justify tinkering with the whole ACPI subsystem. Maybe I also will need to see if that can be used from a module/builti-in, locking, bogus usage, i.e. all that normally comes with promoting a private interface to public. Let me work out the details and post the change here to see what feedback that receives. Last but certainly not least: owing a great debt of gratitude to you (and all other folks) for helping in bringing this to the best shape possible!
From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, February 11, 2025 5:43 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@linux.microsoft.com> > --- > drivers/hv/vmbus_drv.c | 23 ++++++---- > drivers/pci/controller/pci-hyperv.c | 69 ++++++++++++++++++++++++++--- > include/linux/hyperv.h | 2 + > 3 files changed, 80 insertions(+), 14 deletions(-) > > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c > index 9d0c2dbd2a69..3f0f9f01b520 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 *hv_get_vmbus_root_device(void) > +{ > + return vmbus_root_device; > +} > +EXPORT_SYMBOL_GPL(hv_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; > } > > @@ -1920,7 +1927,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; > @@ -2282,7 +2289,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 > @@ -2373,7 +2380,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) > @@ -2692,7 +2699,7 @@ static int __init hv_acpi_init(void) > if (ret) > return ret; > > - if (!hv_dev) { > + if (!vmbus_root_device) { > ret = -ENODEV; > goto cleanup; > } > @@ -2723,7 +2730,7 @@ static int __init hv_acpi_init(void) > > cleanup: > platform_driver_unregister(&vmbus_platform_driver); > - hv_dev = NULL; > + vmbus_root_device = NULL; > return ret; > } These changes to rename hv_dev to vmbus_root_device, along with the introduction of hv_get_vmbus_root_device(), seem like a separate patch from the vPCI changes. The rename is definitely needed because "hv_dev" as a symbol is very overloaded. But the rename is "no functional change", and it doesn't touch the pci-hyperv.c file. You don't have a consumer for hv_get_vmbus_root_device() until the vPCI changes, but that seems OK to me to be in the subsequent patch. > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > index cdd5be16021d..24725bea9ef1 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> > > /* > @@ -817,9 +818,17 @@ static int hv_pci_vec_irq_gic_domain_alloc(struct irq_domain *domain, > int ret; > > fwspec.fwnode = domain->parent->fwnode; > - fwspec.param_count = 2; > - fwspec.param[0] = hwirq; > - fwspec.param[1] = IRQ_TYPE_EDGE_RISING; > + if (is_of_node(fwspec.fwnode)) { > + /* SPI lines for OF translations start at offset 32 */ > + fwspec.param_count = 3; > + fwspec.param[0] = 0; > + fwspec.param[1] = hwirq - 32; > + fwspec.param[2] = IRQ_TYPE_EDGE_RISING; > + } else { > + fwspec.param_count = 2; > + fwspec.param[0] = hwirq; > + fwspec.param[1] = IRQ_TYPE_EDGE_RISING; > + } > > ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec); > if (ret) > @@ -887,6 +896,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(hv_get_vmbus_root_device()->of_node); > + 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; > +} > + > +#endif > + > static int hv_pci_irqchip_init(void) > { > static struct hv_pci_chip_data *chip_data; > @@ -906,10 +944,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 4179add2864b..2be4dd83b0e1 100644 > --- a/include/linux/hyperv.h > +++ b/include/linux/hyperv.h > @@ -1333,6 +1333,8 @@ static inline void *hv_get_drvdata(struct hv_device *dev) > return dev_get_drvdata(&dev->device); > } > > +struct device *hv_get_vmbus_root_device(void); > + > struct hv_ring_buffer_debug_info { > u32 current_interrupt_mask; > u32 current_read_index; > -- > 2.43.0 >
On 2/12/2025 9:42 AM, Bjorn Helgaas wrote: > On Tue, Feb 11, 2025 at 05:43:21PM -0800, Roman Kisel wrote: [...] >> */ >> - 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 > > I don't know if acpi_irq_create_hierarchy() is helping or hurting > here. It obscures the fact that the only difference is the first > argument to irq_domain_create_hierarchy(). If we could open-code or > have a helper to figure out that irq_domain "parent" argument for the > ACPI case, then we'd only have one call of > irq_domain_create_hierarchy() here and it seems like it might be > simpler. > Hey Bjorn, folks, I've added few ACPI maintainers and the ACPI list as we're discussing making a small change to the ACPI subsystem to make one static variable available to make the code above less messy. Change [1] makes the GSI dispatcher function available to the outside world. Would you suggest going in that direction or there is a better approach to converge the code above that deals with IRQ domains both in the ACPI and DT cases? [1] From c6fb8bda21d6c00a308b1febc201a3a7e704c5a9 Mon Sep 17 00:00:00 2001 From: Roman Kisel <romank@linux.microsoft.com> Date: Wed, 19 Feb 2025 15:04:06 -0800 Subject: [PATCH] Refactor the ACPI GIC case --- drivers/acpi/irq.c | 14 ++++++- drivers/pci/controller/pci-hyperv.c | 62 +++++++++++++++++------------ include/linux/acpi.h | 5 ++- 3 files changed, 52 insertions(+), 29 deletions(-) diff --git a/drivers/acpi/irq.c b/drivers/acpi/irq.c index 1687483ff319..6243db610137 100644 --- a/drivers/acpi/irq.c +++ b/drivers/acpi/irq.c @@ -12,7 +12,7 @@ enum acpi_irq_model_id acpi_irq_model; -static struct fwnode_handle *(*acpi_get_gsi_domain_id)(u32 gsi); +static acpi_gsi_domain_disp_fn acpi_get_gsi_domain_id; static u32 (*acpi_gsi_to_irq_fallback)(u32 gsi); /** @@ -307,12 +307,22 @@ EXPORT_SYMBOL_GPL(acpi_irq_get); * for a given GSI */ void __init acpi_set_irq_model(enum acpi_irq_model_id model, - struct fwnode_handle *(*fn)(u32)) + acpi_gsi_domain_disp_fn fn) { acpi_irq_model = model; acpi_get_gsi_domain_id = fn; } +/** + * acpi_get_gsi_dispatcher - Returns dispatcher function that + * computes the domain fwnode for a + * given GSI. + */ +acpi_gsi_domain_disp_fn acpi_get_gsi_dispatcher(void) +{ + return acpi_get_gsi_domain_id; +} + /** * acpi_set_gsi_to_irq_fallback - Register a GSI transfer * callback to fallback to arch specified implementation. diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c index 24725bea9ef1..59e670e1cb6e 100644 --- a/drivers/pci/controller/pci-hyperv.c +++ b/drivers/pci/controller/pci-hyperv.c @@ -910,16 +910,29 @@ static struct irq_domain *hv_pci_of_irq_domain_parent(void) 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. - */ + return domain; +} + +#endif + +#ifdef CONFIG_ACPI + +static struct irq_domain *hv_pci_acpi_irq_domain_parent(void) +{ + struct irq_domain *domain; + acpi_gsi_domain_disp_fn gsi_domain_disp_fn; + + if (acpi_irq_model != ACPI_IRQ_MODEL_GIC) + return NULL; + gsi_domain_disp_fn = acpi_get_gsi_dispatcher(); + if (!gsi_domain_disp_fn) + return NULL; + domain = irq_find_matching_fwnode(gsi_domain_disp_fn(0), + DOMAIN_BUS_ANY); + if (!domain) - WARN_ONCE(1, "No interrupt-parent found, check the DeviceTree data.\n"); + return NULL; + return domain; } @@ -929,6 +942,7 @@ static int hv_pci_irqchip_init(void) { static struct hv_pci_chip_data *chip_data; struct fwnode_handle *fn = NULL; + struct irq_domain *irq_domain_parent = NULL; int ret = -ENOMEM; chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL); @@ -944,29 +958,25 @@ 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. */ #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); + irq_domain_parent = hv_pci_acpi_irq_domain_parent(); #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); + if (!irq_domain_parent) + irq_domain_parent = hv_pci_of_irq_domain_parent(); #endif + if (!irq_domain_parent) { + WARN_ONCE(1, "Invalid firmware configuration for VMBus interrupts\n"); + ret = -EINVAL; + goto free_chip; + } + + hv_msi_gic_irq_domain = irq_domain_create_hierarchy( + irq_domain_parent, 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 6adcd1b92b20..cd70a72c7073 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -336,8 +336,11 @@ int acpi_register_gsi (struct device *dev, u32 gsi, int triggering, int polarity int acpi_gsi_to_irq (u32 gsi, unsigned int *irq); int acpi_isa_irq_to_gsi (unsigned isa_irq, u32 *gsi); +typedef struct fwnode_handle *(*acpi_gsi_domain_disp_fn)(u32); + void acpi_set_irq_model(enum acpi_irq_model_id model, - struct fwnode_handle *(*)(u32)); + acpi_gsi_domain_disp_fn fn); +acpi_gsi_domain_disp_fn acpi_get_gsi_dispatcher(void); void acpi_set_gsi_to_irq_fallback(u32 (*)(u32)); struct irq_domain *acpi_irq_create_hierarchy(unsigned int flags,
On 2/19/2025 3:29 PM, Michael Kelley wrote: > From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, February 11, 2025 5:43 PM [...] >> } > > These changes to rename hv_dev to vmbus_root_device, along with the > introduction of hv_get_vmbus_root_device(), seem like a separate > patch from the vPCI changes. The rename is definitely needed because > "hv_dev" as a symbol is very overloaded. But the rename is "no functional > change", and it doesn't touch the pci-hyperv.c file. You don't have a > consumer for hv_get_vmbus_root_device() until the vPCI changes, but > that seems OK to me to be in the subsequent patch. > Thanks, will split the NFC out! I've asked the ACPI maintainers if a small change in ACPI would be fine to make the functional part of this patch more palatable, too.
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c index 9d0c2dbd2a69..3f0f9f01b520 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 *hv_get_vmbus_root_device(void) +{ + return vmbus_root_device; +} +EXPORT_SYMBOL_GPL(hv_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; } @@ -1920,7 +1927,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; @@ -2282,7 +2289,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 @@ -2373,7 +2380,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) @@ -2692,7 +2699,7 @@ static int __init hv_acpi_init(void) if (ret) return ret; - if (!hv_dev) { + if (!vmbus_root_device) { ret = -ENODEV; goto cleanup; } @@ -2723,7 +2730,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 cdd5be16021d..24725bea9ef1 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> /* @@ -817,9 +818,17 @@ static int hv_pci_vec_irq_gic_domain_alloc(struct irq_domain *domain, int ret; fwspec.fwnode = domain->parent->fwnode; - fwspec.param_count = 2; - fwspec.param[0] = hwirq; - fwspec.param[1] = IRQ_TYPE_EDGE_RISING; + if (is_of_node(fwspec.fwnode)) { + /* SPI lines for OF translations start at offset 32 */ + fwspec.param_count = 3; + fwspec.param[0] = 0; + fwspec.param[1] = hwirq - 32; + fwspec.param[2] = IRQ_TYPE_EDGE_RISING; + } else { + fwspec.param_count = 2; + fwspec.param[0] = hwirq; + fwspec.param[1] = IRQ_TYPE_EDGE_RISING; + } ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec); if (ret) @@ -887,6 +896,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(hv_get_vmbus_root_device()->of_node); + 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; +} + +#endif + static int hv_pci_irqchip_init(void) { static struct hv_pci_chip_data *chip_data; @@ -906,10 +944,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 4179add2864b..2be4dd83b0e1 100644 --- a/include/linux/hyperv.h +++ b/include/linux/hyperv.h @@ -1333,6 +1333,8 @@ static inline void *hv_get_drvdata(struct hv_device *dev) return dev_get_drvdata(&dev->device); } +struct device *hv_get_vmbus_root_device(void); + struct hv_ring_buffer_debug_info { u32 current_interrupt_mask; u32 current_read_index;
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@linux.microsoft.com> --- drivers/hv/vmbus_drv.c | 23 ++++++---- drivers/pci/controller/pci-hyperv.c | 69 ++++++++++++++++++++++++++--- include/linux/hyperv.h | 2 + 3 files changed, 80 insertions(+), 14 deletions(-)