Re: [PATCH v3 7/7] PCI: hv: Get vPCI MSI IRQ domain from DT

[Roman Kisel <romank@xxxxxxxxxxxxxxxxxxx>]

On 8/5/2024 7:51 AM, Roman Kisel wrote:
> On 8/2/2024 6:20 PM, Wei Liu wrote:
> > On Fri, Jul 26, 2024 at 03:59:10PM -0700, 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@xxxxxxxxxxxxxxxxxxx>
> > > ---
> > >   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;
> >
> > You're changing the name of the variable. That should preferably be done
> > in a separate patch. That's going to make this patch shorter and easier
> > to review.
> Will fix in v4, thanks!
>
> > >   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);
> >
> > I would like you to add "hv_" prefix to this exported symbol, or rename
> > it to "vmbus_get_root_device()".
Will do in v4, missed this suggestion in my first reply.

> > > +
> > >   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);
> > > +    domain = NULL;
> > > +    if (parent) {
> > > +        domain = irq_find_host(parent);
> > > +        of_node_put(parent);
> > > +    }
> > > +
> >
> > I cannot really comment on the ARM side of things around how this system
> > is set up. I will leave that to someone who's more familiar with the
> > matter to review.
> >
> > Thanks,
> > Wei.

--
Thank you,
Roman