On Tue, May 07, 2019 at 03:07:27AM -0600, Jan Beulich wrote:
> >>> On 06.05.19 at 16:50, <marmarek@invisiblethingslab.com> wrote:
> > --- a/xen/drivers/video/vesa.c
> > +++ b/xen/drivers/video/vesa.c
> > @@ -84,6 +84,7 @@ void __init vesa_early_init(void)
> >  void __init vesa_init(void)
> >  {
> >      struct lfb_prop lfbp;
> > +    unsigned long lfb_base;
> >  
> >      if ( !font )
> >          return;
> > @@ -97,15 +98,17 @@ 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);
> > +    lfb_base = vlfb_info.lfb_base;
> > +    lfb_base |= (unsigned long)vlfb_info.ext_lfb_base << 32;
> > +    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 %#lx, mapped to 0x%p, "
> >             "using %uk, total %uk\n",
> > -           vlfb_info.lfb_base, lfb,
> > +           lfb_base, lfb,
> >             vram_remap >> 10, vram_total >> 10);
> >      printk(XENLOG_INFO "vesafb: mode is %dx%dx%u, linelength=%d, font %ux%u\n",
> >             vlfb_info.width, vlfb_info.height,
> > @@ -152,6 +155,10 @@ void __init vesa_mtrr_init(void)
> >          MTRR_TYPE_WRCOMB, MTRR_TYPE_WRTHROUGH };
> >      unsigned int size_total;
> >      int rc, type;
> > +    unsigned long lfb_base;
> > +
> > +    lfb_base = vlfb_info.lfb_base;
> > +    lfb_base |= (unsigned long)vlfb_info.ext_lfb_base << 32;
> >  
> >      if ( !lfb || (vesa_mtrr == 0) || (vesa_mtrr >= ARRAY_SIZE(mtrr_types)) )
> >          return;
> > @@ -167,7 +174,7 @@ void __init vesa_mtrr_init(void)
> >  
> >      /* Try and find a power of two to add */
> >      do {
> > -        rc = mtrr_add(vlfb_info.lfb_base, size_total, type, 1);
> > +        rc = mtrr_add(lfb_base, size_total, type, 1);
> >          size_total >>= 1;
> >      } while ( (size_total >= PAGE_SIZE) && (rc == -EINVAL) );
> >  }
> 
> Imo the result would be better readable if, instead of the local
> variables, you introduced an inline helper lfb_base().

Not necessarily - vlfb_info is a #define to vga_console_info.u.vesa_lfb.
This means such helper would either not have any parameters, or would
need to have full struct xen_console_info as a parameter (inner
structure is anonymous). In both cases, it won't be obvious that
lfb_base live inside vlfb_info. I could add yet another #define instead
of inline function for that, but it wouldn't avoid the second (minor)
issue: a helper without a variable would mean reading the value twice in
vesa_init(). In theory it shouldn't change in the meantime, but IMO
better avoid it anyway.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?