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

[PATCH v7 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.

The fourth patch introduces the device tree documentation for VMBus.

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

[V7]
- Use cpu_addr instead of bus_addr.
- Updated Documentation accordingly.

[V6]
- define acpi_sleep_state_supported in acpi header for !ACPI,
  dropped the changes done in hv_common.c under #ifdef CONFIG_ACPI
- "Devicetree" instead of "device tree"
- Remove initialize of ret
- set "np = pdev->dev.of_node;" on declarartion
- remove one error print
- use bus_addr instead of pci_addr


[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.
- Added missing full stop at the end of commit message.
- Typos fix: s/Initaly/Initially/ and s/hibernate/hibernation/.
- Replace to_acpi_device with ACPI_COMPANION which simplify the logic.
- Added more info in cover letter aboutsystem under test.

[v2]
- Convert VMBus acpi device to platform device, and added device tree support
  in separate patch. This enables using same driver structure for both the flows.
- In Device tree documentation, changed virtio folder to hypervisor and moved
  VMBus documentation there.
- Moved bindings before Device tree patch.
- Removed stale ".data" and ".name" field from of_device match table.
- Removed debug print.

Saurabh Sengar (5):
  drivers/clocksource/hyper-v: non ACPI support in hyperv clock
  ACPI: bus: Add stub acpi_sleep_state_supported() in non-ACPI cases
  Drivers: hv: vmbus: Convert acpi_device to more generic
    platform_device
  dt-bindings: bus: VMBus
  Driver: VMBus: Add Devicetree support

 .../bindings/bus/microsoft,vmbus.yaml         |  54 ++++++++
 MAINTAINERS                                   |   1 +
 drivers/clocksource/hyperv_timer.c            |  15 ++-
 drivers/hv/Kconfig                            |   6 +-
 drivers/hv/vmbus_drv.c                        | 115 ++++++++++++++----
 include/linux/acpi.h                          |   5 +
 6 files changed, 167 insertions(+), 29 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml

-- 
2.34.1

[PATCH v7 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 v7 2/5] ACPI: bus: Add stub acpi_sleep_state_supported() in non-ACPI cases

[Saurabh Sengar]

acpi_sleep_state_supported() is defined only when CONFIG_ACPI=y. The
function is in acpi_bus.h, and acpi_bus.h can only be used in
CONFIG_ACPI=y cases. Add the stub function to linux/acpi.h to make
compilation successful for !CONFIG_ACPI cases.

Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 include/linux/acpi.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index efff750f326d..d331f76b0c19 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1075,6 +1075,11 @@ static inline u32 acpi_osc_ctx_get_cxl_control(struct acpi_osc_context *context)
 	return 0;
 }
 
+static inline bool acpi_sleep_state_supported(u8 sleep_state)
+{
+	return false;
+}
+
 #endif	/* !CONFIG_ACPI */
 
 #ifdef CONFIG_ACPI_HOTPLUG_IOAPIC
-- 
2.34.1

[PATCH v7 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
Devicetree 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 v7 4/5] dt-bindings: bus: VMBus

[Saurabh Sengar]

Add dt-bindings for Hyper-V VMBus.

Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
---
[V7]
- update ranges; property in simplie-bus for correct 1:1 translation.

 .../bindings/bus/microsoft,vmbus.yaml         | 54 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 55 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..a8d40c766dcd
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml
@@ -0,0 +1,54 @@
+# 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:
+  - |
+    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>;
+            };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index f32aca51242f..aae3c1fb55fd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9510,6 +9510,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 v7 5/5] Driver: VMBus: Add Devicetree support

[Saurabh Sengar]

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.
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,
 	}
-- 
2.34.1

Re: [PATCH v7 4/5] dt-bindings: bus: VMBus

[Saurabh Singh Sengar]

On Thu, Feb 23, 2023 at 03:29:04AM -0800, Saurabh Sengar wrote:
> Add dt-bindings for Hyper-V VMBus.
> 
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
> [V7]
> - update ranges; property in simplie-bus for correct 1:1 translation.
> 
>  .../bindings/bus/microsoft,vmbus.yaml         | 54 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 55 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..a8d40c766dcd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml
> @@ -0,0 +1,54 @@
> +# 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:
> +  - |
> +    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>;
> +            };
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f32aca51242f..aae3c1fb55fd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9510,6 +9510,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

Hi Rob,

Did you get chance to review this. Please let me know if you need any additional
information or if its good to be merged.

Regards,
Saurabh

Re: [PATCH v7 4/5] dt-bindings: bus: VMBus

[Rob Herring]

On Thu, 23 Feb 2023 03:29:04 -0800, Saurabh Sengar wrote:
> Add dt-bindings for Hyper-V VMBus.
> 
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
> [V7]
> - update ranges; property in simplie-bus for correct 1:1 translation.
> 
>  .../bindings/bus/microsoft,vmbus.yaml         | 54 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v7 5/5] Driver: VMBus: Add Devicetree support

[Wei Liu]

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
> 

Re: [PATCH v7 5/5] Driver: VMBus: Add Devicetree support

[Saurabh Singh Sengar]

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
> > 

RE: [PATCH v7 5/5] Driver: VMBus: Add Devicetree support

[Michael Kelley (LINUX)]

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
> > >

Re: [PATCH v7 5/5] Driver: VMBus: Add Devicetree support

[Saurabh Singh Sengar]

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
> > > >

RE: [PATCH v7 5/5] Driver: VMBus: Add Devicetree support

[Michael Kelley (LINUX)]

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
> > > > >

RE: [PATCH v7 5/5] Driver: VMBus: Add Devicetree support

[Michael Kelley (LINUX)]

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

Re: [PATCH v7 5/5] Driver: VMBus: Add Devicetree support

[Saurabh Singh Sengar]

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

RE: [PATCH v7 5/5] Driver: VMBus: Add Devicetree support

[Michael Kelley (LINUX)]

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

Re: [PATCH v7 5/5] Driver: VMBus: Add Devicetree support

[Saurabh Singh Sengar]

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