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

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

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

Resolve the issue by always reserving FB region on Gen1 VMs (PATCH3) and making
sure we never allocate anything besides framebuffer from there (PATCH4). PATCH1
is a preparatory change, PATCH2 fixes a loosely related issue in Hyper-V DRM
driver.

Vitaly Kuznetsov (4):
  Drivers: hv: Move legacy Hyper-V PCI video device's ids to
    linux/hyperv.h
  drm/hyperv: Don't forget to put PCI device when removing conflicting
    FB fails
  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 |  5 +--
 drivers/hv/vmbus_drv.c                  | 57 ++++++++++++++++++-------
 drivers/video/fbdev/hyperv_fb.c         |  4 --
 include/linux/hyperv.h                  |  4 ++
 4 files changed, 47 insertions(+), 23 deletions(-)

-- 
2.37.1

[PATCH v1 1/4] Drivers: hv: Move legacy Hyper-V PCI video device's ids to linux/hyperv.h

[Vitaly Kuznetsov]

There are already two places in kernel with PCI_VENDOR_ID_MICROSOFT/
PCI_DEVICE_ID_HYPERV_VIDEO and there's a need to use these from core
Vmbus code. Move the defines to a common header.

No functional change.

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

diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
index 4a8941fa0815..46f6c454b820 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/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/hyperv.h b/include/linux/hyperv.h
index 3b42264333ef..4bb39a8f1af7 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1516,6 +1516,10 @@ void vmbus_free_mmio(resource_size_t start, resource_size_t size);
 	.guid = GUID_INIT(0xc376c1c3, 0xd276, 0x48d2, 0x90, 0xa9, \
 			  0xc0, 0x47, 0x48, 0x07, 0x2c, 0x60)
 
+/* Legacy Hyper-V PCI video device */
+#define PCI_VENDOR_ID_MICROSOFT 0x1414
+#define PCI_DEVICE_ID_HYPERV_VIDEO 0x5353
+
 /*
  * Common header for Hyper-V ICs
  */
-- 
2.37.1

[PATCH v1 2/4] drm/hyperv: Don't forget to put PCI device when removing conflicting FB fails

[Vitaly Kuznetsov]

When drm_aperture_remove_conflicting_pci_framebuffers() fails, 'pdev'
needs to be released with pci_dev_put().

Fixes: 76c56a5affeb ("drm/hyperv: Add DRM driver for hyperv synthetic video device")
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/gpu/drm/hyperv/hyperv_drm_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
index 46f6c454b820..ca4e517b95ca 100644
--- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
+++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
@@ -82,7 +82,7 @@ static int hyperv_setup_gen1(struct hyperv_drm_device *hv)
 	ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &hyperv_driver);
 	if (ret) {
 		drm_err(dev, "Not able to remove boot fb\n");
-		return ret;
+		goto error;
 	}
 
 	if (pci_request_region(pdev, 0, DRIVER_NAME) != 0)
-- 
2.37.1

[PATCH v1 3/4] 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
but on Gen1 VM it is observed to be zero. In fact, we do not need to
rely on some other video driver setting it correctly as Gen1 VMs have
a dedicated PCI device to look at. Both Hyper-V DRM and Hyper-V FB
drivers get framebuffer base from this PCI device already so Vmbus
driver can do the same trick.

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

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

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 547ae334e5cd..6edaeefa2c3c 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"
 
@@ -2258,26 +2259,44 @@ 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))
+			return;
+
+		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 v1 4/4] 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 6edaeefa2c3c..54ace5c6b990 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -2328,7 +2328,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;
 
@@ -2363,6 +2363,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 v1 1/4] Drivers: hv: Move legacy Hyper-V PCI video device's ids to linux/hyperv.h

[Michael Kelley (LINUX)]

