[PATCH] xen/x86: obtain upper 32 bits of video frame buffer address for Dom0

Jan Beulich posted 1 patch 3 weeks, 2 days ago
Failed in applying to current master (apply log)

[PATCH] xen/x86: obtain upper 32 bits of video frame buffer address for Dom0

Posted by Jan Beulich 3 weeks, 2 days ago
The hypervisor has been supplying this information for a couple of major
releases. Make use of it. The need to set a flag in the capabilities
field also points out that the prior setting of that field from the
hypervisor interface's gbl_caps one was wrong, so that code gets deleted
(there's also no equivalent of this in native boot code).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/arch/x86/xen/vga.c
+++ b/arch/x86/xen/vga.c
@@ -63,13 +63,17 @@ void __init xen_init_vga(const struct do
 		}
 
 		if (size >= offsetof(struct dom0_vga_console_info,
-				     u.vesa_lfb.gbl_caps)
-		    + sizeof(info->u.vesa_lfb.gbl_caps))
-			screen_info->capabilities = info->u.vesa_lfb.gbl_caps;
-		if (size >= offsetof(struct dom0_vga_console_info,
 				     u.vesa_lfb.mode_attrs)
 		    + sizeof(info->u.vesa_lfb.mode_attrs))
 			screen_info->vesa_attributes = info->u.vesa_lfb.mode_attrs;
+
+		if (size >= offsetof(struct dom0_vga_console_info,
+				     u.vesa_lfb.ext_lfb_base)
+		    + sizeof(info->u.vesa_lfb.ext_lfb_base)
+		    && info->u.vesa_lfb.ext_lfb_base) {
+			screen_info->ext_lfb_base = info->u.vesa_lfb.ext_lfb_base;
+			screen_info->capabilities |= VIDEO_CAPABILITY_64BIT_BASE;
+		}
 		break;
 	}
 }
--- a/include/xen/interface/xen.h
+++ b/include/xen/interface/xen.h
@@ -722,6 +722,9 @@ struct dom0_vga_console_info {
 			uint32_t gbl_caps;
 			/* Mode attributes (offset 0x0, VESA command 0x4f01). */
 			uint16_t mode_attrs;
+			uint16_t pad;
+			/* high 32 bits of lfb_base */
+			uint32_t ext_lfb_base;
 		} vesa_lfb;
 	} u;
 };


Re: [PATCH] xen/x86: obtain upper 32 bits of video frame buffer address for Dom0

