[PATCH v2 0/3] Drivers: hv: Avoid allocating MMIO from framebuffer region for other passed through PCI devices

[PATCH v2 0/3] Drivers: hv: Avoid allocating MMIO from framebuffer region for other passed through PCI devices

[Vitaly Kuznetsov]

Changes since v1:
- Dropped PATCH2 as it is no longer needed.
- Move PCI ids to linux/pci_ids.h [Michael]
- Correctly handle "!(pdev->resource[0].flags & IORESOURCE_MEM)" case
  [Michael].

Passed through PCI device sometimes misbehave on Gen1 VMs when Hyper-V
DRM driver is also loaded. Looking at IOMEM assignment, we can see e.g.

$ cat /proc/iomem
...
f8000000-fffbffff : PCI Bus 0000:00
  f8000000-fbffffff : 0000:00:08.0
    f8000000-f8001fff : bb8c4f33-2ba2-4808-9f7f-02f3b4da22fe
...
fe0000000-fffffffff : PCI Bus 0000:00
  fe0000000-fe07fffff : bb8c4f33-2ba2-4808-9f7f-02f3b4da22fe
    fe0000000-fe07fffff : 2ba2:00:02.0
      fe0000000-fe07fffff : mlx4_core

the interesting part is the 'f8000000' region as it is actually the
VM's framebuffer:

$ lspci -v
...
0000:00:08.0 VGA compatible controller: Microsoft Corporation Hyper-V virtual VGA (prog-if 00 [VGA controller])
	Flags: bus master, fast devsel, latency 0, IRQ 11
	Memory at f8000000 (32-bit, non-prefetchable) [size=64M]
...

Recently merged commit a0ab5abced55 ("drm/hyperv : Removing the restruction of
VRAM allocation with PCI bar size") improved the situation as resources,
reserved through vmbus_allocate_mmio() can't be allocated twice:

...
f8000000-fffbffff : PCI Bus 0000:00
  f8000000-fbffffff : 0000:00:08.0
    f8000000-f8001fff : bb8c4f33-2ba2-4808-9f7f-02f3b4da22fe
    f8100000-f88fffff : 5620e0c7-8062-4dce-aeb7-520c7ef76171
...

Always reserve FB region on Gen1 VMs (PATCH2) and make sure we never allocate
anything besides framebuffer from there (PATCH3).

Vitaly Kuznetsov (3):
  PCI: Move PCI_VENDOR_ID_MICROSOFT/PCI_DEVICE_ID_HYPERV_VIDEO
    definitions to pci_ids.h
  Drivers: hv: Always reserve framebuffer region for Gen1 VMs
  Drivers: hv: Never allocate anything besides framebuffer from
    framebuffer memory region

 drivers/gpu/drm/hyperv/hyperv_drm_drv.c       |  3 -
 drivers/hv/vmbus_drv.c                        | 56 ++++++++++++++-----
 .../net/ethernet/microsoft/mana/gdma_main.c   |  4 --
 drivers/video/fbdev/hyperv_fb.c               |  4 --
 include/linux/pci_ids.h                       |  3 +
 5 files changed, 44 insertions(+), 26 deletions(-)

-- 
2.37.1

[PATCH v2 1/3] PCI: Move PCI_VENDOR_ID_MICROSOFT/PCI_DEVICE_ID_HYPERV_VIDEO definitions to pci_ids.h

[Vitaly Kuznetsov]

There are already three places in kernel which define PCI_VENDOR_ID_MICROSOFT
and two for PCI_DEVICE_ID_HYPERV_VIDEO and there's a need to use these
from core Vmbus code. Move the defines where they belong.

No functional change.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/gpu/drm/hyperv/hyperv_drm_drv.c         | 3 ---
 drivers/net/ethernet/microsoft/mana/gdma_main.c | 4 ----
 drivers/video/fbdev/hyperv_fb.c                 | 4 ----
 include/linux/pci_ids.h                         | 3 +++
 4 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
index 6d11e7938c83..40888e36f91a 100644
--- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
+++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
@@ -23,9 +23,6 @@
 #define DRIVER_MAJOR 1
 #define DRIVER_MINOR 0
 
-#define PCI_VENDOR_ID_MICROSOFT 0x1414
-#define PCI_DEVICE_ID_HYPERV_VIDEO 0x5353
-
 DEFINE_DRM_GEM_FOPS(hv_fops);
 
 static struct drm_driver hyperv_driver = {
diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index 5f9240182351..00d8198072ae 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -1465,10 +1465,6 @@ static void mana_gd_shutdown(struct pci_dev *pdev)
 	pci_disable_device(pdev);
 }
 
-#ifndef PCI_VENDOR_ID_MICROSOFT
-#define PCI_VENDOR_ID_MICROSOFT 0x1414
-#endif
-
 static const struct pci_device_id mana_id_table[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_MICROSOFT, MANA_PF_DEVICE_ID) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_MICROSOFT, MANA_VF_DEVICE_ID) },
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index 886c564787f1..b58b445bb529 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -74,10 +74,6 @@
 #define SYNTHVID_DEPTH_WIN8 32
 #define SYNTHVID_FB_SIZE_WIN8 (8 * 1024 * 1024)
 
