[PATCH v5 0/5] Device tree support for Hyper-V VMBus driver

[PATCH v5 0/5] Device tree support for Hyper-V VMBus driver

[Saurabh Sengar]

This set of patches expands the VMBus driver to include device tree
support. This feature allows for a kernel boot without the use of ACPI
tables, resulting in a smaller memory footprint and potentially faster
boot times. This is tested by enabling CONFIG_FLAT and OF_EARLY_FLATTREE
for x86.

The first two patches enable compilation of Hyper-V APIs in a non-ACPI
build.

The third patch converts the VMBus driver from acpi to more generic
platform driver.

Further to add device tree documentation for VMBus, it needs to club with
other virtualization driver's documentation. For this rename the virtio
folder to more generic hypervisor, so that all the hypervisor based
devices can co-exist in a single place in device tree documentation. The
fourth patch does this renaming.

The fifth patch introduces the device tree documentation for VMBus.

The sixth patch adds device tree support to the VMBus driver. Currently
this is tested only for x86 and it may not work for other archs.

[V5]
- Removed #else for device tree parsing code. This should help better
  test coverage.
- Fix macro '__maybe_unused' warning
- Added below options in Kconfig to enable device tree options for HYPERV
	select OF if !ACPI
	select OF_EARLY_FLATTREE if !ACPI
- moved dt documantation to bus folder
- update the dt node to have 'bus' as parent node instead of 'soc'. Also
  added compatible and ranges property for parent node.
- Made sure dt_binding_check have no error/varnings for microsoft,vmbus.yaml file
- Fix commit messages and add Reviwed-by from Michael for first 3 patches

[V4]
- rebased which fixed return type of 'vmbus_mmio_remove' from int to void
- used __maybe_unused for 'vmbus_of_match' and safeguard vmbus_acpi_device_ids
  under #ifdef

[V3]
- Changed the logic to use generic api (for_each_of_range) for parsing "ranges".
- Remove dependency of ACPI for HYPERV in case of x86.
- Removed "device tree bindings" from title and patch subject.
- Removed duplicate vendor prefix, used microsoft instead of msft.
- Use 'soc' in example of device tree documantation for parent node.
- Fixed compatible schemas error generated in other modules referring to
  virtio.
- Drop hex notation and leading zeros from device tree cell properties.

Saurabh Sengar (5):
  drivers/clocksource/hyper-v: non ACPI support in hyperv clock
  Drivers: hv: allow non ACPI compilation for
    hv_is_hibernation_supported
  Drivers: hv: vmbus: Convert acpi_device to more generic
    platform_device
  dt-bindings: hypervisor: VMBus
  Driver: VMBus: Add device tree support

 .../bindings/bus/microsoft,vmbus.yaml         |  50 ++++++++
 MAINTAINERS                                   |   1 +
 drivers/clocksource/hyperv_timer.c            |  15 ++-
 drivers/hv/Kconfig                            |   6 +-
 drivers/hv/hv_common.c                        |   4 +
 drivers/hv/vmbus_drv.c                        | 118 ++++++++++++++----
 6 files changed, 165 insertions(+), 29 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml

-- 
2.34.1

[PATCH v5 1/5] drivers/clocksource/hyper-v: non ACPI support in hyperv clock

[Saurabh Sengar]

Add a placeholder function for the hv_setup_stimer0_irq API to accommodate
systems without ACPI support. Since this function is not utilized on
x86/x64 systems and non-ACPI support is only intended for x86/x64 systems,
a placeholder function is sufficient for now and can be improved upon if
necessary in the future.

Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
---
 drivers/clocksource/hyperv_timer.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
index c0cef92b12b8..f32948c8a96f 100644
--- a/drivers/clocksource/hyperv_timer.c
+++ b/drivers/clocksource/hyperv_timer.c
@@ -49,7 +49,7 @@ static bool direct_mode_enabled;
 
 static int stimer0_irq = -1;
 static int stimer0_message_sint;
-static DEFINE_PER_CPU(long, stimer0_evt);
+static __maybe_unused DEFINE_PER_CPU(long, stimer0_evt);
 
 /*
  * Common code for stimer0 interrupts coming via Direct Mode or
@@ -68,7 +68,7 @@ EXPORT_SYMBOL_GPL(hv_stimer0_isr);
  * stimer0 interrupt handler for architectures that support
  * per-cpu interrupts, which also implies Direct Mode.
  */
-static irqreturn_t hv_stimer0_percpu_isr(int irq, void *dev_id)
+static irqreturn_t __maybe_unused hv_stimer0_percpu_isr(int irq, void *dev_id)
 {
 	hv_stimer0_isr();
 	return IRQ_HANDLED;
@@ -196,6 +196,7 @@ void __weak hv_remove_stimer0_handler(void)
 {
 };
 
+#ifdef CONFIG_ACPI
 /* Called only on architectures with per-cpu IRQs (i.e., not x86/x64) */
 static int hv_setup_stimer0_irq(void)
 {
@@ -230,6 +231,16 @@ static void hv_remove_stimer0_irq(void)
 		stimer0_irq = -1;
 	}
 }
+#else
+static int hv_setup_stimer0_irq(void)
+{
+	return 0;
+}
+
+static void hv_remove_stimer0_irq(void)
+{
+}
+#endif
 
 /* hv_stimer_alloc - Global initialization of the clockevent and stimer0 */
 int hv_stimer_alloc(bool have_percpu_irqs)
-- 
2.34.1

[PATCH v5 2/5] Drivers: hv: allow non ACPI compilation for hv_is_hibernation_supported

[Saurabh Sengar]

acpi_sleep_state_supported() currently is defined only when
CONFIG_ACPI=y.  For future work to enable device tree builds, put this
function under #ifdef CONFIG_ACPI.  Otherwise, return 'false' from
hv_is_hibernation_supported() as Hyper-V guest configs using device
tree don't support hibernation.

Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
---
 drivers/hv/hv_common.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
index 52a6f89ccdbd..370ec20d1993 100644
--- a/drivers/hv/hv_common.c
+++ b/drivers/hv/hv_common.c
@@ -234,7 +234,11 @@ EXPORT_SYMBOL_GPL(hv_setup_dma_ops);
 
 bool hv_is_hibernation_supported(void)
 {
+#ifdef CONFIG_ACPI
 	return !hv_root_partition && acpi_sleep_state_supported(ACPI_STATE_S4);
+#else
+	return false;
+#endif
 }
 EXPORT_SYMBOL_GPL(hv_is_hibernation_supported);
 
-- 
2.34.1

[PATCH v5 3/5] Drivers: hv: vmbus: Convert acpi_device to more generic platform_device

[Saurabh Sengar]

VMBus driver code currently has direct dependency on ACPI and struct
acpi_device.  As a staging step toward optionally configuring based on
device tree instead of ACPI, use a more generic platform device to reduce
the dependency on ACPI where possible, though the dependency on ACPI
is not completely removed.  Also rename the function vmbus_acpi_remove()
to the more generic vmbus_mmio_remove().

Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
---
 drivers/hv/vmbus_drv.c | 58 +++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 23 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index d24dd65b33d4..73497157a23a 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -12,6 +12,7 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/device.h>
+#include <linux/platform_device.h>
 #include <linux/interrupt.h>
 #include <linux/sysctl.h>
 #include <linux/slab.h>
@@ -44,7 +45,7 @@ struct vmbus_dynid {
 	struct hv_vmbus_device_id id;
 };
 
-static struct acpi_device  *hv_acpi_dev;
+static struct device  *hv_dev;
 
 static int hyperv_cpuhp_online;
 
@@ -143,7 +144,7 @@ static DEFINE_MUTEX(hyperv_mmio_lock);
 
 static int vmbus_exists(void)
 {
-	if (hv_acpi_dev == NULL)
+	if (hv_dev == NULL)
 		return -ENODEV;
 
 	return 0;
@@ -932,7 +933,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_acpi_dev->dev) == DEV_DMA_COHERENT);
+		device_get_dma_attr(hv_dev) == DEV_DMA_COHERENT);
 	return 0;
 }
 
@@ -2090,7 +2091,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_acpi_dev->dev;
+	child_device_obj->device.parent = hv_dev;
 	child_device_obj->device.release = vmbus_device_release;
 
 	child_device_obj->device.dma_parms = &child_device_obj->dma_parms;