Posted by Juergen Gross 2 weeks, 6 days ago
On 04.01.22 09:46, Jan Beulich wrote:
> The hypervisor has been supplying this information for a couple of major
> releases. Make use of it. The need to set a flag in the capabilities
> field also points out that the prior setting of that field from the
> hypervisor interface's gbl_caps one was wrong, so that code gets deleted
> (there's also no equivalent of this in native boot code).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Pushed to xen/tip.git for-linus-5.17


Juergen

Re: [PATCH] xen/x86: obtain upper 32 bits of video frame buffer address for Dom0

Posted by Boris Ostrovsky 3 weeks, 2 days ago
On 1/4/22 3:46 AM, Jan Beulich wrote:
> The hypervisor has been supplying this information for a couple of major
> releases. Make use of it. The need to set a flag in the capabilities
> field also points out that the prior setting of that field from the
> hypervisor interface's gbl_caps one was wrong, so that code gets deleted
> (there's also no equivalent of this in native boot code).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/arch/x86/xen/vga.c
> +++ b/arch/x86/xen/vga.c
> @@ -63,13 +63,17 @@ void __init xen_init_vga(const struct do
>   		}
>   
>   		if (size >= offsetof(struct dom0_vga_console_info,
> -				     u.vesa_lfb.gbl_caps)
> -		    + sizeof(info->u.vesa_lfb.gbl_caps))
> -			screen_info->capabilities = info->u.vesa_lfb.gbl_caps;
> -		if (size >= offsetof(struct dom0_vga_console_info,
>   				     u.vesa_lfb.mode_attrs)
>   		    + sizeof(info->u.vesa_lfb.mode_attrs))


Do we still need this test? All 4.0+ hypervisors will have mode_attrs.


-boris



>   			screen_info->vesa_attributes = info->u.vesa_lfb.mode_attrs;
> +
> +		if (size >= offsetof(struct dom0_vga_console_info,
> +				     u.vesa_lfb.ext_lfb_base)
> +		    + sizeof(info->u.vesa_lfb.ext_lfb_base)
> +		    && info->u.vesa_lfb.ext_lfb_base) {
> +			screen_info->ext_lfb_base = info->u.vesa_lfb.ext_lfb_base;
> +			screen_info->capabilities |= VIDEO_CAPABILITY_64BIT_BASE;
> +		}
>   		break;
>   	}
>   }
> --- a/include/xen/interface/xen.h
> +++ b/include/xen/interface/xen.h
> @@ -722,6 +722,9 @@ struct dom0_vga_console_info {
>   			uint32_t gbl_caps;
>   			/* Mode attributes (offset 0x0, VESA command 0x4f01). */
>   			uint16_t mode_attrs;
> +			uint16_t pad;
> +			/* high 32 bits of lfb_base */
> +			uint32_t ext_lfb_base;
>   		} vesa_lfb;
>   	} u;
>   };
>

Re: [PATCH] xen/x86: obtain upper 32 bits of video frame buffer address for Dom0