-#define PCI_VENDOR_ID_MICROSOFT 0x1414
-#define PCI_DEVICE_ID_HYPERV_VIDEO 0x5353
-
-
 enum pipe_msg_type {
 	PIPE_MSG_INVALID,
 	PIPE_MSG_DATA,
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 6feade66efdb..15b49e655ce3 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2079,6 +2079,9 @@
 #define PCI_DEVICE_ID_ICE_1712		0x1712
 #define PCI_DEVICE_ID_VT1724		0x1724
 
+#define PCI_VENDOR_ID_MICROSOFT		0x1414
+#define PCI_DEVICE_ID_HYPERV_VIDEO	0x5353
+
 #define PCI_VENDOR_ID_OXSEMI		0x1415
 #define PCI_DEVICE_ID_OXSEMI_12PCI840	0x8403
 #define PCI_DEVICE_ID_OXSEMI_PCIe840		0xC000
-- 
2.37.1

[PATCH v2 2/3] Drivers: hv: Always reserve framebuffer region for Gen1 VMs

[Vitaly Kuznetsov]

vmbus_reserve_fb() tries reserving framebuffer region iff
'screen_info.lfb_base' is set. Gen2 VMs seem to have it set by EFI fb
(or, in some edge cases like kexec, the address where the buffer was
moved, see
https://lore.kernel.org/all/20201014092429.1415040-1-kasong@redhat.com/)
but on Gen1 VM it depends on bootloader behavior. With grub, it depends
on 'gfxpayload=' setting but in some cases it is observed to be zero.
Relying on 'screen_info.lfb_base' to reserve framebuffer region is
risky. Instead, it is possible to get the address from the dedicated
PCI device which is always present.

Check for legacy PCI video device presence and reserve the whole
region for framebuffer on Gen1 VMs.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/vmbus_drv.c | 46 +++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 23c680d1a0f5..536f68e563c6 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -35,6 +35,7 @@
 #include <linux/kernel.h>
 #include <linux/syscore_ops.h>
 #include <linux/dma-map-ops.h>
+#include <linux/pci.h>
 #include <clocksource/hyperv_timer.h>
 #include "hyperv_vmbus.h"
 
@@ -2262,26 +2263,43 @@ static int vmbus_acpi_remove(struct acpi_device *device)
 
 static void vmbus_reserve_fb(void)
 {
-	int size;
+	resource_size_t start = 0, size;
+	struct pci_dev *pdev;
+
+	if (efi_enabled(EFI_BOOT)) {
+		/* Gen2 VM: get FB base from EFI framebuffer */
+		start = screen_info.lfb_base;
+		size = max_t(__u32, screen_info.lfb_size, 0x800000);
+	} else {
+		/* Gen1 VM: get FB base from PCI */
+		pdev = pci_get_device(PCI_VENDOR_ID_MICROSOFT,
+				      PCI_DEVICE_ID_HYPERV_VIDEO, NULL);
+		if (!pdev)
+			return;
+
+		if (pdev->resource[0].flags & IORESOURCE_MEM) {
+			start = pci_resource_start(pdev, 0);
+			size = pci_resource_len(pdev, 0);
+		}
+
+		/*
+		 * Release the PCI device so hyperv_drm or hyperv_fb driver can
+		 * grab it later.
+		 */
+		pci_dev_put(pdev);
+	}
+
+	if (!start)
+		return;
+
 	/*
 	 * Make a claim for the frame buffer in the resource tree under the
 	 * first node, which will be the one below 4GB.  The length seems to
 	 * be underreported, particularly in a Generation 1 VM.  So start out
 	 * reserving a larger area and make it smaller until it succeeds.
 	 */
-
-	if (screen_info.lfb_base) {
-		if (efi_enabled(EFI_BOOT))
-			size = max_t(__u32, screen_info.lfb_size, 0x800000);
-		else
-			size = max_t(__u32, screen_info.lfb_size, 0x4000000);
-
-		for (; !fb_mmio && (size >= 0x100000); size >>= 1) {
-			fb_mmio = __request_region(hyperv_mmio,
-						   screen_info.lfb_base, size,
-						   fb_mmio_name, 0);
-		}
-	}
+	for (; !fb_mmio && (size >= 0x100000); size >>= 1)
+		fb_mmio = __request_region(hyperv_mmio, start, size, fb_mmio_name, 0);
 }
 
 /**
-- 
2.37.1

[PATCH v2 3/3] Drivers: hv: Never allocate anything besides framebuffer from framebuffer memory region

[Vitaly Kuznetsov]

Passed through PCI device sometimes misbehave on Gen1 VMs when Hyper-V
DRM driver is also loaded. Looking at IOMEM assignment, we can see e.g.

$ cat /proc/iomem
...
f8000000-fffbffff : PCI Bus 0000:00
  f8000000-fbffffff : 0000:00:08.0
    f8000000-f8001fff : bb8c4f33-2ba2-4808-9f7f-02f3b4da22fe
...
fe0000000-fffffffff : PCI Bus 0000:00
  fe0000000-fe07fffff : bb8c4f33-2ba2-4808-9f7f-02f3b4da22fe
    fe0000000-fe07fffff : 2ba2:00:02.0
      fe0000000-fe07fffff : mlx4_core

the interesting part is the 'f8000000' region as it is actually the
VM's framebuffer:

$ lspci -v
...
0000:00:08.0 VGA compatible controller: Microsoft Corporation Hyper-V virtual VGA (prog-if 00 [VGA controller])
	Flags: bus master, fast devsel, latency 0, IRQ 11
	Memory at f8000000 (32-bit, non-prefetchable) [size=64M]
...

 hv_vmbus: registering driver hyperv_drm
 hyperv_drm 5620e0c7-8062-4dce-aeb7-520c7ef76171: [drm] Synthvid Version major 3, minor 5
 hyperv_drm 0000:00:08.0: vgaarb: deactivate vga console
 hyperv_drm 0000:00:08.0: BAR 0: can't reserve [mem 0xf8000000-0xfbffffff]
 hyperv_drm 5620e0c7-8062-4dce-aeb7-520c7ef76171: [drm] Cannot request framebuffer, boot fb still active?

Note: "Cannot request framebuffer" is not a fatal error in
hyperv_setup_gen1() as the code assumes there's some other framebuffer
device there but we actually have some other PCI device (mlx4 in this
case) config space there!

The problem appears to be that vmbus_allocate_mmio() can allocate from
the reserved framebuffer region (fb_overlap_ok), however, if the
request to allocate MMIO comes from some other device before
framebuffer region is taken, it can happily use framebuffer region for
it. Note, Gen2 VMs are usually unaffected by the issue because
framebuffer region is already taken by EFI fb (in case kernel supports
it) but Gen1 VMs may have this region unclaimed by the time Hyper-V PCI
pass-through driver tries allocating MMIO space if Hyper-V DRM/FB drivers
load after it. Devices can be brought up in any sequence so let's
resolve the issue by always ignoring 'fb_mmio' region for non-FB
requests, even if the region is unclaimed.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/vmbus_drv.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 536f68e563c6..3c833ea60db6 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -2331,7 +2331,7 @@ int vmbus_allocate_mmio(struct resource **new, struct hv_device *device_obj,
 			bool fb_overlap_ok)
 {
 	struct resource *iter, *shadow;
-	resource_size_t range_min, range_max, start;
+	resource_size_t range_min, range_max, start, end;
 	const char *dev_n = dev_name(&device_obj->device);
 	int retval;
 
@@ -2366,6 +2366,14 @@ int vmbus_allocate_mmio(struct resource **new, struct hv_device *device_obj,
 		range_max = iter->end;
 		start = (range_min + align - 1) & ~(align - 1);
 		for (; start + size - 1 <= range_max; start += align) {
+			end = start + size - 1;
+
+			/* Skip the whole fb_mmio region if not fb_overlap_ok */
+			if (!fb_overlap_ok && fb_mmio &&
+			    (((start >= fb_mmio->start) && (start <= fb_mmio->end)) ||
+			     ((end >= fb_mmio->start) && (end <= fb_mmio->end))))
+				continue;
+
 			shadow = __request_region(iter, start, size, NULL,
 						  IORESOURCE_BUSY);
 			if (!shadow)
-- 
2.37.1

RE: [PATCH v2 1/3] PCI: Move PCI_VENDOR_ID_MICROSOFT/PCI_DEVICE_ID_HYPERV_VIDEO definitions to pci_ids.h

[Michael Kelley (LINUX)]

From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Thursday, August 25, 2022 2:00 AM
> 
> There are already three places in kernel which define PCI_VENDOR_ID_MICROSOFT
> and two for PCI_DEVICE_ID_HYPERV_VIDEO and there's a need to use these
> from core Vmbus code. Move the defines where they belong.
> 
> No functional change.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  drivers/gpu/drm/hyperv/hyperv_drm_drv.c         | 3 ---
>  drivers/net/ethernet/microsoft/mana/gdma_main.c | 4 ----
>  drivers/video/fbdev/hyperv_fb.c                 | 4 ----
>  include/linux/pci_ids.h                         | 3 +++
>  4 files changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> index 6d11e7938c83..40888e36f91a 100644
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> @@ -23,9 +23,6 @@
>  #define DRIVER_MAJOR 1
>  #define DRIVER_MINOR 0
> 
> -#define PCI_VENDOR_ID_MICROSOFT 0x1414
> -#define PCI_DEVICE_ID_HYPERV_VIDEO 0x5353
> -
>  DEFINE_DRM_GEM_FOPS(hv_fops);
> 
>  static struct drm_driver hyperv_driver = {
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 5f9240182351..00d8198072ae 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -1465,10 +1465,6 @@ static void mana_gd_shutdown(struct pci_dev *pdev)
>  	pci_disable_device(pdev);
>  }
> 
> -#ifndef PCI_VENDOR_ID_MICROSOFT
> -#define PCI_VENDOR_ID_MICROSOFT 0x1414
> -#endif
> -
>  static const struct pci_device_id mana_id_table[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_MICROSOFT, MANA_PF_DEVICE_ID) },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_MICROSOFT, MANA_VF_DEVICE_ID) },
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index 886c564787f1..b58b445bb529 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -74,10 +74,6 @@
>  #define SYNTHVID_DEPTH_WIN8 32
>  #define SYNTHVID_FB_SIZE_WIN8 (8 * 1024 * 1024)
> 
> -#define PCI_VENDOR_ID_MICROSOFT 0x1414
> -#define PCI_DEVICE_ID_HYPERV_VIDEO 0x5353
> -
> -
>  enum pipe_msg_type {
>  	PIPE_MSG_INVALID,
>  	PIPE_MSG_DATA,
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 6feade66efdb..15b49e655ce3 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2079,6 +2079,9 @@
>  #define PCI_DEVICE_ID_ICE_1712		0x1712
>  #define PCI_DEVICE_ID_VT1724		0x1724
> 
> +#define PCI_VENDOR_ID_MICROSOFT		0x1414
> +#define PCI_DEVICE_ID_HYPERV_VIDEO	0x5353
> +
>  #define PCI_VENDOR_ID_OXSEMI		0x1415
>  #define PCI_DEVICE_ID_OXSEMI_12PCI840	0x8403
>  #define PCI_DEVICE_ID_OXSEMI_PCIe840		0xC000
> --
> 2.37.1

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

RE: [PATCH v2 2/3] Drivers: hv: Always reserve framebuffer region for Gen1 VMs

[Michael Kelley (LINUX)]

From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Thursday, August 25, 2022 2:00 AM
> 
> vmbus_reserve_fb() tries reserving framebuffer region iff
> 'screen_info.lfb_base' is set. Gen2 VMs seem to have it set by EFI fb

Just so I'm clear, by "EFI fb" you mean the EFI layer code that sets
up the frame buffer before the Linux kernel ever boots, right?
You are not referring to the Linux kernel EFI framebuffer
driver, which may or may not be configured in the kernel.

> (or, in some edge cases like kexec, the address where the buffer was
> moved, see https://lore.kernel.org/all/20201014092429.1415040-1-kasong@redhat.com/
> but on Gen1 VM it depends on bootloader behavior. With grub, it depends
> on 'gfxpayload=' setting but in some cases it is observed to be zero.
> Relying on 'screen_info.lfb_base' to reserve framebuffer region is
> risky. Instead, it is possible to get the address from the dedicated
> PCI device which is always present.
> 
> Check for legacy PCI video device presence and reserve the whole
> region for framebuffer on Gen1 VMs.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  drivers/hv/vmbus_drv.c | 46 +++++++++++++++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 23c680d1a0f5..536f68e563c6 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -35,6 +35,7 @@
>  #include <linux/kernel.h>
>  #include <linux/syscore_ops.h>
>  #include <linux/dma-map-ops.h>
> +#include <linux/pci.h>
>  #include <clocksource/hyperv_timer.h>
>  #include "hyperv_vmbus.h"
> 
> @@ -2262,26 +2263,43 @@ static int vmbus_acpi_remove(struct acpi_device *device)
> 
>  static void vmbus_reserve_fb(void)
>  {
> -	int size;
> +	resource_size_t start = 0, size;
> +	struct pci_dev *pdev;
> +
> +	if (efi_enabled(EFI_BOOT)) {
> +		/* Gen2 VM: get FB base from EFI framebuffer */
> +		start = screen_info.lfb_base;
> +		size = max_t(__u32, screen_info.lfb_size, 0x800000);
> +	} else {
> +		/* Gen1 VM: get FB base from PCI */
> +		pdev = pci_get_device(PCI_VENDOR_ID_MICROSOFT,
> +				      PCI_DEVICE_ID_HYPERV_VIDEO, NULL);
> +		if (!pdev)
> +			return;
> +
> +		if (pdev->resource[0].flags & IORESOURCE_MEM) {
> +			start = pci_resource_start(pdev, 0);
> +			size = pci_resource_len(pdev, 0);
> +		}
> +
> +		/*
> +		 * Release the PCI device so hyperv_drm or hyperv_fb driver can
> +		 * grab it later.
> +		 */
> +		pci_dev_put(pdev);
> +	}
> +
> +	if (!start)
> +		return;
> +
>  	/*
>  	 * Make a claim for the frame buffer in the resource tree under the
>  	 * first node, which will be the one below 4GB.  The length seems to
>  	 * be underreported, particularly in a Generation 1 VM.  So start out
>  	 * reserving a larger area and make it smaller until it succeeds.
>  	 */
> -
> -	if (screen_info.lfb_base) {
> -		if (efi_enabled(EFI_BOOT))
> -			size = max_t(__u32, screen_info.lfb_size, 0x800000);
> -		else
> -			size = max_t(__u32, screen_info.lfb_size, 0x4000000);
> -
> -		for (; !fb_mmio && (size >= 0x100000); size >>= 1) {
> -			fb_mmio = __request_region(hyperv_mmio,
> -						   screen_info.lfb_base, size,
> -						   fb_mmio_name, 0);
> -		}
> -	}
> +	for (; !fb_mmio && (size >= 0x100000); size >>= 1)
> +		fb_mmio = __request_region(hyperv_mmio, start, size, fb_mmio_name, 0);
>  }
> 
>  /**
> --
> 2.37.1

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

RE: [PATCH v2 3/3] Drivers: hv: Never allocate anything besides framebuffer from framebuffer memory region

[Michael Kelley (LINUX)]

From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Thursday, August 25, 2022 2:00 AM
> 
> Passed through PCI device sometimes misbehave on Gen1 VMs when Hyper-V
> DRM driver is also loaded. Looking at IOMEM assignment, we can see e.g.
> 
> $ cat /proc/iomem
> ...
> f8000000-fffbffff : PCI Bus 0000:00
>   f8000000-fbffffff : 0000:00:08.0
>     f8000000-f8001fff : bb8c4f33-2ba2-4808-9f7f-02f3b4da22fe
> ...
> fe0000000-fffffffff : PCI Bus 0000:00
>   fe0000000-fe07fffff : bb8c4f33-2ba2-4808-9f7f-02f3b4da22fe
>     fe0000000-fe07fffff : 2ba2:00:02.0
>       fe0000000-fe07fffff : mlx4_core
> 
> the interesting part is the 'f8000000' region as it is actually the
> VM's framebuffer:
> 
> $ lspci -v
> ...
> 0000:00:08.0 VGA compatible controller: Microsoft Corporation Hyper-V virtual VGA
> (prog-if 00 [VGA controller])
> 	Flags: bus master, fast devsel, latency 0, IRQ 11
> 	Memory at f8000000 (32-bit, non-prefetchable) [size=64M]
> ...
> 
>  hv_vmbus: registering driver hyperv_drm
>  hyperv_drm 5620e0c7-8062-4dce-aeb7-520c7ef76171: [drm] Synthvid Version major 3, minor 5
>  hyperv_drm 0000:00:08.0: vgaarb: deactivate vga console
>  hyperv_drm 0000:00:08.0: BAR 0: can't reserve [mem 0xf8000000-0xfbffffff]
>  hyperv_drm 5620e0c7-8062-4dce-aeb7-520c7ef76171: [drm] Cannot request framebuffer, boot fb still active?
> 
> Note: "Cannot request framebuffer" is not a fatal error in
> hyperv_setup_gen1() as the code assumes there's some other framebuffer
> device there but we actually have some other PCI device (mlx4 in this
> case) config space there!

My apologies for not getting around to commenting on the previous
version of this patch.  The function hyperv_setup_gen1() and the
"Cannot request framebuffer" message have gone away as of
commit a0ab5abced55.

> 
> The problem appears to be that vmbus_allocate_mmio() can allocate from
> the reserved framebuffer region (fb_overlap_ok), however, if the
> request to allocate MMIO comes from some other device before
> framebuffer region is taken, it can happily use framebuffer region for
> it. 

Interesting. I had never looked at the details of vmbus_allocate_mmio().
The semantics one might assume of a parameter named "fb_overlap_ok"
aren't implemented because !fb_overlap_ok essentially has no effect.   The
existing semantics are really "prefer_fb_overlap".  This patch implements
the expected and needed semantics, which is to not allocate from the frame
buffer space when !fb_overlap_ok.

If that's an accurate high level summary, maybe this commit message
could describe it that way?  The other details you provide about what can
go wrong should still be included as well.

> Note, Gen2 VMs are usually unaffected by the issue because
> framebuffer region is already taken by EFI fb (in case kernel supports
> it) but Gen1 VMs may have this region unclaimed by the time Hyper-V PCI
> pass-through driver tries allocating MMIO space if Hyper-V DRM/FB drivers
> load after it. Devices can be brought up in any sequence so let's
> resolve the issue by always ignoring 'fb_mmio' region for non-FB
> requests, even if the region is unclaimed.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  drivers/hv/vmbus_drv.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 536f68e563c6..3c833ea60db6 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -2331,7 +2331,7 @@ int vmbus_allocate_mmio(struct resource **new, struct
> hv_device *device_obj,
>  			bool fb_overlap_ok)
>  {
>  	struct resource *iter, *shadow;
> -	resource_size_t range_min, range_max, start;
> +	resource_size_t range_min, range_max, start, end;
>  	const char *dev_n = dev_name(&device_obj->device);
>  	int retval;
> 
> @@ -2366,6 +2366,14 @@ int vmbus_allocate_mmio(struct resource **new, struct
> hv_device *device_obj,
>  		range_max = iter->end;
>  		start = (range_min + align - 1) & ~(align - 1);
>  		for (; start + size - 1 <= range_max; start += align) {
> +			end = start + size - 1;
> +
> +			/* Skip the whole fb_mmio region if not fb_overlap_ok */
> +			if (!fb_overlap_ok && fb_mmio &&
> +			    (((start >= fb_mmio->start) && (start <= fb_mmio->end)) ||
> +			     ((end >= fb_mmio->start) && (end <= fb_mmio->end))))
> +				continue;
> +
>  			shadow = __request_region(iter, start, size, NULL,
>  						  IORESOURCE_BUSY);
>  			if (!shadow)
> --
> 2.37.1

Other than my musings on the commit message,

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

Re: [PATCH v2 1/3] PCI: Move PCI_VENDOR_ID_MICROSOFT/PCI_DEVICE_ID_HYPERV_VIDEO definitions to pci_ids.h

[Bjorn Helgaas]

On Thu, Aug 25, 2022 at 11:00:22AM +0200, Vitaly Kuznetsov wrote:
> There are already three places in kernel which define PCI_VENDOR_ID_MICROSOFT
> and two for PCI_DEVICE_ID_HYPERV_VIDEO and there's a need to use these
> from core Vmbus code. Move the defines where they belong.
> 
> No functional change.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>	# pci_ids.h

> ---
>  drivers/gpu/drm/hyperv/hyperv_drm_drv.c         | 3 ---
>  drivers/net/ethernet/microsoft/mana/gdma_main.c | 4 ----
>  drivers/video/fbdev/hyperv_fb.c                 | 4 ----
>  include/linux/pci_ids.h                         | 3 +++
>  4 files changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> index 6d11e7938c83..40888e36f91a 100644
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> @@ -23,9 +23,6 @@
>  #define DRIVER_MAJOR 1
>  #define DRIVER_MINOR 0
>  
> -#define PCI_VENDOR_ID_MICROSOFT 0x1414
> -#define PCI_DEVICE_ID_HYPERV_VIDEO 0x5353
> -
>  DEFINE_DRM_GEM_FOPS(hv_fops);
>  
>  static struct drm_driver hyperv_driver = {
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 5f9240182351..00d8198072ae 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -1465,10 +1465,6 @@ static void mana_gd_shutdown(struct pci_dev *pdev)
>  	pci_disable_device(pdev);
>  }
>  
> -#ifndef PCI_VENDOR_ID_MICROSOFT
> -#define PCI_VENDOR_ID_MICROSOFT 0x1414
> -#endif
> -
>  static const struct pci_device_id mana_id_table[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_MICROSOFT, MANA_PF_DEVICE_ID) },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_MICROSOFT, MANA_VF_DEVICE_ID) },
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index 886c564787f1..b58b445bb529 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -74,10 +74,6 @@
>  #define SYNTHVID_DEPTH_WIN8 32
>  #define SYNTHVID_FB_SIZE_WIN8 (8 * 1024 * 1024)
>  
> -#define PCI_VENDOR_ID_MICROSOFT 0x1414
> -#define PCI_DEVICE_ID_HYPERV_VIDEO 0x5353
> -
> -
>  enum pipe_msg_type {
>  	PIPE_MSG_INVALID,
>  	PIPE_MSG_DATA,
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 6feade66efdb..15b49e655ce3 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2079,6 +2079,9 @@
>  #define PCI_DEVICE_ID_ICE_1712		0x1712
>  #define PCI_DEVICE_ID_VT1724		0x1724
>  
> +#define PCI_VENDOR_ID_MICROSOFT		0x1414
> +#define PCI_DEVICE_ID_HYPERV_VIDEO	0x5353
> +
>  #define PCI_VENDOR_ID_OXSEMI		0x1415
>  #define PCI_DEVICE_ID_OXSEMI_12PCI840	0x8403
>  #define PCI_DEVICE_ID_OXSEMI_PCIe840		0xC000
> -- 
> 2.37.1
> 

RE: [PATCH v2 2/3] Drivers: hv: Always reserve framebuffer region for Gen1 VMs

[Vitaly Kuznetsov]

"Michael Kelley (LINUX)" <mikelley@microsoft.com> writes:

> From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Thursday, August 25, 2022 2:00 AM
>> 
>> vmbus_reserve_fb() tries reserving framebuffer region iff
>> 'screen_info.lfb_base' is set. Gen2 VMs seem to have it set by EFI fb
>
> Just so I'm clear, by "EFI fb" you mean the EFI layer code that sets
> up the frame buffer before the Linux kernel ever boots, right?
> You are not referring to the Linux kernel EFI framebuffer
> driver, which may or may not be configured in the kernel.

My very shallow understanding is that initially, screen_info comes from
boot_params and this depends on how Linux was booted. Kernel EFI
framebuffer (when enabled), however, gets it first and can modify it
(see efifb_setup()) before we get to analyze it in Vmbus.

>
>> (or, in some edge cases like kexec, the address where the buffer was
>> moved, see https://lore.kernel.org/all/20201014092429.1415040-1-kasong@redhat.com/
>> but on Gen1 VM it depends on bootloader behavior. With grub, it depends
>> on 'gfxpayload=' setting but in some cases it is observed to be zero.
>> Relying on 'screen_info.lfb_base' to reserve framebuffer region is
>> risky. Instead, it is possible to get the address from the dedicated
>> PCI device which is always present.
>> 
>> Check for legacy PCI video device presence and reserve the whole
>> region for framebuffer on Gen1 VMs.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  drivers/hv/vmbus_drv.c | 46 +++++++++++++++++++++++++++++-------------
>>  1 file changed, 32 insertions(+), 14 deletions(-)
>> 
>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> index 23c680d1a0f5..536f68e563c6 100644
>> --- a/drivers/hv/vmbus_drv.c
>> +++ b/drivers/hv/vmbus_drv.c
>> @@ -35,6 +35,7 @@
>>  #include <linux/kernel.h>
>>  #include <linux/syscore_ops.h>
>>  #include <linux/dma-map-ops.h>
>> +#include <linux/pci.h>
>>  #include <clocksource/hyperv_timer.h>
>>  #include "hyperv_vmbus.h"
>> 
>> @@ -2262,26 +2263,43 @@ static int vmbus_acpi_remove(struct acpi_device *device)
>> 
>>  static void vmbus_reserve_fb(void)
>>  {
>> -	int size;
>> +	resource_size_t start = 0, size;
>> +	struct pci_dev *pdev;
>> +
>> +	if (efi_enabled(EFI_BOOT)) {
>> +		/* Gen2 VM: get FB base from EFI framebuffer */
>> +		start = screen_info.lfb_base;
>> +		size = max_t(__u32, screen_info.lfb_size, 0x800000);
>> +	} else {
>> +		/* Gen1 VM: get FB base from PCI */
>> +		pdev = pci_get_device(PCI_VENDOR_ID_MICROSOFT,
>> +				      PCI_DEVICE_ID_HYPERV_VIDEO, NULL);
>> +		if (!pdev)
>> +			return;
>> +
>> +		if (pdev->resource[0].flags & IORESOURCE_MEM) {
>> +			start = pci_resource_start(pdev, 0);
>> +			size = pci_resource_len(pdev, 0);
>> +		}
>> +
>> +		/*
>> +		 * Release the PCI device so hyperv_drm or hyperv_fb driver can
>> +		 * grab it later.
>> +		 */
>> +		pci_dev_put(pdev);
>> +	}
>> +
>> +	if (!start)
>> +		return;
>> +
>>  	/*
>>  	 * Make a claim for the frame buffer in the resource tree under the
>>  	 * first node, which will be the one below 4GB.  The length seems to
>>  	 * be underreported, particularly in a Generation 1 VM.  So start out
>>  	 * reserving a larger area and make it smaller until it succeeds.
>>  	 */
>> -
>> -	if (screen_info.lfb_base) {
>> -		if (efi_enabled(EFI_BOOT))
>> -			size = max_t(__u32, screen_info.lfb_size, 0x800000);
>> -		else
>> -			size = max_t(__u32, screen_info.lfb_size, 0x4000000);
>> -
>> -		for (; !fb_mmio && (size >= 0x100000); size >>= 1) {
>> -			fb_mmio = __request_region(hyperv_mmio,
>> -						   screen_info.lfb_base, size,
>> -						   fb_mmio_name, 0);
>> -		}
>> -	}
>> +	for (; !fb_mmio && (size >= 0x100000); size >>= 1)
>> +		fb_mmio = __request_region(hyperv_mmio, start, size, fb_mmio_name, 0);
>>  }
>> 
>>  /**
>> --
>> 2.37.1
>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
>

Thanks!

-- 
Vitaly

RE: [PATCH v2 3/3] Drivers: hv: Never allocate anything besides framebuffer from framebuffer memory region

[Vitaly Kuznetsov]

"Michael Kelley (LINUX)" <mikelley@microsoft.com> writes:

> From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Thursday, August 25, 2022 2:00 AM
>> 
>> Passed through PCI device sometimes misbehave on Gen1 VMs when Hyper-V
>> DRM driver is also loaded. Looking at IOMEM assignment, we can see e.g.
>> 
>> $ cat /proc/iomem
>> ...
>> f8000000-fffbffff : PCI Bus 0000:00
>>   f8000000-fbffffff : 0000:00:08.0
>>     f8000000-f8001fff : bb8c4f33-2ba2-4808-9f7f-02f3b4da22fe
>> ...
>> fe0000000-fffffffff : PCI Bus 0000:00
>>   fe0000000-fe07fffff : bb8c4f33-2ba2-4808-9f7f-02f3b4da22fe
>>     fe0000000-fe07fffff : 2ba2:00:02.0
>>       fe0000000-fe07fffff : mlx4_core
>> 
>> the interesting part is the 'f8000000' region as it is actually the
>> VM's framebuffer:
>> 
>> $ lspci -v
>> ...
>> 0000:00:08.0 VGA compatible controller: Microsoft Corporation Hyper-V virtual VGA
>> (prog-if 00 [VGA controller])
>> 	Flags: bus master, fast devsel, latency 0, IRQ 11
>> 	Memory at f8000000 (32-bit, non-prefetchable) [size=64M]
>> ...
>> 
>>  hv_vmbus: registering driver hyperv_drm
>>  hyperv_drm 5620e0c7-8062-4dce-aeb7-520c7ef76171: [drm] Synthvid Version major 3, minor 5
>>  hyperv_drm 0000:00:08.0: vgaarb: deactivate vga console
>>  hyperv_drm 0000:00:08.0: BAR 0: can't reserve [mem 0xf8000000-0xfbffffff]
>>  hyperv_drm 5620e0c7-8062-4dce-aeb7-520c7ef76171: [drm] Cannot request framebuffer, boot fb still active?
>> 
>> Note: "Cannot request framebuffer" is not a fatal error in
>> hyperv_setup_gen1() as the code assumes there's some other framebuffer
>> device there but we actually have some other PCI device (mlx4 in this
>> case) config space there!
>
> My apologies for not getting around to commenting on the previous
> version of this patch.  The function hyperv_setup_gen1() and the
> "Cannot request framebuffer" message have gone away as of
> commit a0ab5abced55.
>

True, will fix!

>> 
>> The problem appears to be that vmbus_allocate_mmio() can allocate from
>> the reserved framebuffer region (fb_overlap_ok), however, if the
>> request to allocate MMIO comes from some other device before
>> framebuffer region is taken, it can happily use framebuffer region for
>> it. 
>
> Interesting. I had never looked at the details of vmbus_allocate_mmio().
> The semantics one might assume of a parameter named "fb_overlap_ok"
> aren't implemented because !fb_overlap_ok essentially has no effect.   The
> existing semantics are really "prefer_fb_overlap".  This patch implements
> the expected and needed semantics, which is to not allocate from the frame
> buffer space when !fb_overlap_ok.
>
> If that's an accurate high level summary, maybe this commit message
> could describe it that way?  The other details you provide about what can
> go wrong should still be included as well.

That's acually a very good summary! Let me update the commit message,
I'll be sending out v3 shortly.

>
>> Note, Gen2 VMs are usually unaffected by the issue because
>> framebuffer region is already taken by EFI fb (in case kernel supports
>> it) but Gen1 VMs may have this region unclaimed by the time Hyper-V PCI
>> pass-through driver tries allocating MMIO space if Hyper-V DRM/FB drivers
>> load after it. Devices can be brought up in any sequence so let's
>> resolve the issue by always ignoring 'fb_mmio' region for non-FB
>> requests, even if the region is unclaimed.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  drivers/hv/vmbus_drv.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
>> index 536f68e563c6..3c833ea60db6 100644
>> --- a/drivers/hv/vmbus_drv.c
>> +++ b/drivers/hv/vmbus_drv.c
>> @@ -2331,7 +2331,7 @@ int vmbus_allocate_mmio(struct resource **new, struct
>> hv_device *device_obj,
>>  			bool fb_overlap_ok)
>>  {
>>  	struct resource *iter, *shadow;
>> -	resource_size_t range_min, range_max, start;
>> +	resource_size_t range_min, range_max, start, end;
>>  	const char *dev_n = dev_name(&device_obj->device);
>>  	int retval;
>> 
>> @@ -2366,6 +2366,14 @@ int vmbus_allocate_mmio(struct resource **new, struct
>> hv_device *device_obj,
>>  		range_max = iter->end;
>>  		start = (range_min + align - 1) & ~(align - 1);
>>  		for (; start + size - 1 <= range_max; start += align) {
>> +			end = start + size - 1;
>> +
>> +			/* Skip the whole fb_mmio region if not fb_overlap_ok */
>> +			if (!fb_overlap_ok && fb_mmio &&
>> +			    (((start >= fb_mmio->start) && (start <= fb_mmio->end)) ||
>> +			     ((end >= fb_mmio->start) && (end <= fb_mmio->end))))
>> +				continue;
>> +
>>  			shadow = __request_region(iter, start, size, NULL,
>>  						  IORESOURCE_BUSY);
>>  			if (!shadow)
>> --
>> 2.37.1
>
> Other than my musings on the commit message,
>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
>

Thanks!

-- 
Vitaly