@@ -2262,7 +2263,7 @@ static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx)
 	return AE_OK;
 }
 
-static void vmbus_acpi_remove(struct acpi_device *device)
+static void vmbus_mmio_remove(void)
 {
 	struct resource *cur_res;
 	struct resource *next_res;
@@ -2441,13 +2442,14 @@ void vmbus_free_mmio(resource_size_t start, resource_size_t size)
 }
 EXPORT_SYMBOL_GPL(vmbus_free_mmio);
 
-static int vmbus_acpi_add(struct acpi_device *device)
+static int vmbus_acpi_add(struct platform_device *pdev)
 {
 	acpi_status result;
 	int ret_val = -ENODEV;
 	struct acpi_device *ancestor;
+	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
 
-	hv_acpi_dev = device;
+	hv_dev = &device->dev;
 
 	/*
 	 * Older versions of Hyper-V for ARM64 fail to include the _CCA
@@ -2489,10 +2491,21 @@ static int vmbus_acpi_add(struct acpi_device *device)
 
 acpi_walk_err:
 	if (ret_val)
-		vmbus_acpi_remove(device);
+		vmbus_mmio_remove();
 	return ret_val;
 }
 
+static int vmbus_platform_driver_probe(struct platform_device *pdev)
+{
+	return vmbus_acpi_add(pdev);
+}
+
+static int vmbus_platform_driver_remove(struct platform_device *pdev)
+{
+	vmbus_mmio_remove();
+	return 0;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int vmbus_bus_suspend(struct device *dev)
 {
@@ -2658,15 +2671,15 @@ static const struct dev_pm_ops vmbus_bus_pm = {
 	.restore_noirq	= vmbus_bus_resume
 };
 
-static struct acpi_driver vmbus_acpi_driver = {
-	.name = "vmbus",
-	.ids = vmbus_acpi_device_ids,
-	.ops = {
-		.add = vmbus_acpi_add,
-		.remove = vmbus_acpi_remove,
-	},
-	.drv.pm = &vmbus_bus_pm,
-	.drv.probe_type = PROBE_FORCE_SYNCHRONOUS,
+static struct platform_driver vmbus_platform_driver = {
+	.probe = vmbus_platform_driver_probe,
+	.remove = vmbus_platform_driver_remove,
+	.driver = {
+		.name = "vmbus",
+		.acpi_match_table = ACPI_PTR(vmbus_acpi_device_ids),
+		.pm = &vmbus_bus_pm,
+		.probe_type = PROBE_FORCE_SYNCHRONOUS,
+	}
 };
 
 static void hv_kexec_handler(void)
@@ -2750,12 +2763,11 @@ static int __init hv_acpi_init(void)
 	/*
 	 * Get ACPI resources first.
 	 */
-	ret = acpi_bus_register_driver(&vmbus_acpi_driver);
-
+	ret = platform_driver_register(&vmbus_platform_driver);
 	if (ret)
 		return ret;
 
-	if (!hv_acpi_dev) {
+	if (!hv_dev) {
 		ret = -ENODEV;
 		goto cleanup;
 	}
@@ -2785,8 +2797,8 @@ static int __init hv_acpi_init(void)
 	return 0;
 
 cleanup:
-	acpi_bus_unregister_driver(&vmbus_acpi_driver);
-	hv_acpi_dev = NULL;
+	platform_driver_unregister(&vmbus_platform_driver);
+	hv_dev = NULL;
 	return ret;
 }
 
@@ -2839,7 +2851,7 @@ static void __exit vmbus_exit(void)
 
 	cpuhp_remove_state(hyperv_cpuhp_online);
 	hv_synic_free();
-	acpi_bus_unregister_driver(&vmbus_acpi_driver);
+	platform_driver_unregister(&vmbus_platform_driver);
 }
 
 
-- 
2.34.1

[PATCH v5 4/5] dt-bindings: hypervisor: VMBus

[Saurabh Sengar]

Add dt-bindings for Hyper-V VMBus.

Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
---
[V5]
- moved dt documantation to bus folder
- update the dt node to have 'bus' as parent node instead of 'soc'. Also
  added compatible and ranges property for parent node.
