Re: [Xen-devel] [PATCH v2 4/5] xen: fix handling framebuffer located above 4GB

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.05.19 at 21:48, <marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> --- a/xen/drivers/video/vesa.c
> +++ b/xen/drivers/video/vesa.c
> @@ -40,6 +40,11 @@ static int __init parse_font_height(const char *s)
>  }
>  custom_param("font", parse_font_height);
>  
> +static inline paddr_t lfb_base(void)
> +{
> +    return (paddr_t)(vlfb_info.ext_lfb_base) << 32 | vlfb_info.lfb_base;

This wants another set of parentheses around the operands of <<
for disambiguation purposes. We really only leave the school math
operators (+, -, *, /, %, and often the relational ones when used
in isolation) un-parenthesized, for it being (hopefully) known to
everyone what their precedence is.

> @@ -97,15 +102,15 @@ void __init vesa_init(void)
>      lfbp.text_columns = vlfb_info.width / font->width;
>      lfbp.text_rows = vlfb_info.height / font->height;
>  
> -    lfbp.lfb = lfb = ioremap(vlfb_info.lfb_base, vram_remap);
> +    lfbp.lfb = lfb = ioremap(lfb_base(), vram_remap);
>      if ( !lfb )
>          return;
>  
>      memset(lfb, 0, vram_remap);
>  
> -    printk(XENLOG_INFO "vesafb: framebuffer at %#x, mapped to 0x%p, "
> +    printk(XENLOG_INFO "vesafb: framebuffer at %" PRIpaddr ", mapped to 
> 0x%p, "

This drops the 0x prefix from the logged line; it needs to be made
explicit not in the format string.

I think this would also be a good opportunity to un-wrap the format
string.

> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -923,6 +923,10 @@ typedef struct dom0_vga_console_info {
>              /* Mode attributes (offset 0x0, VESA command 0x4f01). */
>              uint16_t mode_attrs;
>  #endif
> +#if __XEN_INTERFACE_VERSION__ >= 0x00040d00 || defined(__XEN__)

The "defined(__XEN__)" is unnecessary for master, and should hence
be dropped. I've suggested this for backporting only.

There's also an unnamed padding field that you introduce. This
should be made explicit, so it can be assigned a meaning later on (and
in particular can be checked to be zero, if need be). Afaict the only
instance of this structure type is a static variable, in which case there's
no need to add explicit zero initialization anywhere.

With all of them taken care of
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel