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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

"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