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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.05.19 at 02:22, <marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> On Tue, May 07, 2019 at 03:07:27AM -0600, Jan Beulich wrote:
>> >>> On 06.05.19 at 16:50, <marmarek@xxxxxxxxxxxxxxxxxxxxxx> 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).

Anonymous structures can, iirc, be easily have their type used by
using typeof(). But indeed I was thinking about a parameter-less
function or macro as a possible option.

> 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.

To be honest, I've noticed this while putting together the previous
reply, and I didn't think it would by any problem in the slightest.

Jan



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