Posted by Jan Beulich 3 weeks, 2 days ago
On 04.01.2022 17:50, Boris Ostrovsky wrote:
> 
> On 1/4/22 3:46 AM, Jan Beulich wrote:
>> The hypervisor has been supplying this information for a couple of major
>> releases. Make use of it. The need to set a flag in the capabilities
>> field also points out that the prior setting of that field from the
>> hypervisor interface's gbl_caps one was wrong, so that code gets deleted
>> (there's also no equivalent of this in native boot code).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/arch/x86/xen/vga.c
>> +++ b/arch/x86/xen/vga.c
>> @@ -63,13 +63,17 @@ void __init xen_init_vga(const struct do
>>   		}
>>   
>>   		if (size >= offsetof(struct dom0_vga_console_info,
>> -				     u.vesa_lfb.gbl_caps)
>> -		    + sizeof(info->u.vesa_lfb.gbl_caps))
>> -			screen_info->capabilities = info->u.vesa_lfb.gbl_caps;
>> -		if (size >= offsetof(struct dom0_vga_console_info,
>>   				     u.vesa_lfb.mode_attrs)
>>   		    + sizeof(info->u.vesa_lfb.mode_attrs))
> 
> 
> Do we still need this test? All 4.0+ hypervisors will have mode_attrs.

Perhaps this could also be dropped, but unlike the capabilities part
I'd view this as an unrelated change. Furthermore even a new hypervisor
would be free to omit the field, provided it also sets size low enough.

Jan


Re: [PATCH] xen/x86: obtain upper 32 bits of video frame buffer address for Dom0

Posted by Boris Ostrovsky 3 weeks, 2 days ago
On 1/4/22 11:54 AM, Jan Beulich wrote:
> On 04.01.2022 17:50, Boris Ostrovsky wrote:
>> On 1/4/22 3:46 AM, Jan Beulich wrote:
>>> The hypervisor has been supplying this information for a couple of major
>>> releases. Make use of it. The need to set a flag in the capabilities
>>> field also points out that the prior setting of that field from the
>>> hypervisor interface's gbl_caps one was wrong, so that code gets deleted
>>> (there's also no equivalent of this in native boot code).
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/arch/x86/xen/vga.c
>>> +++ b/arch/x86/xen/vga.c
>>> @@ -63,13 +63,17 @@ void __init xen_init_vga(const struct do
>>>    		}
>>>    
>>>    		if (size >= offsetof(struct dom0_vga_console_info,
>>> -				     u.vesa_lfb.gbl_caps)
>>> -		    + sizeof(info->u.vesa_lfb.gbl_caps))
>>> -			screen_info->capabilities = info->u.vesa_lfb.gbl_caps;
>>> -		if (size >= offsetof(struct dom0_vga_console_info,
>>>    				     u.vesa_lfb.mode_attrs)
>>>    		    + sizeof(info->u.vesa_lfb.mode_attrs))
>>
>> Do we still need this test? All 4.0+ hypervisors will have mode_attrs.
> Perhaps this could also be dropped, but unlike the capabilities part
> I'd view this as an unrelated change.


Right.


>   Furthermore even a new hypervisor
> would be free to omit the field, provided it also sets size low enough.


If this is allowed, how would we deal with hypervisor dropping some other random field here? Have we had a precedent of this happening?


I think it's safe to drop these checks. In fact, most of them in this function (except for the last one, obviously).



-boris


Re: [PATCH] xen/x86: obtain upper 32 bits of video frame buffer address for Dom0

Posted by Jan Beulich 3 weeks, 1 day ago
On 04.01.2022 18:03, Boris Ostrovsky wrote:
> 
> On 1/4/22 11:54 AM, Jan Beulich wrote:
>> On 04.01.2022 17:50, Boris Ostrovsky wrote:
>>> On 1/4/22 3:46 AM, Jan Beulich wrote:
>>>> The hypervisor has been supplying this information for a couple of major
>>>> releases. Make use of it. The need to set a flag in the capabilities
>>>> field also points out that the prior setting of that field from the
>>>> hypervisor interface's gbl_caps one was wrong, so that code gets deleted
>>>> (there's also no equivalent of this in native boot code).
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> --- a/arch/x86/xen/vga.c
>>>> +++ b/arch/x86/xen/vga.c
>>>> @@ -63,13 +63,17 @@ void __init xen_init_vga(const struct do
>>>>    		}
>>>>    
>>>>    		if (size >= offsetof(struct dom0_vga_console_info,
>>>> -				     u.vesa_lfb.gbl_caps)
>>>> -		    + sizeof(info->u.vesa_lfb.gbl_caps))
>>>> -			screen_info->capabilities = info->u.vesa_lfb.gbl_caps;
>>>> -		if (size >= offsetof(struct dom0_vga_console_info,
>>>>    				     u.vesa_lfb.mode_attrs)
>>>>    		    + sizeof(info->u.vesa_lfb.mode_attrs))
>>>
>>> Do we still need this test? All 4.0+ hypervisors will have mode_attrs.
>> Perhaps this could also be dropped, but unlike the capabilities part
>> I'd view this as an unrelated change.
> 
> 
> Right.
> 
> 
>>   Furthermore even a new hypervisor
>> would be free to omit the field, provided it also sets size low enough.
> 
> 
> If this is allowed, how would we deal with hypervisor dropping some other random field here?

Random fields can't be dropped, or very old Dom0 kernels might break.
It's only the "extensions" that have been added later which we can
expect consumers to properly deal with (by checking whether they're
covered by the supplied size).

> Have we had a precedent of this happening?

No. But doing so wouldn't violate the ABI.

Anyway - I'd appreciate if the patch at hand could be taken
independent of possible further adjustments here, as it addresses
an issue observed in the field.

Jan


Re: [PATCH] xen/x86: obtain upper 32 bits of video frame buffer address for Dom0

Posted by Boris Ostrovsky 3 weeks, 1 day ago
On 1/5/22 2:59 AM, Jan Beulich wrote:
>
> Anyway - I'd appreciate if the patch at hand could be taken
> independent of possible further adjustments here, as it addresses
> an issue observed in the field.


Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>