- Made sure dt_binding_check have no error/varnings for microsoft,vmbus.yaml file

 .../bindings/bus/microsoft,vmbus.yaml         | 50 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml

diff --git a/Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml b/Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml
new file mode 100644
index 000000000000..40daefc37615
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/bus/microsoft,vmbus.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microsoft Hyper-V VMBus
+
+maintainers:
+  - Saurabh Sengar <ssengar@linux.microsoft.com>
+
+description:
+  VMBus is a software bus that implement the protocols for communication
+  between the root or host OS and guest OSs (virtual machines).
+
+properties:
+  compatible:
+    const: microsoft,vmbus
+
+  ranges: true
+
+  '#address-cells':
+    const: 2
+
+  '#size-cells':
+    const: 1
+
+required:
+  - compatible
+  - ranges
+  - '#address-cells'
+  - '#size-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    bus@ff0000000 {
+        compatible = "simple-bus";
+        #address-cells = <2>;
+        #size-cells = <1>;
+        ranges = <0x0 0xf 0xf0000000 0x10000000>;
+
+        vmbus@ff0000000 {
+            compatible = "microsoft,vmbus";
+            #address-cells = <2>;
+            #size-cells = <1>;
+            ranges = <0x0f 0xf0000000 0x0f 0xf0000000 0x10000000>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index df202a1e4b96..67ddecd1a383 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9490,6 +9490,7 @@ S:	Supported
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git
 F:	Documentation/ABI/stable/sysfs-bus-vmbus
 F:	Documentation/ABI/testing/debugfs-hyperv
+F:	Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml
 F:	Documentation/virt/hyperv
 F:	Documentation/networking/device_drivers/ethernet/microsoft/netvsc.rst
 F:	arch/arm64/hyperv
-- 
2.34.1

[PATCH v5 5/5] Driver: VMBus: Add device tree support

[Saurabh Sengar]

Update the driver to support device tree boot as well along with ACPI.
At present the device tree parsing only provides the mmio region info
and is not the exact copy of ACPI parsing. This is sufficient to cater
all the current device tree usecases for VMBus.

Currently device tree is supported only for x86 systems.

Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
---
- Removed #else for device tree parsing code. This should help better
  test coverage.
- Fix macro '__maybe_unused' warning
- Added below options in Kconfig to enable device tree options for HYPERV
	select OF if !ACPI
	select OF_EARLY_FLATTREE if !ACPI

 drivers/hv/Kconfig     |  6 +++--
 drivers/hv/vmbus_drv.c | 60 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 62 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.
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 73497157a23a..02f6bab61c37 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,50 @@ 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;
+	int ret = 0;
+
+	hv_dev = &pdev->dev;
+	np = pdev->dev.of_node;
+
+	ret = of_range_parser_init(&parser, np);
+	if (ret) {
+		dev_err(hv_dev, "Failed to parse resources.\n");
+		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.pci_addr;
+		res->end = range.pci_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 +2686,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 +2732,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

Re: [PATCH v5 2/5] Drivers: hv: allow non ACPI compilation for hv_is_hibernation_supported

[Rob Herring]

On Thu, Feb 9, 2023 at 6:15 AM Saurabh Sengar
<ssengar@linux.microsoft.com> wrote:
>
> acpi_sleep_state_supported() currently is defined only when
> CONFIG_ACPI=y.  For future work to enable device tree builds, put this
> function under #ifdef CONFIG_ACPI.  Otherwise, return 'false' from
> hv_is_hibernation_supported() as Hyper-V guest configs using device
> tree don't support hibernation.
>
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> ---
>  drivers/hv/hv_common.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index 52a6f89ccdbd..370ec20d1993 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -234,7 +234,11 @@ EXPORT_SYMBOL_GPL(hv_setup_dma_ops);
>
>  bool hv_is_hibernation_supported(void)
>  {
> +#ifdef CONFIG_ACPI
>         return !hv_root_partition && acpi_sleep_state_supported(ACPI_STATE_S4);

Rework the acpi_bus.h header so that this is defined for !CONFIG_ACPI:

static inline bool acpi_sleep_state_supported(u8 sleep_state) { return false; }

Then you don't need this change here. That or using IS_ENABLED() is
strongly preferred over #ifdefs.

Rob

Re: [PATCH v5 5/5] Driver: VMBus: Add device tree support

[Rob Herring]

On Thu, Feb 9, 2023 at 6:15 AM Saurabh Sengar
<ssengar@linux.microsoft.com> wrote:
>
> Update the driver to support device tree boot as well along with ACPI.

Devicetree

> At present the device tree parsing only provides the mmio region info

Devicetree

And anywhere else.

> and is not the exact copy of ACPI parsing. This is sufficient to cater
> all the current device tree usecases for VMBus.
>
> Currently device tree is supported only for x86 systems.
>
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
> - Removed #else for device tree parsing code. This should help better
>   test coverage.
> - Fix macro '__maybe_unused' warning
> - Added below options in Kconfig to enable device tree options for HYPERV
>         select OF if !ACPI
>         select OF_EARLY_FLATTREE if !ACPI
>
>  drivers/hv/Kconfig     |  6 +++--
>  drivers/hv/vmbus_drv.c | 60 ++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 62 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.
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 73497157a23a..02f6bab61c37 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,50 @@ 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;
> +       int ret = 0;

No need to initialize.

> +
> +       hv_dev = &pdev->dev;
> +       np = pdev->dev.of_node;

Set this on the declaration.

> +
> +       ret = of_range_parser_init(&parser, np);
> +       if (ret) {
> +               dev_err(hv_dev, "Failed to parse resources.\n");

If a print is needed, put it in of_range_parser_init().

> +               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.pci_addr;

This is not PCI. It's a union, so use 'bus_addr' instead.

But wait, resources and IORESOURCE_MEM are *CPU* addresses. You need
cpu_addr here. Your DT happens to do 1:1 addresses so it happens to
work either way.

Rob

Re: [PATCH v5 2/5] Drivers: hv: allow non ACPI compilation for hv_is_hibernation_supported

[Saurabh Singh Sengar]

On Thu, Feb 09, 2023 at 09:17:45AM -0600, Rob Herring wrote:
> On Thu, Feb 9, 2023 at 6:15 AM Saurabh Sengar
> <ssengar@linux.microsoft.com> wrote:
> >
> > acpi_sleep_state_supported() currently is defined only when
> > CONFIG_ACPI=y.  For future work to enable device tree builds, put this
> > function under #ifdef CONFIG_ACPI.  Otherwise, return 'false' from
> > hv_is_hibernation_supported() as Hyper-V guest configs using device
> > tree don't support hibernation.
> >
> > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> > ---
> >  drivers/hv/hv_common.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> > index 52a6f89ccdbd..370ec20d1993 100644
> > --- a/drivers/hv/hv_common.c
> > +++ b/drivers/hv/hv_common.c
> > @@ -234,7 +234,11 @@ EXPORT_SYMBOL_GPL(hv_setup_dma_ops);
> >
> >  bool hv_is_hibernation_supported(void)
> >  {
> > +#ifdef CONFIG_ACPI
> >         return !hv_root_partition && acpi_sleep_state_supported(ACPI_STATE_S4);
> 
> Rework the acpi_bus.h header so that this is defined for !CONFIG_ACPI:
> 
> static inline bool acpi_sleep_state_supported(u8 sleep_state) { return false; }

Sure,
acpi_bus.h doesn't get included in case of !ACPI, I will put this in include/linux/acpi.h

Regards,
Saurabh

> 
> Then you don't need this change here. That or using IS_ENABLED() is
> strongly preferred over #ifdefs.
> 
> Rob

Re: [PATCH v5 5/5] Driver: VMBus: Add device tree support

[Saurabh Singh Sengar]

On Thu, Feb 09, 2023 at 09:50:31AM -0600, Rob Herring wrote:
> On Thu, Feb 9, 2023 at 6:15 AM Saurabh Sengar
> <ssengar@linux.microsoft.com> wrote:
> >
> > Update the driver to support device tree boot as well along with ACPI.
> 
> Devicetree
> 
> > At present the device tree parsing only provides the mmio region info
> 
> Devicetree
> 
> And anywhere else.

OK

> 
> > and is not the exact copy of ACPI parsing. This is sufficient to cater
> > all the current device tree usecases for VMBus.
> >
> > Currently device tree is supported only for x86 systems.
> >
> > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > ---
> > - Removed #else for device tree parsing code. This should help better
> >   test coverage.
> > - Fix macro '__maybe_unused' warning
> > - Added below options in Kconfig to enable device tree options for HYPERV
> >         select OF if !ACPI
> >         select OF_EARLY_FLATTREE if !ACPI
> >
> >  drivers/hv/Kconfig     |  6 +++--
> >  drivers/hv/vmbus_drv.c | 60 ++++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 62 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.
> > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > index 73497157a23a..02f6bab61c37 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,50 @@ 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;
> > +       int ret = 0;
> 
> No need to initialize.

OK

> 
> > +
> > +       hv_dev = &pdev->dev;
> > +       np = pdev->dev.of_node;
> 
> Set this on the declaration.

OK

> 
> > +
> > +       ret = of_range_parser_init(&parser, np);
> > +       if (ret) {
> > +               dev_err(hv_dev, "Failed to parse resources.\n");
> 
> If a print is needed, put it in of_range_parser_init().

Will remove the print

> 
> > +               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.pci_addr;
> 
> This is not PCI. It's a union, so use 'bus_addr' instead.
> 
> But wait, resources and IORESOURCE_MEM are *CPU* addresses. You need
> cpu_addr here. Your DT happens to do 1:1 addresses so it happens to
> work either way.

bus_addr works for us, will send V6

Regards,
Saurabh

> 
> Rob

Re: [PATCH v5 5/5] Driver: VMBus: Add device tree support

[Rob Herring]

On Thu, Feb 9, 2023 at 11:46 AM Saurabh Singh Sengar
<ssengar@linux.microsoft.com> wrote:
>
> On Thu, Feb 09, 2023 at 09:50:31AM -0600, Rob Herring wrote:
> > On Thu, Feb 9, 2023 at 6:15 AM Saurabh Sengar
> > <ssengar@linux.microsoft.com> wrote:
> > >
> > > Update the driver to support device tree boot as well along with ACPI.
> >
> > Devicetree

[...]

> > > +       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.pci_addr;
> >
> > This is not PCI. It's a union, so use 'bus_addr' instead.
> >
> > But wait, resources and IORESOURCE_MEM are *CPU* addresses. You need
> > cpu_addr here. Your DT happens to do 1:1 addresses so it happens to
> > work either way.
>
> bus_addr works for us, will send V6

Sigh. bus_addr may work, but is wrong as I explained.

And you've already sent v6... Please slow down your pace with sending
new versions. 4 versions in a week is too much. Give others time to
comment and me to respond to discussions. Like a week...

Rob

Re: [PATCH v5 5/5] Driver: VMBus: Add device tree support

[Saurabh Singh Sengar]

On Fri, Feb 10, 2023 at 04:37:28PM -0600, Rob Herring wrote:
> On Thu, Feb 9, 2023 at 11:46 AM Saurabh Singh Sengar
> <ssengar@linux.microsoft.com> wrote:
> >
> > On Thu, Feb 09, 2023 at 09:50:31AM -0600, Rob Herring wrote:
> > > On Thu, Feb 9, 2023 at 6:15 AM Saurabh Sengar
> > > <ssengar@linux.microsoft.com> wrote:
> > > >
> > > > Update the driver to support device tree boot as well along with ACPI.
> > >
> > > Devicetree
> 
> [...]
> 
> > > > +       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.pci_addr;
> > >
> > > This is not PCI. It's a union, so use 'bus_addr' instead.
> > >
> > > But wait, resources and IORESOURCE_MEM are *CPU* addresses. You need
> > > cpu_addr here. Your DT happens to do 1:1 addresses so it happens to
> > > work either way.
> >
> > bus_addr works for us, will send V6
> 
> Sigh. bus_addr may work, but is wrong as I explained.
> 
> And you've already sent v6... Please slow down your pace with sending
> new versions. 4 versions in a week is too much. Give others time to
> comment and me to respond to discussions. Like a week...

I apologize if my actions may have come across as overly hasty. I will make
sure to allow for more time between submissions in the future, to ensure that
everyone has an adequate opportunity to review and provide feedback.

Regarding the use of bus_addr instead of cpu_addr, I found that cpu_addr was
populating as OF_BAD_ADDR while bus_addr was populating correctly.  I think
this is because I should be defining a empty ranges property in parent node
for indicating 1:1 mapping between parent and child.

But once I add empty ranges in property I get other warnings by dt_binding_check
tool. After fixing all I am able to come up with below device tree example, please
let me know if there is anything to be corrected. If this is good I will send
the next version (offcource after a week :)) using cpu_addr.

    soc {
        #address-cells = <2>;
        #size-cells = <1>;
        bus {
            compatible = "simple-bus";
            #address-cells = <2>;
            #size-cells = <1>;
            ranges;

            vmbus@ff0000000 {
                compatible = "microsoft,vmbus";
                #address-cells = <2>;
                #size-cells = <1>;
                ranges = <0x0f 0xf0000000 0x0f 0xf0000000 0x10000000>;
            };
        };
    };

> 
> Rob

Re: [PATCH v5 5/5] Driver: VMBus: Add device tree support

[Saurabh Singh Sengar]

On Mon, Feb 13, 2023 at 03:24:39AM -0800, Saurabh Singh Sengar wrote:
> On Fri, Feb 10, 2023 at 04:37:28PM -0600, Rob Herring wrote:
> > On Thu, Feb 9, 2023 at 11:46 AM Saurabh Singh Sengar
> > <ssengar@linux.microsoft.com> wrote:
> > >
> > > On Thu, Feb 09, 2023 at 09:50:31AM -0600, Rob Herring wrote:
> > > > On Thu, Feb 9, 2023 at 6:15 AM Saurabh Sengar
> > > > <ssengar@linux.microsoft.com> wrote:
> > > > >
> > > > > Update the driver to support device tree boot as well along with ACPI.
> > > >
> > > > Devicetree
> > 
> > [...]
> > 
> > > > > +       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.pci_addr;
> > > >
> > > > This is not PCI. It's a union, so use 'bus_addr' instead.
> > > >
> > > > But wait, resources and IORESOURCE_MEM are *CPU* addresses. You need
> > > > cpu_addr here. Your DT happens to do 1:1 addresses so it happens to
> > > > work either way.
> > >
> > > bus_addr works for us, will send V6
> > 
> > Sigh. bus_addr may work, but is wrong as I explained.
> > 
> > And you've already sent v6... Please slow down your pace with sending
> > new versions. 4 versions in a week is too much. Give others time to
> > comment and me to respond to discussions. Like a week...
> 
> I apologize if my actions may have come across as overly hasty. I will make
> sure to allow for more time between submissions in the future, to ensure that
> everyone has an adequate opportunity to review and provide feedback.
> 
> Regarding the use of bus_addr instead of cpu_addr, I found that cpu_addr was
> populating as OF_BAD_ADDR while bus_addr was populating correctly.  I think
> this is because I should be defining a empty ranges property in parent node
> for indicating 1:1 mapping between parent and child.
> 
> But once I add empty ranges in property I get other warnings by dt_binding_check
> tool. After fixing all I am able to come up with below device tree example, please
> let me know if there is anything to be corrected. If this is good I will send
> the next version (offcource after a week :)) using cpu_addr.
> 
>     soc {
>         #address-cells = <2>;
>         #size-cells = <1>;
>         bus {
>             compatible = "simple-bus";
>             #address-cells = <2>;
>             #size-cells = <1>;
>             ranges;
> 
>             vmbus@ff0000000 {
>                 compatible = "microsoft,vmbus";
>                 #address-cells = <2>;
>                 #size-cells = <1>;
>                 ranges = <0x0f 0xf0000000 0x0f 0xf0000000 0x10000000>;
>             };
>         };
>     };
> 
> > 

Rob,

If you find the above changes satisfactory, please do let me know so that
I can send the updated version.

As far as I am aware, all the pending comments and issues have been addressed
in this series. However, if there are any remaining concerns or feedback that
require attention, please let me know.

- Saurabh

> > Rob