| 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 >
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: > [...] > > +#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? Thanks for pointing this. I will fix this in next version. Moreover, although there is a similar flag in the ACPI flow, I am contemplating whether that also requires fixing. > > > + 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. Yes IORESOURCE_MEM_64 is required as there are cases where we are mapping 64 bit resources. But now I realize its better to fetch this info from range struct only (range.flags), as for_each_of_range populates this flag. I will fix this up as well. > > > + 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. 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 Regards, Saurabh > > 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 > > > >
From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Sunday, March 12, 2023 10:40 AM > > 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. Fair enough. I wasn't sure .... > > > > > 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. OK, I see your point about a different consistency, so this could go either way. :-) I tend to prefer getting rid of #ifdef's whenever possible. Since there's a valid argument either way, let's prefer the approach that eliminates the #ifdef. Michael > > 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 > > > > >
From: Saurabh Sengar <ssengar@linux.microsoft.com> Sent: Thursday, February 23, 2023 3:29 AM > > 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(-) > > 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. One further thing occurred to me. OF_EARLY_FLATTREE really depends on OF instead of ACPI. The ACPI dependency is indirect through OF. So I'd suggest doing select OF_EARLY_FLATTRE if OF to express the direct dependency. Separately, I wonder if the "select OF if !ACPI" is even needed. It doesn't hurt anything to leave it, but it seems like any config that doesn't independently select either ACPI or OF is broken for reasons unrelated to Hyper-V. I'm OK with leaving the select of OF if you want, so I'm more just wondering than asserting it should be removed. I didn't see "select OF if !ACPI" anywhere else in the Kconfig files, and it seems like Hyper-V would not be the only environment where this is the expectation. Michael
On Mon, Mar 13, 2023 at 02:33:53AM +0000, Michael Kelley (LINUX) wrote: > From: Saurabh Sengar <ssengar@linux.microsoft.com> Sent: Thursday, February 23, 2023 3:29 AM > > > > 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(-) > > > > 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. > > One further thing occurred to me. OF_EARLY_FLATTREE really depends > on OF instead of ACPI. The ACPI dependency is indirect through OF. So > I'd suggest doing > > select OF_EARLY_FLATTRE if OF > > to express the direct dependency. As you pointed out OF_EARLY_FLATTRE is anyway dependent on OF, and thus I feel this check is redundant. I see all the Kconfig options which enables both of these flags don't explicitly mention this dependency. > > Separately, I wonder if the "select OF if !ACPI" is even needed. It doesn't > hurt anything to leave it, but it seems like any config that doesn't > independently select either ACPI or OF is broken for reasons unrelated > to Hyper-V. I'm OK with leaving the select of OF if you want, so I'm > more just wondering than asserting it should be removed. I didn't > see "select OF if !ACPI" anywhere else in the Kconfig files, and it > seems like Hyper-V would not be the only environment where this > is the expectation. Ok I can remove the !ACPI dependency. Hope kernel size increase due to both the code compiled in shouldn't be problem for ACPI systems. And here if config doesn't select ACPI or OF it will assume OF, which is better then selecting none of them. To address both of your comments I feel below will be sufficient: select OF select OF_EARLY_FLATTRE Regards, Saurabh > > Michael
From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Sunday, March 12, 2023 11:16 PM > > On Mon, Mar 13, 2023 at 02:33:53AM +0000, Michael Kelley (LINUX) wrote: > > From: Saurabh Sengar <ssengar@linux.microsoft.com> Sent: Thursday, February 23, > 2023 3:29 AM > > > > > > 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(-) > > > > > > 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. > > > > One further thing occurred to me. OF_EARLY_FLATTREE really depends > > on OF instead of ACPI. The ACPI dependency is indirect through OF. So > > I'd suggest doing > > > > select OF_EARLY_FLATTRE if OF > > > > to express the direct dependency. > > As you pointed out OF_EARLY_FLATTRE is anyway dependent on OF, and thus I > feel this check is redundant. I see all the Kconfig options which enables > both of these flags don't explicitly mention this dependency. > > > > > Separately, I wonder if the "select OF if !ACPI" is even needed. It doesn't > > hurt anything to leave it, but it seems like any config that doesn't > > independently select either ACPI or OF is broken for reasons unrelated > > to Hyper-V. I'm OK with leaving the select of OF if you want, so I'm > > more just wondering than asserting it should be removed. I didn't > > see "select OF if !ACPI" anywhere else in the Kconfig files, and it > > seems like Hyper-V would not be the only environment where this > > is the expectation. > > Ok I can remove the !ACPI dependency. Hope kernel size increase due to both > the code compiled in shouldn't be problem for ACPI systems. > And here if config doesn't select ACPI or OF it will assume OF, which is > better then selecting none of them. > > > To address both of your comments I feel below will be sufficient: > select OF > select OF_EARLY_FLATTRE Actually, that's not what I was thinking. :-) I was thinking for the Hyper-V Kconfig to be silent on selecting OF, just like it is silent on selecting ACPI. Whoever is configuring the kernel build would separately be selecting ACPI, or OF, or both, depending on their needs. I don't think the Hyper-V Kconfig should always be selecting OF, because of the reason you noted -- it drags in code that is not needed for normal VTL 0 usage. If you take that approach, then select OF_EARLY_FLATTREE if OF is appropriate. Michael > > > Regards, > Saurabh > > > > > Michael
On Mon, Mar 13, 2023 at 12:26:56PM +0000, Michael Kelley (LINUX) wrote: > From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Sunday, March 12, 2023 11:16 PM > > > > On Mon, Mar 13, 2023 at 02:33:53AM +0000, Michael Kelley (LINUX) wrote: > > > From: Saurabh Sengar <ssengar@linux.microsoft.com> Sent: Thursday, February 23, > > 2023 3:29 AM > > > > > > > > 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(-) > > > > > > > > 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. > > > > > > One further thing occurred to me. OF_EARLY_FLATTREE really depends > > > on OF instead of ACPI. The ACPI dependency is indirect through OF. So > > > I'd suggest doing > > > > > > select OF_EARLY_FLATTRE if OF > > > > > > to express the direct dependency. > > > > As you pointed out OF_EARLY_FLATTRE is anyway dependent on OF, and thus I > > feel this check is redundant. I see all the Kconfig options which enables > > both of these flags don't explicitly mention this dependency. > > > > > > > > Separately, I wonder if the "select OF if !ACPI" is even needed. It doesn't > > > hurt anything to leave it, but it seems like any config that doesn't > > > independently select either ACPI or OF is broken for reasons unrelated > > > to Hyper-V. I'm OK with leaving the select of OF if you want, so I'm > > > more just wondering than asserting it should be removed. I didn't > > > see "select OF if !ACPI" anywhere else in the Kconfig files, and it > > > seems like Hyper-V would not be the only environment where this > > > is the expectation. > > > > Ok I can remove the !ACPI dependency. Hope kernel size increase due to both > > the code compiled in shouldn't be problem for ACPI systems. > > And here if config doesn't select ACPI or OF it will assume OF, which is > > better then selecting none of them. > > > > > > To address both of your comments I feel below will be sufficient: > > select OF > > select OF_EARLY_FLATTRE > > Actually, that's not what I was thinking. :-) I was thinking for the Hyper-V > Kconfig to be silent on selecting OF, just like it is silent on selecting ACPI. > Whoever is configuring the kernel build would separately be selecting > ACPI, or OF, or both, depending on their needs. I don't think the Hyper-V > Kconfig should always be selecting OF, because of the reason you noted -- > it drags in code that is not needed for normal VTL 0 usage. If you take > that approach, then > > select OF_EARLY_FLATTREE if OF > > is appropriate. Thanks for clarifying, I will fix this in next vesrion. Regards, Saurabh > > Michael > > > > > > > > > Regards, > > Saurabh > > > > > > > > Michael
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(-)