From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Thursday, August 18, 2022 7:25 AM
> 
> There are already two places in kernel with PCI_VENDOR_ID_MICROSOFT/
> PCI_DEVICE_ID_HYPERV_VIDEO and there's a need to use these from core
> Vmbus code. Move the defines to a common header.
> 
> No functional change.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  drivers/gpu/drm/hyperv/hyperv_drm_drv.c | 3 ---
>  drivers/video/fbdev/hyperv_fb.c         | 4 ----
>  include/linux/hyperv.h                  | 4 ++++
>  3 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> index 4a8941fa0815..46f6c454b820 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/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/hyperv.h b/include/linux/hyperv.h
> index 3b42264333ef..4bb39a8f1af7 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1516,6 +1516,10 @@ void vmbus_free_mmio(resource_size_t start,
> resource_size_t size);
>  	.guid = GUID_INIT(0xc376c1c3, 0xd276, 0x48d2, 0x90, 0xa9, \
>  			  0xc0, 0x47, 0x48, 0x07, 0x2c, 0x60)
> 
> +/* Legacy Hyper-V PCI video device */
> +#define PCI_VENDOR_ID_MICROSOFT 0x1414
> +#define PCI_DEVICE_ID_HYPERV_VIDEO 0x5353

I've never looked at this before, but shouldn't these move to
include/linux/pci_ids.h with all the others?  And we've got
another #define of PCI_VENDOR_ID_MICROSOFT in
drivers/net/ethernet/microsoft/mana/gdma_main.c that
could be deleted.

Michael

> +
>  /*
>   * Common header for Hyper-V ICs
>   */
> --
> 2.37.1

RE: [PATCH v1 2/4] drm/hyperv: Don't forget to put PCI device when removing conflicting FB fails

[Michael Kelley (LINUX)]

From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Thursday, August 18, 2022 7:25 AM
> 
> When drm_aperture_remove_conflicting_pci_framebuffers() fails, 'pdev'
> needs to be released with pci_dev_put().
> 
> Fixes: 76c56a5affeb ("drm/hyperv: Add DRM driver for hyperv synthetic video device")
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  drivers/gpu/drm/hyperv/hyperv_drm_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> index 46f6c454b820..ca4e517b95ca 100644
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> @@ -82,7 +82,7 @@ static int hyperv_setup_gen1(struct hyperv_drm_device *hv)
>  	ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev,
> &hyperv_driver);
>  	if (ret) {
>  		drm_err(dev, "Not able to remove boot fb\n");
> -		return ret;
> +		goto error;
>  	}
> 
>  	if (pci_request_region(pdev, 0, DRIVER_NAME) != 0)
> --
> 2.37.1

This patch appears to be obsoleted by commit a0ab5abced55
that was merged into 6.0-rc1.  Of course, it does beg the question of
why the original function hyperv_setup_gen2(), which is now renamed
to hyperv_setup_vram(), doesn't check the return value from
drm_aperture_remove_conflicting_framebuffers().


Michael

RE: [PATCH v1 1/4] Drivers: hv: Move legacy Hyper-V PCI video device's ids to linux/hyperv.h

[Vitaly Kuznetsov]

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

> From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Thursday, August 18, 2022 7:25 AM
>> 
>> There are already two places in kernel with PCI_VENDOR_ID_MICROSOFT/
>> PCI_DEVICE_ID_HYPERV_VIDEO and there's a need to use these from core
>> Vmbus code. Move the defines to a common header.
>> 
>> No functional change.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  drivers/gpu/drm/hyperv/hyperv_drm_drv.c | 3 ---
>>  drivers/video/fbdev/hyperv_fb.c         | 4 ----
>>  include/linux/hyperv.h                  | 4 ++++
>>  3 files changed, 4 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
>> b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
>> index 4a8941fa0815..46f6c454b820 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/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/hyperv.h b/include/linux/hyperv.h
>> index 3b42264333ef..4bb39a8f1af7 100644
>> --- a/include/linux/hyperv.h
>> +++ b/include/linux/hyperv.h
>> @@ -1516,6 +1516,10 @@ void vmbus_free_mmio(resource_size_t start,
>> resource_size_t size);
>>  	.guid = GUID_INIT(0xc376c1c3, 0xd276, 0x48d2, 0x90, 0xa9, \
>>  			  0xc0, 0x47, 0x48, 0x07, 0x2c, 0x60)
>> 
>> +/* Legacy Hyper-V PCI video device */
>> +#define PCI_VENDOR_ID_MICROSOFT 0x1414
>> +#define PCI_DEVICE_ID_HYPERV_VIDEO 0x5353
>
> I've never looked at this before, but shouldn't these move to
> include/linux/pci_ids.h with all the others?  And we've got
> another #define of PCI_VENDOR_ID_MICROSOFT in
> drivers/net/ethernet/microsoft/mana/gdma_main.c that
> could be deleted.
>

