| Message ID | 1677151745-16521-6-git-send-email-ssengar@linux.microsoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | Device tree support for Hyper-V VMBus driver | expand |
On Thu, Feb 23, 2023 at 03:29:05AM -0800, Saurabh Sengar wrote: [...] > +#ifdef CONFIG_ACPI > static int vmbus_acpi_add(struct platform_device *pdev) > { > acpi_status result; > @@ -2494,10 +2497,47 @@ static int vmbus_acpi_add(struct platform_device *pdev) > vmbus_mmio_remove(); > return ret_val; > } > +#endif > + > +static int vmbus_device_add(struct platform_device *pdev) > +{ > + struct resource **cur_res = &hyperv_mmio; > + struct of_range range; > + struct of_range_parser parser; > + struct device_node *np = pdev->dev.of_node; > + int ret; > + > + hv_dev = &pdev->dev; > + > + ret = of_range_parser_init(&parser, np); > + if (ret) > + return ret; > + > + for_each_of_range(&parser, &range) { > + struct resource *res; > + > + res = kzalloc(sizeof(*res), GFP_ATOMIC); Why GFP_ATOMIC here? I don't think this function will be called in atomic context, right? > + if (!res) > + return -ENOMEM; > + > + res->name = "hyperv mmio"; > + res->flags = IORESOURCE_MEM | IORESOURCE_MEM_64; Are you sure IORESOURCE_MEM_64 is correct or required? The ACPI method does not set this flag. > + res->start = range.cpu_addr; > + res->end = range.cpu_addr + range.size; > + > + *cur_res = res; > + cur_res = &res->sibling; > + } > + > + return ret; > +} > > static int vmbus_platform_driver_probe(struct platform_device *pdev) > { > +#ifdef CONFIG_ACPI > return vmbus_acpi_add(pdev); > +#endif Please use #else here. > + return vmbus_device_add(pdev); Is there going to be a configuration that ACPI and OF are available at the same time? I don't see they are marked as mutually exclusive in the proposed KConfig. Thanks, Wei. > } > > static int vmbus_platform_driver_remove(struct platform_device *pdev) > @@ -2643,12 +2683,24 @@ static int vmbus_bus_resume(struct device *dev) > #define vmbus_bus_resume NULL > #endif /* CONFIG_PM_SLEEP */ > > +static const __maybe_unused struct of_device_id vmbus_of_match[] = { > + { > + .compatible = "microsoft,vmbus", > + }, > + { > + /* sentinel */ > + }, > +}; > +MODULE_DEVICE_TABLE(of, vmbus_of_match); > + > +#ifdef CONFIG_ACPI > static const struct acpi_device_id vmbus_acpi_device_ids[] = { > {"VMBUS", 0}, > {"VMBus", 0}, > {"", 0}, > }; > MODULE_DEVICE_TABLE(acpi, vmbus_acpi_device_ids); > +#endif > > /* > * Note: we must use the "no_irq" ops, otherwise hibernation can not work with > @@ -2677,6 +2729,7 @@ static struct platform_driver vmbus_platform_driver = { > .driver = { > .name = "vmbus", > .acpi_match_table = ACPI_PTR(vmbus_acpi_device_ids), > + .of_match_table = of_match_ptr(vmbus_of_match), > .pm = &vmbus_bus_pm, > .probe_type = PROBE_FORCE_SYNCHRONOUS, > } > -- > 2.34.1 >
From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Thursday, March 9, 2023 9:35 PM > > On Thu, Mar 09, 2023 at 09:16:25PM +0000, Wei Liu wrote: > > On Thu, Feb 23, 2023 at 03:29:05AM -0800, Saurabh Sengar wrote: [snip] > > > > > > static int vmbus_platform_driver_probe(struct platform_device *pdev) > > > { > > > +#ifdef CONFIG_ACPI > > > return vmbus_acpi_add(pdev); > > > +#endif > > > > Please use #else here. > > > > > + return vmbus_device_add(pdev); > > > > Is there going to be a configuration that ACPI and OF are available at > > the same time? I don't see they are marked as mutually exclusive in the > > proposed KConfig. > > Initially, the device tree functions was included in "#else" section after > the "#ifdef CONFIG_ACPI" section. However, it was subsequently removed to > increase the coverage for CI builds. > > Ref: https://lkml.org/lkml/2023/2/7/910 > I think the point here is that it is possible (and even likely on ARM64?) to build a kernel where CONFIG_ACPI and CONFIG_OF are both "Y". So the code for ACPI and OF is compiled and present in the kernel image. However, for a particular Linux boot on a particular hardware or virtual platform, only one of the two will be enabled. I specifically mention a particular Linux kernel boot because there's a kernel boot line option that can force disabling ACPI. Ideally, the VMBus code should work if both CONFIG_ACPI and CONFIG_OF are enabled in the kernel image, and it would determine at runtime which to use. This approach meets the goals Rob spells out. There's an exported global variable "acpi_disabled" that is set correctly depending on CONFIG_ACPI and the kernel boot line option (and perhaps if ACPI is not detected at runtime during boot -- I didn't check all the details). So the above could be written as: if (!acpi_disabled) return vmbus_acpi_add(pdev); else return vmbus_device_add(pdev); This avoids the weird "two return statements in a row" while preferring ACPI over OF if ACPI is enabled for a particular boot of Linux. I'm not sure if you'll need a stub for vmbus_acpi_add() when CONFIG_ACPI=n. In that case, acpi_disabled is #defined to be 1, so the compiler should just drop the call to vmbus_acpi_add() entirely and no stub will be needed. But you'll need to confirm. Also just confirming, it looks like vmbus_device_add() compiles correctly if CONFIG_OF=n. There are enough stubs in places so that you don't need an #ifdef CONFIG_OF around vmbus_device_add() like is needed for vmbus_acpi_add(). > > > > > > +static const __maybe_unused struct of_device_id vmbus_of_match[] = { > > > + { > > > + .compatible = "microsoft,vmbus", > > > + }, > > > + { > > > + /* sentinel */ > > > + }, > > > +}; > > > +MODULE_DEVICE_TABLE(of, vmbus_of_match); > > > + > > > +#ifdef CONFIG_ACPI > > > static const struct acpi_device_id vmbus_acpi_device_ids[] = { > > > {"VMBUS", 0}, > > > {"VMBus", 0}, > > > {"", 0}, > > > }; > > > MODULE_DEVICE_TABLE(acpi, vmbus_acpi_device_ids); > > > +#endif Couldn't the bracketing #ifdef be dropped and add __maybe_unused, just as you've done with vmbus_of_match? ACPI_PTR() is defined to return NULL if CONFIG_ACPI=n, just like with of_match_ptr() and CONFIG_OF. > > > > > > /* > > > * Note: we must use the "no_irq" ops, otherwise hibernation can not work with > > > @@ -2677,6 +2729,7 @@ static struct platform_driver vmbus_platform_driver = { > > > .driver = { > > > .name = "vmbus", > > > .acpi_match_table = ACPI_PTR(vmbus_acpi_device_ids), > > > + .of_match_table = of_match_ptr(vmbus_of_match), > > > .pm = &vmbus_bus_pm, > > > .probe_type = PROBE_FORCE_SYNCHRONOUS, > > > } > > > -- > > > 2.34.1 > > >
On Sun, Mar 12, 2023 at 01:08:02PM +0000, Michael Kelley (LINUX) wrote: > From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Thursday, March 9, 2023 9:35 PM > > > > On Thu, Mar 09, 2023 at 09:16:25PM +0000, Wei Liu wrote: > > > On Thu, Feb 23, 2023 at 03:29:05AM -0800, Saurabh Sengar wrote: > > [snip] > > > > > > > > > static int vmbus_platform_driver_probe(struct platform_device *pdev) > > > > { > > > > +#ifdef CONFIG_ACPI > > > > return vmbus_acpi_add(pdev); > > > > +#endif > > > > > > Please use #else here. > > > > > > > + return vmbus_device_add(pdev); > > > > > > Is there going to be a configuration that ACPI and OF are available at > > > the same time? I don't see they are marked as mutually exclusive in the > > > proposed KConfig. > > > > Initially, the device tree functions was included in "#else" section after > > the "#ifdef CONFIG_ACPI" section. However, it was subsequently removed to > > increase the coverage for CI builds. > > > > Ref: https://lkml.org/lkml/2023/2/7/910 > > > > I think the point here is that it is possible (and even likely on ARM64?) to > build a kernel where CONFIG_ACPI and CONFIG_OF are both "Y". So the > code for ACPI and OF is compiled and present in the kernel image. However, > for a particular Linux boot on a particular hardware or virtual platform, > only one of the two will be enabled. I specifically mention a particular Linux > kernel boot because there's a kernel boot line option that can force disabling > ACPI. Ideally, the VMBus code should work if both CONFIG_ACPI and > CONFIG_OF are enabled in the kernel image, and it would determine at > runtime which to use. This approach meets the goals Rob spells out. > > There's an exported global variable "acpi_disabled" that is set correctly > depending on CONFIG_ACPI and the kernel boot line option (and perhaps if > ACPI is not detected at runtime during boot -- I didn't check all the details). > So the above could be written as: > > if (!acpi_disabled) > return vmbus_acpi_add(pdev); > else > return vmbus_device_add(pdev); > > This avoids the weird "two return statements in a row" while preferring > ACPI over OF if ACPI is enabled for a particular boot of Linux. > > I'm not sure if you'll need a stub for vmbus_acpi_add() when CONFIG_ACPI=n. > In that case, acpi_disabled is #defined to be 1, so the compiler should just > drop the call to vmbus_acpi_add() entirely and no stub will be needed. But > you'll need to confirm. Thanks for suggesting acpi_disabled, definitely this looks better. However, we need a dummy stub for vmbus_acpi_add in case of CONFIG_ACPI=n, as compiler doesn't take out vmbus_acpi_add reference completely for CONFIG_ACPI=n. > > Also just confirming, it looks like vmbus_device_add() compiles correctly if > CONFIG_OF=n. There are enough stubs in places so that you don't need an > #ifdef CONFIG_OF around vmbus_device_add() like is needed for > vmbus_acpi_add(). Yes, I tested this scenario. > > > > > > > > > +static const __maybe_unused struct of_device_id vmbus_of_match[] = { > > > > + { > > > > + .compatible = "microsoft,vmbus", > > > > + }, > > > > + { > > > > + /* sentinel */ > > > > + }, > > > > +}; > > > > +MODULE_DEVICE_TABLE(of, vmbus_of_match); > > > > + > > > > +#ifdef CONFIG_ACPI > > > > static const struct acpi_device_id vmbus_acpi_device_ids[] = { > > > > {"VMBUS", 0}, > > > > {"VMBus", 0}, > > > > {"", 0}, > > > > }; > > > > MODULE_DEVICE_TABLE(acpi, vmbus_acpi_device_ids); > > > > +#endif > > Couldn't the bracketing #ifdef be dropped and add __maybe_unused, just > as you've done with vmbus_of_match? ACPI_PTR() is defined to return NULL > if CONFIG_ACPI=n, just like with of_match_ptr() and CONFIG_OF. I kept #ifdef so that all the acpi code is treated equally. However, I am fine to use __maybe_unused, will fix this in next version. Regards, Saurabh > > > > > > > > > /* > > > > * Note: we must use the "no_irq" ops, otherwise hibernation can not work with > > > > @@ -2677,6 +2729,7 @@ static struct platform_driver vmbus_platform_driver = { > > > > .driver = { > > > > .name = "vmbus", > > > > .acpi_match_table = ACPI_PTR(vmbus_acpi_device_ids), > > > > + .of_match_table = of_match_ptr(vmbus_of_match), > > > > .pm = &vmbus_bus_pm, > > > > .probe_type = PROBE_FORCE_SYNCHRONOUS, > > > > } > > > > -- > > > > 2.34.1 > > > >
diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig index 0747a8f1fcee..1a55bf32d195 100644 --- a/drivers/hv/Kconfig +++ b/drivers/hv/Kconfig @@ -4,11 +4,13 @@ menu "Microsoft Hyper-V guest support" config HYPERV tristate "Microsoft Hyper-V client drivers" - depends on ACPI && ((X86 && X86_LOCAL_APIC && HYPERVISOR_GUEST) \ - || (ARM64 && !CPU_BIG_ENDIAN)) + depends on (X86 && X86_LOCAL_APIC && HYPERVISOR_GUEST) \ + || (ACPI && ARM64 && !CPU_BIG_ENDIAN) select PARAVIRT select X86_HV_CALLBACK_VECTOR if X86 select VMAP_PFN + select OF if !ACPI + select OF_EARLY_FLATTREE if !ACPI help Select this option to run Linux as a Hyper-V client operating system. diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c index 73497157a23a..e370721f334e 100644 --- a/drivers/hv/vmbus_drv.c +++ b/drivers/hv/vmbus_drv.c @@ -20,6 +20,7 @@ #include <linux/completion.h> #include <linux/hyperv.h> #include <linux/kernel_stat.h> +#include <linux/of_address.h> #include <linux/clockchips.h> #include <linux/cpu.h> #include <linux/sched/isolation.h> @@ -2152,7 +2153,7 @@ void vmbus_device_unregister(struct hv_device *device_obj) device_unregister(&device_obj->device); } - +#ifdef CONFIG_ACPI /* * VMBUS is an acpi enumerated device. Get the information we * need from DSDT. @@ -2262,6 +2263,7 @@ static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx) return AE_OK; } +#endif static void vmbus_mmio_remove(void) { @@ -2282,7 +2284,7 @@ static void vmbus_mmio_remove(void) } } -static void vmbus_reserve_fb(void) +static void __maybe_unused vmbus_reserve_fb(void) { resource_size_t start = 0, size; struct pci_dev *pdev; @@ -2442,6 +2444,7 @@ void vmbus_free_mmio(resource_size_t start, resource_size_t size) } EXPORT_SYMBOL_GPL(vmbus_free_mmio); +#ifdef CONFIG_ACPI static int vmbus_acpi_add(struct platform_device *pdev) { acpi_status result; @@ -2494,10 +2497,47 @@ static int vmbus_acpi_add(struct platform_device *pdev) vmbus_mmio_remove(); return ret_val; } +#endif + +static int vmbus_device_add(struct platform_device *pdev) +{ + struct resource **cur_res = &hyperv_mmio; + struct of_range range; + struct of_range_parser parser; + struct device_node *np = pdev->dev.of_node; + int ret; + + hv_dev = &pdev->dev; + + ret = of_range_parser_init(&parser, np); + if (ret) + return ret; + + for_each_of_range(&parser, &range) { + struct resource *res; + + res = kzalloc(sizeof(*res), GFP_ATOMIC); + if (!res) + return -ENOMEM; + + res->name = "hyperv mmio"; + res->flags = IORESOURCE_MEM | IORESOURCE_MEM_64; + res->start = range.cpu_addr; + res->end = range.cpu_addr + range.size; + + *cur_res = res; + cur_res = &res->sibling; + } + + return ret; +} static int vmbus_platform_driver_probe(struct platform_device *pdev) { +#ifdef CONFIG_ACPI return vmbus_acpi_add(pdev); +#endif + return vmbus_device_add(pdev); } static int vmbus_platform_driver_remove(struct platform_device *pdev) @@ -2643,12 +2683,24 @@ static int vmbus_bus_resume(struct device *dev) #define vmbus_bus_resume NULL #endif /* CONFIG_PM_SLEEP */ +static const __maybe_unused struct of_device_id vmbus_of_match[] = { + { + .compatible = "microsoft,vmbus", + }, + { + /* sentinel */ + }, +}; +MODULE_DEVICE_TABLE(of, vmbus_of_match); + +#ifdef CONFIG_ACPI static const struct acpi_device_id vmbus_acpi_device_ids[] = { {"VMBUS", 0}, {"VMBus", 0}, {"", 0}, }; MODULE_DEVICE_TABLE(acpi, vmbus_acpi_device_ids); +#endif /* * Note: we must use the "no_irq" ops, otherwise hibernation can not work with @@ -2677,6 +2729,7 @@ static struct platform_driver vmbus_platform_driver = { .driver = { .name = "vmbus", .acpi_match_table = ACPI_PTR(vmbus_acpi_device_ids), + .of_match_table = of_match_ptr(vmbus_of_match), .pm = &vmbus_bus_pm, .probe_type = PROBE_FORCE_SYNCHRONOUS, }
Update the driver to support Devicetree boot as well along with ACPI. At present the Devicetree parsing only provides the mmio region info and is not the exact copy of ACPI parsing. This is sufficient to cater all the current Devicetree usecases for VMBus. Currently Devicetree is supported only for x86 systems. Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com> --- [V7] - Use cpu_addr instead of bus_addr drivers/hv/Kconfig | 6 +++-- drivers/hv/vmbus_drv.c | 57 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 59 insertions(+), 4 deletions(-)