[PATCH] video: hyperv_fb: Allow resolutions with size > 64 MB for Gen1

[PATCH] video: hyperv_fb: Allow resolutions with size > 64 MB for Gen1

[Saurabh Sengar]

This patch fixes a bug where GEN1 VMs doesn't allow resolutions greater
than 64 MB size (eg 7680x4320). Unnecessary PCI check limits Gen1 VRAM
to legacy PCI BAR size only (ie 64MB). Thus any, resolution requesting
greater then 64MB (eg 7680x4320) would fail. MMIO region assigning this
memory shouldn't be limited by PCI bar size.

Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
---
 drivers/video/fbdev/hyperv_fb.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index c8e0ea2..58c304a 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -1009,7 +1009,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
 	struct pci_dev *pdev  = NULL;
 	void __iomem *fb_virt;
 	int gen2vm = efi_enabled(EFI_BOOT);
-	resource_size_t pot_start, pot_end;
 	phys_addr_t paddr;
 	int ret;
 
@@ -1060,23 +1059,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
 	dio_fb_size =
 		screen_width * screen_height * screen_depth / 8;
 
-	if (gen2vm) {
-		pot_start = 0;
-		pot_end = -1;
-	} else {
-		if (!(pci_resource_flags(pdev, 0) & IORESOURCE_MEM) ||
-		    pci_resource_len(pdev, 0) < screen_fb_size) {
-			pr_err("Resource not available or (0x%lx < 0x%lx)\n",
-			       (unsigned long) pci_resource_len(pdev, 0),
-			       (unsigned long) screen_fb_size);
-			goto err1;
-		}
-
-		pot_end = pci_resource_end(pdev, 0);
-		pot_start = pot_end - screen_fb_size + 1;
-	}
-
-	ret = vmbus_allocate_mmio(&par->mem, hdev, pot_start, pot_end,
+	ret = vmbus_allocate_mmio(&par->mem, hdev, 0, -1,
 				  screen_fb_size, 0x100000, true);
 	if (ret != 0) {
 		pr_err("Unable to allocate framebuffer memory\n");
-- 
1.8.3.1

Re: [PATCH] video: hyperv_fb: Allow resolutions with size > 64 MB for Gen1

[Helge Deller]

On 4/27/22 15:47, Saurabh Sengar wrote:
> This patch fixes a bug where GEN1 VMs doesn't allow resolutions greater
> than 64 MB size (eg 7680x4320). Unnecessary PCI check limits Gen1 VRAM
> to legacy PCI BAR size only (ie 64MB). Thus any, resolution requesting
> greater then 64MB (eg 7680x4320) would fail. MMIO region assigning this
> memory shouldn't be limited by PCI bar size.

Is that right?
Allocating more memory than what the PCI bar states?
That sounds to me that theoretically the "now bigger" memory framebuffer could
overwrite other memory areas or mapped PCI bars.
I'd like to see some other person from Microsoft to please comment/ack/nack.

Helge

> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
>  drivers/video/fbdev/hyperv_fb.c | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
>
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index c8e0ea2..58c304a 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -1009,7 +1009,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
>  	struct pci_dev *pdev  = NULL;
>  	void __iomem *fb_virt;
>  	int gen2vm = efi_enabled(EFI_BOOT);
> -	resource_size_t pot_start, pot_end;
>  	phys_addr_t paddr;
>  	int ret;
>
> @@ -1060,23 +1059,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
>  	dio_fb_size =
>  		screen_width * screen_height * screen_depth / 8;
>
> -	if (gen2vm) {
> -		pot_start = 0;
> -		pot_end = -1;
> -	} else {
> -		if (!(pci_resource_flags(pdev, 0) & IORESOURCE_MEM) ||
> -		    pci_resource_len(pdev, 0) < screen_fb_size) {
> -			pr_err("Resource not available or (0x%lx < 0x%lx)\n",
> -			       (unsigned long) pci_resource_len(pdev, 0),
> -			       (unsigned long) screen_fb_size);
> -			goto err1;
> -		}
> -
> -		pot_end = pci_resource_end(pdev, 0);
> -		pot_start = pot_end - screen_fb_size + 1;
> -	}
> -
> -	ret = vmbus_allocate_mmio(&par->mem, hdev, pot_start, pot_end,
> +	ret = vmbus_allocate_mmio(&par->mem, hdev, 0, -1,
>  				  screen_fb_size, 0x100000, true);
>  	if (ret != 0) {
>  		pr_err("Unable to allocate framebuffer memory\n");

Re: [PATCH] video: hyperv_fb: Allow resolutions with size > 64 MB for Gen1

[Wei Liu]

On Wed, Apr 27, 2022 at 06:47:53AM -0700, Saurabh Sengar wrote:
> This patch fixes a bug where GEN1 VMs doesn't allow resolutions greater
> than 64 MB size (eg 7680x4320). Unnecessary PCI check limits Gen1 VRAM
> to legacy PCI BAR size only (ie 64MB). Thus any, resolution requesting
> greater then 64MB (eg 7680x4320) would fail. MMIO region assigning this
> memory shouldn't be limited by PCI bar size.
> 
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> ---
>  drivers/video/fbdev/hyperv_fb.c | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
> 
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index c8e0ea2..58c304a 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -1009,7 +1009,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
>  	struct pci_dev *pdev  = NULL;
>  	void __iomem *fb_virt;
>  	int gen2vm = efi_enabled(EFI_BOOT);
> -	resource_size_t pot_start, pot_end;
>  	phys_addr_t paddr;
>  	int ret;
>  
> @@ -1060,23 +1059,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
>  	dio_fb_size =
>  		screen_width * screen_height * screen_depth / 8;
>  
> -	if (gen2vm) {
> -		pot_start = 0;
> -		pot_end = -1;
> -	} else {
> -		if (!(pci_resource_flags(pdev, 0) & IORESOURCE_MEM) ||
> -		    pci_resource_len(pdev, 0) < screen_fb_size) {
> -			pr_err("Resource not available or (0x%lx < 0x%lx)\n",
> -			       (unsigned long) pci_resource_len(pdev, 0),
> -			       (unsigned long) screen_fb_size);
> -			goto err1;

This restriction has been in place since day 1. Haiyang, you wrote this
driver. Can you comment on whether this change here is sensible?

Thanks,
Wei.

> -		}
> -
> -		pot_end = pci_resource_end(pdev, 0);
> -		pot_start = pot_end - screen_fb_size + 1;
> -	}
> -
> -	ret = vmbus_allocate_mmio(&par->mem, hdev, pot_start, pot_end,
> +	ret = vmbus_allocate_mmio(&par->mem, hdev, 0, -1,
>  				  screen_fb_size, 0x100000, true);
>  	if (ret != 0) {
>  		pr_err("Unable to allocate framebuffer memory\n");
> -- 
> 1.8.3.1
> 

RE: [PATCH] video: hyperv_fb: Allow resolutions with size > 64 MB for Gen1

[Haiyang Zhang]

> -----Original Message-----
> From: Wei Liu <wei.liu@kernel.org>
> Sent: Thursday, April 28, 2022 10:38 AM
> To: Saurabh Sengar <ssengar@linux.microsoft.com>
> Cc: Saurabh Singh Sengar <ssengar@microsoft.com>; KY Srinivasan
> <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>; Stephen
> Hemminger <sthemmin@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> <decui@microsoft.com>; deller@gmx.de; linux-hyperv@vger.kernel.org; linux-
> fbdev@vger.kernel.org; dri-devel@lists.freedesktop.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] video: hyperv_fb: Allow resolutions with size > 64 MB for
> Gen1
> 
> On Wed, Apr 27, 2022 at 06:47:53AM -0700, Saurabh Sengar wrote:
> > This patch fixes a bug where GEN1 VMs doesn't allow resolutions greater
> > than 64 MB size (eg 7680x4320). Unnecessary PCI check limits Gen1 VRAM
> > to legacy PCI BAR size only (ie 64MB). Thus any, resolution requesting
> > greater then 64MB (eg 7680x4320) would fail. MMIO region assigning this
> > memory shouldn't be limited by PCI bar size.
> >
> > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> > ---
> >  drivers/video/fbdev/hyperv_fb.c | 19 +------------------
> >  1 file changed, 1 insertion(+), 18 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/hyperv_fb.c
> b/drivers/video/fbdev/hyperv_fb.c
> > index c8e0ea2..58c304a 100644
> > --- a/drivers/video/fbdev/hyperv_fb.c
> > +++ b/drivers/video/fbdev/hyperv_fb.c
> > @@ -1009,7 +1009,6 @@ static int hvfb_getmem(struct hv_device *hdev,
> struct fb_info *info)
> >  	struct pci_dev *pdev  = NULL;
> >  	void __iomem *fb_virt;
> >  	int gen2vm = efi_enabled(EFI_BOOT);
> > -	resource_size_t pot_start, pot_end;
> >  	phys_addr_t paddr;
> >  	int ret;
> >
> > @@ -1060,23 +1059,7 @@ static int hvfb_getmem(struct hv_device *hdev,
> struct fb_info *info)
> >  	dio_fb_size =
> >  		screen_width * screen_height * screen_depth / 8;
> >
> > -	if (gen2vm) {
> > -		pot_start = 0;
> > -		pot_end = -1;
> > -	} else {
> > -		if (!(pci_resource_flags(pdev, 0) & IORESOURCE_MEM) ||
> > -		    pci_resource_len(pdev, 0) < screen_fb_size) {
> > -			pr_err("Resource not available or (0x%lx < 0x%lx)\n",
> > -			       (unsigned long) pci_resource_len(pdev, 0),
> > -			       (unsigned long) screen_fb_size);
> > -			goto err1;
> 
> This restriction has been in place since day 1. Haiyang, you wrote this
> driver. Can you comment on whether this change here is sensible?

When I initially implemented this driver 10 years ago, I believe there 
was smaller limit for the fb... But I think this patch is good for the 
newer MMIO alloc scheme. I hope to see reviews also from
 @Dexuan Cui @Michael Kelley (LINUX) who are more familiar with 
the PCI/BAR/MMIO area.

Thanks,
- Haiyang

RE: [PATCH] video: hyperv_fb: Allow resolutions with size > 64 MB for Gen1

[Dexuan Cui]

> From: Haiyang Zhang <haiyangz@microsoft.com>
> > > ...
> > > -	if (gen2vm) {
> > > -		pot_start = 0;
> > > -		pot_end = -1;
> > > -	} else {
> > > -		if (!(pci_resource_flags(pdev, 0) & IORESOURCE_MEM) ||
> > > -		    pci_resource_len(pdev, 0) < screen_fb_size) {
> > > -			pr_err("Resource not available or (0x%lx < 0x%lx)\n",
> > > -			       (unsigned long) pci_resource_len(pdev, 0),
> > > -			       (unsigned long) screen_fb_size);
> > > -			goto err1;
> >
> > This restriction has been in place since day 1. Haiyang, you wrote this
> > driver. Can you comment on whether this change here is sensible?
> 
> When I initially implemented this driver 10 years ago, I believe there
> was smaller limit for the fb... But I think this patch is good for the
> newer MMIO alloc scheme. I hope to see reviews also from
>  @Dexuan Cui @Michael Kelley (LINUX) who are more familiar with
> the PCI/BAR/MMIO area.
> 
> Thanks,
> - Haiyang

The patch looks good to me but I suggest we check with the Hyper-V
team to figure out how a Gen1 Windows VM supports a higher
resolution that needs a VRAM size of more than 64MB. Just in case we
miss something..

Thanks,
-- Dexuan

RE: [PATCH] video: hyperv_fb: Allow resolutions with size > 64 MB for Gen1

[Dexuan Cui]

> From: Dexuan Cui <decui@microsoft.com>
> Sent: Wednesday, May 4, 2022 10:05 AM
> To: Haiyang Zhang <haiyangz@microsoft.com>; Wei Liu <wei.liu@kernel.org>;
> > ...
> > When I initially implemented this driver 10 years ago, I believe there
> > was smaller limit for the fb... But I think this patch is good for the
> > newer MMIO alloc scheme. I hope to see reviews also from
> >  @Dexuan Cui @Michael Kelley (LINUX) who are more familiar with
> > the PCI/BAR/MMIO area.
> >
> > Thanks,
> > - Haiyang
> 
> The patch looks good to me but I suggest we check with the Hyper-V
> team to figure out how a Gen1 Windows VM supports a higher
> resolution that needs a VRAM size of more than 64MB. Just in case we
> miss something..
> 
> Thanks,
> -- Dexuan

Reviewed-by: Dexuan Cui <decui@microsoft.com>

Saurabh checked this with Hyper-V team, who said there is no
Generation 1-specific block for larger VRAM sizes in Windows VM.

When the driver was originally developed, we didn't have the API
vmbus_allocate_mmio(), and I guess we just used the PCI device's BAR
address for simplicity, and didn't realize the restriction with very high
resolutions that require >64 MB VRAM. It looks like the synthetic
VMBus framebuffer device doesn't have to use the same MMIO range
used by the Hyper-V legacy PCI framebuffer device, so the patch
looks good to me.

BTW, please check the hyperv-drm driver as well:
drivers/gpu/drm/hyperv/hyperv_drm_drv.c
I think we should make the same change there to support 7680x4320
for Gen1 VMs.

Thanks,
Dexuan

Re: [PATCH] video: hyperv_fb: Allow resolutions with size > 64 MB for Gen1

[Helge Deller]

On 5/18/22 20:48, Dexuan Cui wrote:
>> From: Dexuan Cui <decui@microsoft.com>
>> Sent: Wednesday, May 4, 2022 10:05 AM
>> To: Haiyang Zhang <haiyangz@microsoft.com>; Wei Liu <wei.liu@kernel.org>;
>>> ...
>>> When I initially implemented this driver 10 years ago, I believe there
>>> was smaller limit for the fb... But I think this patch is good for the
>>> newer MMIO alloc scheme. I hope to see reviews also from
>>>  @Dexuan Cui @Michael Kelley (LINUX) who are more familiar with
>>> the PCI/BAR/MMIO area.
>>>
>>> Thanks,
>>> - Haiyang
>>
>> The patch looks good to me but I suggest we check with the Hyper-V
>> team to figure out how a Gen1 Windows VM supports a higher
>> resolution that needs a VRAM size of more than 64MB. Just in case we
>> miss something..
>>
>> Thanks,
>> -- Dexuan
>
> Reviewed-by: Dexuan Cui <decui@microsoft.com>
>
> Saurabh checked this with Hyper-V team, who said there is no
> Generation 1-specific block for larger VRAM sizes in Windows VM.
>
> When the driver was originally developed, we didn't have the API
> vmbus_allocate_mmio(), and I guess we just used the PCI device's BAR
> address for simplicity, and didn't realize the restriction with very high
> resolutions that require >64 MB VRAM. It looks like the synthetic
> VMBus framebuffer device doesn't have to use the same MMIO range
> used by the Hyper-V legacy PCI framebuffer device, so the patch
> looks good to me.

Thanks for the review, Dexuan!

I've applied this patch now to the fbdev git tree.

> BTW, please check the hyperv-drm driver as well:
> drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> I think we should make the same change there to support 7680x4320
> for Gen1 VMs.

Haiyang, can you check that as well and send another patch for
the drm tree ?

Helge