Oh, true, include/linux/pci_ids.h is much better. Let me send an updated
patch (detached from this series).

-- 
Vitaly

RE: [PATCH v1 2/4] drm/hyperv: Don't forget to put PCI device when removing conflicting FB fails

[Vitaly Kuznetsov]

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

> From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Thursday, August 18, 2022 7:25 AM
>> 
>> When drm_aperture_remove_conflicting_pci_framebuffers() fails, 'pdev'
>> needs to be released with pci_dev_put().
>> 
>> Fixes: 76c56a5affeb ("drm/hyperv: Add DRM driver for hyperv synthetic video device")
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  drivers/gpu/drm/hyperv/hyperv_drm_drv.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
>> b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
>> index 46f6c454b820..ca4e517b95ca 100644
>> --- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
>> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
>> @@ -82,7 +82,7 @@ static int hyperv_setup_gen1(struct hyperv_drm_device *hv)
>>  	ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev,
>> &hyperv_driver);
>>  	if (ret) {
>>  		drm_err(dev, "Not able to remove boot fb\n");
>> -		return ret;
>> +		goto error;
>>  	}
>> 
>>  	if (pci_request_region(pdev, 0, DRIVER_NAME) != 0)
>> --
>> 2.37.1
>
> This patch appears to be obsoleted by commit a0ab5abced55
> that was merged into 6.0-rc1.  Of course, it does beg the question of
> why the original function hyperv_setup_gen2(), which is now renamed
> to hyperv_setup_vram(), doesn't check the return value from
> drm_aperture_remove_conflicting_framebuffers().

AFAICT this commit (which I've obviously missed) also solves the worst
issue I'm trying to address with this series: conflict between
framebuffer and SR-IOV VF config space. It would probably still make
sense to reserve the whole FB region on Gen1 first thing and use it
as-is for DRM/FB later (Patches 3-4).

-- 
Vitaly

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

[Michael Kelley (LINUX)]

From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Thursday, August 18, 2022 7:25 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
> but on Gen1 VM it is observed to be zero. 

FWIW, in a Gen1 VM, whether screen_info.lfb_base is set depends on what
grub sets up, which in turn seems to depend on the gfxpayload= setting
in grub.cfg and certain versions of grub.  There are cases where it is
observed to be zero, but from our experiments it's not all cases.

In a Gen2 VM, there's an edge case where the frame buffer has been
moved, and a kexec() kernel may see the moved location instead of
what was set by EFI.  See
https://lore.kernel.org/all/20201014092429.1415040-1-kasong@redhat.com/

I think these points may be worth recording in the commit message here
so that there's accurate record for the future.  The Hyper-V and grub
idiosyncrasies make this a very tricky area.

> In fact, we do not need to
> rely on some other video driver setting it correctly as Gen1 VMs have
> a dedicated PCI device to look at. Both Hyper-V DRM and Hyper-V FB
> drivers get framebuffer base from this PCI device already so Vmbus
> driver can do the same trick.
> 
> Check for legacy PCI video device presence and reserve the whole
> region for framebuffer.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  drivers/hv/vmbus_drv.c | 47 +++++++++++++++++++++++++++++-------------
>  1 file changed, 33 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 547ae334e5cd..6edaeefa2c3c 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"
> 
> @@ -2258,26 +2259,44 @@ 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))
> +			return;

Doesn't this error exit need a pci_dev_put(pdev)?  Or maybe reverse
the test like this, and the later check for !start will do the error exit.

		if (pdev->resource[0].flags & IORESOURCE_MEM) {
			start = pci_resource_start(pdev, 0);
			size = pci_resource_len(pdev, 0);
		}

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