[PATCH 0/5] Fixes for large framebuffer, placed above 4GB

[PATCH 0/5] Fixes for large framebuffer, placed above 4GB

[Marek Marczykowski-Górecki]

A bunch of fixes for booting Xen on Thinkpad P52, through grub2-efi +
multiboot2. Most of them can be applied independently.

---
cc: Andrew Cooper <andrew.cooper3@citrix.com>
cc: George Dunlap <George.Dunlap@eu.citrix.com>
cc: Ian Jackson <ian.jackson@eu.citrix.com>
cc: Jan Beulich <jbeulich@suse.com>
cc: Julien Grall <julien.grall@arm.com>
cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
cc: Stefano Stabellini <sstabellini@kernel.org>
cc: Tim Deegan <tim@xen.org>
cc: Wei Liu <wei.liu2@citrix.com>
cc: Olaf Hering <olaf@aepfle.de>

Marek Marczykowski-Górecki (5):
  xen/bitmap: fix bitmap_fill with zero-sized bitmap
  drivers/video: drop unused limits
  drivers/video: Drop framebuffer size constraints
  xen: fix handling framebuffer located above 4GB
  drivers/video: use vlfb_info consistently

 xen/arch/x86/efi/efi-boot.h |  1 +
 xen/drivers/video/lfb.c     | 13 -------------
 xen/drivers/video/vesa.c    | 17 ++++++++++++-----
 xen/include/public/xen.h    |  2 ++
 xen/include/xen/bitmap.h    |  2 ++
 5 files changed, 17 insertions(+), 18 deletions(-)

base-commit: dc497635d93f6672f82727ad97a55205177be2aa
-- 
git-series 0.9.1

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

[Xen-devel] [PATCH 0/5] Fixes for large framebuffer, placed above 4GB

[Marek Marczykowski-Górecki]

A bunch of fixes for booting Xen on Thinkpad P52, through grub2-efi +
multiboot2. Most of them can be applied independently.

---
cc: Andrew Cooper <andrew.cooper3@citrix.com>
cc: George Dunlap <George.Dunlap@eu.citrix.com>
cc: Ian Jackson <ian.jackson@eu.citrix.com>
cc: Jan Beulich <jbeulich@suse.com>
cc: Julien Grall <julien.grall@arm.com>
cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
cc: Stefano Stabellini <sstabellini@kernel.org>
cc: Tim Deegan <tim@xen.org>
cc: Wei Liu <wei.liu2@citrix.com>
cc: Olaf Hering <olaf@aepfle.de>

Marek Marczykowski-Górecki (5):
  xen/bitmap: fix bitmap_fill with zero-sized bitmap
  drivers/video: drop unused limits
  drivers/video: Drop framebuffer size constraints
  xen: fix handling framebuffer located above 4GB
  drivers/video: use vlfb_info consistently

 xen/arch/x86/efi/efi-boot.h |  1 +
 xen/drivers/video/lfb.c     | 13 -------------
 xen/drivers/video/vesa.c    | 17 ++++++++++++-----
 xen/include/public/xen.h    |  2 ++
 xen/include/xen/bitmap.h    |  2 ++
 5 files changed, 17 insertions(+), 18 deletions(-)

base-commit: dc497635d93f6672f82727ad97a55205177be2aa
-- 
git-series 0.9.1

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

[PATCH 1/5] xen/bitmap: fix bitmap_fill with zero-sized bitmap

[Marek Marczykowski-Górecki]

When bitmap_fill(..., 0) is called, do not try to write anything. Before
this patch, it tried to write almost LONG_MAX, surely overwriting
something.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Found while debugging framebuffer located above 4GB. In that case 32bit
variable for it overflows and framebuffer initialization zeroed
unrelated memory. Specifically, it hit mbi->mods_count, so later on
bitmap_fill(module_map, mbi->mods_count) in __start_xen() crashed.
---
 xen/include/xen/bitmap.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/include/xen/bitmap.h b/xen/include/xen/bitmap.h
index fe3c720..0430c1c 100644
--- a/xen/include/xen/bitmap.h
+++ b/xen/include/xen/bitmap.h
@@ -126,6 +126,8 @@ static inline void bitmap_fill(unsigned long *dst, int nbits)
 	size_t nlongs = BITS_TO_LONGS(nbits);
 
 	switch (nlongs) {
+	case 0:
+		break;
 	default:
 		memset(dst, -1, (nlongs - 1) * sizeof(unsigned long));
 		/* fall through */
-- 
git-series 0.9.1

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

[Xen-devel] [PATCH 1/5] xen/bitmap: fix bitmap_fill with zero-sized bitmap

[Marek Marczykowski-Górecki]

When bitmap_fill(..., 0) is called, do not try to write anything. Before
this patch, it tried to write almost LONG_MAX, surely overwriting
something.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Found while debugging framebuffer located above 4GB. In that case 32bit
variable for it overflows and framebuffer initialization zeroed
unrelated memory. Specifically, it hit mbi->mods_count, so later on
bitmap_fill(module_map, mbi->mods_count) in __start_xen() crashed.
---
 xen/include/xen/bitmap.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/include/xen/bitmap.h b/xen/include/xen/bitmap.h
index fe3c720..0430c1c 100644
--- a/xen/include/xen/bitmap.h
+++ b/xen/include/xen/bitmap.h
@@ -126,6 +126,8 @@ static inline void bitmap_fill(unsigned long *dst, int nbits)
 	size_t nlongs = BITS_TO_LONGS(nbits);
 
 	switch (nlongs) {
+	case 0:
+		break;
 	default:
 		memset(dst, -1, (nlongs - 1) * sizeof(unsigned long));
 		/* fall through */
-- 
git-series 0.9.1

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

[PATCH 2/5] drivers/video: drop unused limits

[Marek Marczykowski-Górecki]

MAX_BPP, MAX_FONT_W, MAX_FONT_H are not used in the code at all.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 xen/drivers/video/lfb.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/xen/drivers/video/lfb.c b/xen/drivers/video/lfb.c
index d0c8c49..0475a68 100644
--- a/xen/drivers/video/lfb.c
+++ b/xen/drivers/video/lfb.c
@@ -12,9 +12,6 @@
 
 #define MAX_XRES 1900
 #define MAX_YRES 1200
-#define MAX_BPP 4
-#define MAX_FONT_W 8
-#define MAX_FONT_H 16
 
 struct lfb_status {
     struct lfb_prop lfbp;
-- 
git-series 0.9.1

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

[Xen-devel] [PATCH 2/5] drivers/video: drop unused limits

[Marek Marczykowski-Górecki]

MAX_BPP, MAX_FONT_W, MAX_FONT_H are not used in the code at all.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 xen/drivers/video/lfb.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/xen/drivers/video/lfb.c b/xen/drivers/video/lfb.c
index d0c8c49..0475a68 100644
--- a/xen/drivers/video/lfb.c
+++ b/xen/drivers/video/lfb.c
@@ -12,9 +12,6 @@
 
 #define MAX_XRES 1900
 #define MAX_YRES 1200
-#define MAX_BPP 4
-#define MAX_FONT_W 8
-#define MAX_FONT_H 16
 
 struct lfb_status {
     struct lfb_prop lfbp;
-- 
git-series 0.9.1

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

[PATCH 3/5] drivers/video: Drop framebuffer size constraints

[Marek Marczykowski-Górecki]

The limit 1900x1200 do not match real world devices (1900 looks like a
typo, should be 1920). But in practice the limits are arbitrary and do
not serve any real purpose. As discussed in "Increase framebuffer size
to todays standards" thread, drop them completely.

This fixes graphic console on device with 3840x2160 native resolution.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Cc: Olaf Hering <olaf@aepfle.de>
---
 xen/drivers/video/lfb.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/xen/drivers/video/lfb.c b/xen/drivers/video/lfb.c
index 0475a68..5022195 100644
--- a/xen/drivers/video/lfb.c
+++ b/xen/drivers/video/lfb.c
@@ -10,9 +10,6 @@
 #include "lfb.h"
 #include "font.h"
 
-#define MAX_XRES 1900
-#define MAX_YRES 1200
-
 struct lfb_status {
     struct lfb_prop lfbp;
 
@@ -146,13 +143,6 @@ void lfb_carriage_return(void)
 
 int __init lfb_init(struct lfb_prop *lfbp)
 {
-    if ( lfbp->width > MAX_XRES || lfbp->height > MAX_YRES )
-    {
-        printk(XENLOG_WARNING "Couldn't initialize a %ux%u framebuffer early.\n",
-               lfbp->width, lfbp->height);
-        return -EINVAL;
-    }
-
     lfb.lfbp = *lfbp;
 
     lfb.lbuf = xmalloc_bytes(lfb.lfbp.bytes_per_line);
-- 
git-series 0.9.1

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

[Xen-devel] [PATCH 3/5] drivers/video: Drop framebuffer size constraints

[Marek Marczykowski-Górecki]

The limit 1900x1200 do not match real world devices (1900 looks like a
typo, should be 1920). But in practice the limits are arbitrary and do
not serve any real purpose. As discussed in "Increase framebuffer size
to todays standards" thread, drop them completely.

This fixes graphic console on device with 3840x2160 native resolution.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Cc: Olaf Hering <olaf@aepfle.de>
---
 xen/drivers/video/lfb.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/xen/drivers/video/lfb.c b/xen/drivers/video/lfb.c
index 0475a68..5022195 100644
--- a/xen/drivers/video/lfb.c
+++ b/xen/drivers/video/lfb.c
@@ -10,9 +10,6 @@
 #include "lfb.h"
 #include "font.h"
 
-#define MAX_XRES 1900
-#define MAX_YRES 1200
-
 struct lfb_status {
     struct lfb_prop lfbp;
 
@@ -146,13 +143,6 @@ void lfb_carriage_return(void)
 
 int __init lfb_init(struct lfb_prop *lfbp)
 {
-    if ( lfbp->width > MAX_XRES || lfbp->height > MAX_YRES )
-    {
-        printk(XENLOG_WARNING "Couldn't initialize a %ux%u framebuffer early.\n",
-               lfbp->width, lfbp->height);
-        return -EINVAL;
-    }
-
     lfb.lfbp = *lfbp;
 
     lfb.lbuf = xmalloc_bytes(lfb.lfbp.bytes_per_line);
-- 
git-series 0.9.1

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

[PATCH 4/5] xen: fix handling framebuffer located above 4GB

[Marek Marczykowski-Górecki]

On some machines (for example Thinkpad P52), UEFI GOP reports
framebuffer located above 4GB (0x4000000000 on that machine). This
address does not fit in {xen,dom0}_vga_console_info.u.vesa_lfb.lfb_base
field, which is 32bit. The overflow here cause all kind of memory
corruption when anything tries to write something on the screen,
starting with zeroing the whole framebuffer in vesa_init().

Fix this similar to how it's done in Linux: add ext_lfb_base field at
the end of the structure, to hold upper 32bits of the address. Since the
field is added at the end of the structure, it will work with older
Linux versions too (other than using possibly truncated address - no
worse than without this change). Thanks to ABI containing size of the
structure (start_info.console.dom0.info_size), Linux can detect when
this field is present and use it appropriately then.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 xen/arch/x86/efi/efi-boot.h |  1 +
 xen/drivers/video/vesa.c    | 15 +++++++++++----
 xen/include/public/xen.h    |  2 ++
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 5789d2c..7a13a30 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -550,6 +550,7 @@ static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
         vga_console_info.u.vesa_lfb.bytes_per_line =
             (mode_info->PixelsPerScanLine * bpp + 7) >> 3;
         vga_console_info.u.vesa_lfb.lfb_base = gop->Mode->FrameBufferBase;
+        vga_console_info.u.vesa_lfb.ext_lfb_base = gop->Mode->FrameBufferBase >> 32;
         vga_console_info.u.vesa_lfb.lfb_size =
             (gop->Mode->FrameBufferSize + 0xffff) >> 16;
     }
diff --git a/xen/drivers/video/vesa.c b/xen/drivers/video/vesa.c
index c92497e..f22cf7f 100644
--- 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) );
 }
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index ccdffc0..b0f0f7e 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -923,6 +923,8 @@ typedef struct dom0_vga_console_info {
             /* Mode attributes (offset 0x0, VESA command 0x4f01). */
             uint16_t mode_attrs;
 #endif
+            /* high 32 bits of lfb_base */
+            uint32_t ext_lfb_base;
         } vesa_lfb;
     } u;
 } dom0_vga_console_info_t;
-- 
git-series 0.9.1

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

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

[Marek Marczykowski-Górecki]

On some machines (for example Thinkpad P52), UEFI GOP reports
framebuffer located above 4GB (0x4000000000 on that machine). This
address does not fit in {xen,dom0}_vga_console_info.u.vesa_lfb.lfb_base
field, which is 32bit. The overflow here cause all kind of memory
corruption when anything tries to write something on the screen,
starting with zeroing the whole framebuffer in vesa_init().

Fix this similar to how it's done in Linux: add ext_lfb_base field at
the end of the structure, to hold upper 32bits of the address. Since the
field is added at the end of the structure, it will work with older
Linux versions too (other than using possibly truncated address - no
worse than without this change). Thanks to ABI containing size of the
structure (start_info.console.dom0.info_size), Linux can detect when
this field is present and use it appropriately then.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 xen/arch/x86/efi/efi-boot.h |  1 +
 xen/drivers/video/vesa.c    | 15 +++++++++++----
 xen/include/public/xen.h    |  2 ++
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 5789d2c..7a13a30 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -550,6 +550,7 @@ static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
         vga_console_info.u.vesa_lfb.bytes_per_line =
             (mode_info->PixelsPerScanLine * bpp + 7) >> 3;
         vga_console_info.u.vesa_lfb.lfb_base = gop->Mode->FrameBufferBase;
+        vga_console_info.u.vesa_lfb.ext_lfb_base = gop->Mode->FrameBufferBase >> 32;
         vga_console_info.u.vesa_lfb.lfb_size =
             (gop->Mode->FrameBufferSize + 0xffff) >> 16;
     }
diff --git a/xen/drivers/video/vesa.c b/xen/drivers/video/vesa.c
index c92497e..f22cf7f 100644
--- 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) );
 }
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index ccdffc0..b0f0f7e 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -923,6 +923,8 @@ typedef struct dom0_vga_console_info {
             /* Mode attributes (offset 0x0, VESA command 0x4f01). */
             uint16_t mode_attrs;
 #endif
+            /* high 32 bits of lfb_base */
+            uint32_t ext_lfb_base;
         } vesa_lfb;
     } u;
 } dom0_vga_console_info_t;
-- 
git-series 0.9.1

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

[PATCH 5/5] drivers/video: use vlfb_info consistently

[Marek Marczykowski-Górecki]

vlfb_info is an alias for vga_console_info.u.vesa_lfb, so this change is
purely cosmetic. But using the same name helps reading the code.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 xen/drivers/video/vesa.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/video/vesa.c b/xen/drivers/video/vesa.c
index f22cf7f..3767024 100644
--- a/xen/drivers/video/vesa.c
+++ b/xen/drivers/video/vesa.c
@@ -44,7 +44,7 @@ void __init vesa_early_init(void)
 {
     unsigned int vram_vmode;
 
-    vga_compat = !(vga_console_info.u.vesa_lfb.gbl_caps & 2);
+    vga_compat = !(vlfb_info.gbl_caps & 2);
 
     if ( (vlfb_info.bits_per_pixel < 8) || (vlfb_info.bits_per_pixel > 32) )
         return;
-- 
git-series 0.9.1

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

[Xen-devel] [PATCH 5/5] drivers/video: use vlfb_info consistently

[Marek Marczykowski-Górecki]

vlfb_info is an alias for vga_console_info.u.vesa_lfb, so this change is
purely cosmetic. But using the same name helps reading the code.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 xen/drivers/video/vesa.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/video/vesa.c b/xen/drivers/video/vesa.c
index f22cf7f..3767024 100644
--- a/xen/drivers/video/vesa.c
+++ b/xen/drivers/video/vesa.c
@@ -44,7 +44,7 @@ void __init vesa_early_init(void)
 {
     unsigned int vram_vmode;
 
-    vga_compat = !(vga_console_info.u.vesa_lfb.gbl_caps & 2);
+    vga_compat = !(vlfb_info.gbl_caps & 2);
 
     if ( (vlfb_info.bits_per_pixel < 8) || (vlfb_info.bits_per_pixel > 32) )
         return;
-- 
git-series 0.9.1

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

Re: [PATCH 3/5] drivers/video: Drop framebuffer size constraints

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 415 bytes --]

Am Mon,  6 May 2019 16:50:19 +0200
schrieb Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>:

> The limit 1900x1200 do not match real world devices (1900 looks like a
> typo, should be 1920). But in practice the limits are arbitrary and do
> not serve any real purpose. As discussed in "Increase framebuffer size
> to todays standards" thread, drop them completely.

ACK for the idea.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [Xen-devel] [PATCH 3/5] drivers/video: Drop framebuffer size constraints

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 415 bytes --]

Am Mon,  6 May 2019 16:50:19 +0200
schrieb Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>:

> The limit 1900x1200 do not match real world devices (1900 looks like a
> typo, should be 1920). But in practice the limits are arbitrary and do
> not serve any real purpose. As discussed in "Increase framebuffer size
> to todays standards" thread, drop them completely.

ACK for the idea.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Juergen Gross]

On 06/05/2019 16:50, Marek Marczykowski-Górecki wrote:
> On some machines (for example Thinkpad P52), UEFI GOP reports
> framebuffer located above 4GB (0x4000000000 on that machine). This
> address does not fit in {xen,dom0}_vga_console_info.u.vesa_lfb.lfb_base
> field, which is 32bit. The overflow here cause all kind of memory
> corruption when anything tries to write something on the screen,
> starting with zeroing the whole framebuffer in vesa_init().
> 
> Fix this similar to how it's done in Linux: add ext_lfb_base field at
> the end of the structure, to hold upper 32bits of the address. Since the
> field is added at the end of the structure, it will work with older
> Linux versions too (other than using possibly truncated address - no
> worse than without this change). Thanks to ABI containing size of the
> structure (start_info.console.dom0.info_size), Linux can detect when
> this field is present and use it appropriately then.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
>  xen/arch/x86/efi/efi-boot.h |  1 +
>  xen/drivers/video/vesa.c    | 15 +++++++++++----
>  xen/include/public/xen.h    |  2 ++
>  3 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 5789d2c..7a13a30 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -550,6 +550,7 @@ static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
>          vga_console_info.u.vesa_lfb.bytes_per_line =
>              (mode_info->PixelsPerScanLine * bpp + 7) >> 3;
>          vga_console_info.u.vesa_lfb.lfb_base = gop->Mode->FrameBufferBase;
> +        vga_console_info.u.vesa_lfb.ext_lfb_base = gop->Mode->FrameBufferBase >> 32;
>          vga_console_info.u.vesa_lfb.lfb_size =
>              (gop->Mode->FrameBufferSize + 0xffff) >> 16;
>      }
> diff --git a/xen/drivers/video/vesa.c b/xen/drivers/video/vesa.c
> index c92497e..f22cf7f 100644
> --- 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) );
>  }
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> index ccdffc0..b0f0f7e 100644
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -923,6 +923,8 @@ typedef struct dom0_vga_console_info {
>              /* Mode attributes (offset 0x0, VESA command 0x4f01). */
>              uint16_t mode_attrs;
>  #endif
> +            /* high 32 bits of lfb_base */
> +            uint32_t ext_lfb_base;

You will need to put this addition into an:

#if __XEN_INTERFACE_VERSION__ >= 0x00040d00
...
#endif

section (only available from Xen 4.13 onwards).


Juergen

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

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

[Juergen Gross]

On 06/05/2019 16:50, Marek Marczykowski-Górecki wrote:
> On some machines (for example Thinkpad P52), UEFI GOP reports
> framebuffer located above 4GB (0x4000000000 on that machine). This
> address does not fit in {xen,dom0}_vga_console_info.u.vesa_lfb.lfb_base
> field, which is 32bit. The overflow here cause all kind of memory
> corruption when anything tries to write something on the screen,
> starting with zeroing the whole framebuffer in vesa_init().
> 
> Fix this similar to how it's done in Linux: add ext_lfb_base field at
> the end of the structure, to hold upper 32bits of the address. Since the
> field is added at the end of the structure, it will work with older
> Linux versions too (other than using possibly truncated address - no
> worse than without this change). Thanks to ABI containing size of the
> structure (start_info.console.dom0.info_size), Linux can detect when
> this field is present and use it appropriately then.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
>  xen/arch/x86/efi/efi-boot.h |  1 +
>  xen/drivers/video/vesa.c    | 15 +++++++++++----
>  xen/include/public/xen.h    |  2 ++
>  3 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 5789d2c..7a13a30 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -550,6 +550,7 @@ static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
>          vga_console_info.u.vesa_lfb.bytes_per_line =
>              (mode_info->PixelsPerScanLine * bpp + 7) >> 3;
>          vga_console_info.u.vesa_lfb.lfb_base = gop->Mode->FrameBufferBase;
> +        vga_console_info.u.vesa_lfb.ext_lfb_base = gop->Mode->FrameBufferBase >> 32;
>          vga_console_info.u.vesa_lfb.lfb_size =
>              (gop->Mode->FrameBufferSize + 0xffff) >> 16;
>      }
> diff --git a/xen/drivers/video/vesa.c b/xen/drivers/video/vesa.c
> index c92497e..f22cf7f 100644
> --- 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) );
>  }
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> index ccdffc0..b0f0f7e 100644
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -923,6 +923,8 @@ typedef struct dom0_vga_console_info {
>              /* Mode attributes (offset 0x0, VESA command 0x4f01). */
>              uint16_t mode_attrs;
>  #endif
> +            /* high 32 bits of lfb_base */
> +            uint32_t ext_lfb_base;

You will need to put this addition into an:

#if __XEN_INTERFACE_VERSION__ >= 0x00040d00
...
#endif

section (only available from Xen 4.13 onwards).


Juergen

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

Re: [PATCH 1/5] xen/bitmap: fix bitmap_fill with zero-sized bitmap

[Andrew Cooper]

On 06/05/2019 15:50, Marek Marczykowski-Górecki wrote:
> When bitmap_fill(..., 0) is called, do not try to write anything. Before
> this patch, it tried to write almost LONG_MAX, surely overwriting
> something.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

It looks like all other operations do cope correctly with nbits being 0.

~Andrew

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

Re: [Xen-devel] [PATCH 1/5] xen/bitmap: fix bitmap_fill with zero-sized bitmap

[Andrew Cooper]

On 06/05/2019 15:50, Marek Marczykowski-Górecki wrote:
> When bitmap_fill(..., 0) is called, do not try to write anything. Before
> this patch, it tried to write almost LONG_MAX, surely overwriting
> something.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

It looks like all other operations do cope correctly with nbits being 0.

~Andrew

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

Re: [PATCH 2/5] drivers/video: drop unused limits

[Andrew Cooper]

On 06/05/2019 15:50, Marek Marczykowski-Górecki wrote:
> MAX_BPP, MAX_FONT_W, MAX_FONT_H are not used in the code at all.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [Xen-devel] [PATCH 2/5] drivers/video: drop unused limits

[Andrew Cooper]

On 06/05/2019 15:50, Marek Marczykowski-Górecki wrote:
> MAX_BPP, MAX_FONT_W, MAX_FONT_H are not used in the code at all.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

[Andrew Cooper]

On 06/05/2019 15:50, Marek Marczykowski-Górecki wrote:
> diff --git a/xen/drivers/video/vesa.c b/xen/drivers/video/vesa.c
> index c92497e..f22cf7f 100644
> --- 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;

This is common code, which is (in principle, although not currently)
shared with arm32, where unsigned long isn't wide enough.

Use paddr_t and PRIpaddr, which should be wide enough in any build.

~Andrew

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

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

[Andrew Cooper]

On 06/05/2019 15:50, Marek Marczykowski-Górecki wrote:
> diff --git a/xen/drivers/video/vesa.c b/xen/drivers/video/vesa.c
> index c92497e..f22cf7f 100644
> --- 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;

This is common code, which is (in principle, although not currently)
shared with arm32, where unsigned long isn't wide enough.

Use paddr_t and PRIpaddr, which should be wide enough in any build.

~Andrew

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

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

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 1122 bytes --]

On Mon, May 06, 2019 at 05:15:19PM +0200, Juergen Gross wrote:
> On 06/05/2019 16:50, Marek Marczykowski-Górecki wrote:
> > diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> > index ccdffc0..b0f0f7e 100644
> > --- a/xen/include/public/xen.h
> > +++ b/xen/include/public/xen.h
> > @@ -923,6 +923,8 @@ typedef struct dom0_vga_console_info {
> >              /* Mode attributes (offset 0x0, VESA command 0x4f01). */
> >              uint16_t mode_attrs;
> >  #endif
> > +            /* high 32 bits of lfb_base */
> > +            uint32_t ext_lfb_base;
> 
> You will need to put this addition into an:
> 
> #if __XEN_INTERFACE_VERSION__ >= 0x00040d00
> ...
> #endif
> 
> section (only available from Xen 4.13 onwards).

Why exactly? I don't see this structure used in any hypercall argument.
The only externally accessible place is start_info structure, where it
has explicit struct size already.

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 1122 bytes --]

On Mon, May 06, 2019 at 05:15:19PM +0200, Juergen Gross wrote:
> On 06/05/2019 16:50, Marek Marczykowski-Górecki wrote:
> > diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> > index ccdffc0..b0f0f7e 100644
> > --- a/xen/include/public/xen.h
> > +++ b/xen/include/public/xen.h
> > @@ -923,6 +923,8 @@ typedef struct dom0_vga_console_info {
> >              /* Mode attributes (offset 0x0, VESA command 0x4f01). */
> >              uint16_t mode_attrs;
> >  #endif
> > +            /* high 32 bits of lfb_base */
> > +            uint32_t ext_lfb_base;
> 
> You will need to put this addition into an:
> 
> #if __XEN_INTERFACE_VERSION__ >= 0x00040d00
> ...
> #endif
> 
> section (only available from Xen 4.13 onwards).

Why exactly? I don't see this structure used in any hypercall argument.
The only externally accessible place is start_info structure, where it
has explicit struct size already.

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH 3/5] drivers/video: Drop framebuffer size constraints

[Andrew Cooper]

On 06/05/2019 15:50, Marek Marczykowski-Górecki wrote:
> The limit 1900x1200 do not match real world devices (1900 looks like a
> typo, should be 1920). But in practice the limits are arbitrary and do
> not serve any real purpose. As discussed in "Increase framebuffer size
> to todays standards" thread, drop them completely.
>
> This fixes graphic console on device with 3840x2160 native resolution.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [Xen-devel] [PATCH 3/5] drivers/video: Drop framebuffer size constraints

[Andrew Cooper]

On 06/05/2019 15:50, Marek Marczykowski-Górecki wrote:
> The limit 1900x1200 do not match real world devices (1900 looks like a
> typo, should be 1920). But in practice the limits are arbitrary and do
> not serve any real purpose. As discussed in "Increase framebuffer size
> to todays standards" thread, drop them completely.
>
> This fixes graphic console on device with 3840x2160 native resolution.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 5/5] drivers/video: use vlfb_info consistently

[Andrew Cooper]

On 06/05/2019 15:50, Marek Marczykowski-Górecki wrote:
> vlfb_info is an alias for vga_console_info.u.vesa_lfb, so this change is
> purely cosmetic. But using the same name helps reading the code.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [Xen-devel] [PATCH 5/5] drivers/video: use vlfb_info consistently

[Andrew Cooper]

On 06/05/2019 15:50, Marek Marczykowski-Górecki wrote:
> vlfb_info is an alias for vga_console_info.u.vesa_lfb, so this change is
> purely cosmetic. But using the same name helps reading the code.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 0/5] Fixes for large framebuffer, placed above 4GB

[Andrew Cooper]

On 06/05/2019 15:50, Marek Marczykowski-Górecki wrote:
> A bunch of fixes for booting Xen on Thinkpad P52, through grub2-efi +
> multiboot2. Most of them can be applied independently.

Patches 1-3,5 are all trivially correct, and I've pulled them into x86-next.

~Andrew

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

Re: [Xen-devel] [PATCH 0/5] Fixes for large framebuffer, placed above 4GB

[Andrew Cooper]

On 06/05/2019 15:50, Marek Marczykowski-Górecki wrote:
> A bunch of fixes for booting Xen on Thinkpad P52, through grub2-efi +
> multiboot2. Most of them can be applied independently.

Patches 1-3,5 are all trivially correct, and I've pulled them into x86-next.

~Andrew

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

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

[Juergen Gross]

On 06/05/2019 17:32, Marek Marczykowski-Górecki wrote:
> On Mon, May 06, 2019 at 05:15:19PM +0200, Juergen Gross wrote:
>> On 06/05/2019 16:50, Marek Marczykowski-Górecki wrote:
>>> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
>>> index ccdffc0..b0f0f7e 100644
>>> --- a/xen/include/public/xen.h
>>> +++ b/xen/include/public/xen.h
>>> @@ -923,6 +923,8 @@ typedef struct dom0_vga_console_info {
>>>              /* Mode attributes (offset 0x0, VESA command 0x4f01). */
>>>              uint16_t mode_attrs;
>>>  #endif
>>> +            /* high 32 bits of lfb_base */
>>> +            uint32_t ext_lfb_base;
>>
>> You will need to put this addition into an:
>>
>> #if __XEN_INTERFACE_VERSION__ >= 0x00040d00
>> ...
>> #endif
>>
>> section (only available from Xen 4.13 onwards).
> 
> Why exactly? I don't see this structure used in any hypercall argument.
> The only externally accessible place is start_info structure, where it
> has explicit struct size already.

With the #ifdef...#endif just above your addition a consumer using an
interface version < 0x00030206 could think gbl_caps is available instead
of ext_lfb_base.


Juergen

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

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

[Juergen Gross]

On 06/05/2019 17:32, Marek Marczykowski-Górecki wrote:
> On Mon, May 06, 2019 at 05:15:19PM +0200, Juergen Gross wrote:
>> On 06/05/2019 16:50, Marek Marczykowski-Górecki wrote:
>>> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
>>> index ccdffc0..b0f0f7e 100644
>>> --- a/xen/include/public/xen.h
>>> +++ b/xen/include/public/xen.h
>>> @@ -923,6 +923,8 @@ typedef struct dom0_vga_console_info {
>>>              /* Mode attributes (offset 0x0, VESA command 0x4f01). */
>>>              uint16_t mode_attrs;
>>>  #endif
>>> +            /* high 32 bits of lfb_base */
>>> +            uint32_t ext_lfb_base;
>>
>> You will need to put this addition into an:
>>
>> #if __XEN_INTERFACE_VERSION__ >= 0x00040d00
>> ...
>> #endif
>>
>> section (only available from Xen 4.13 onwards).
> 
> Why exactly? I don't see this structure used in any hypercall argument.
> The only externally accessible place is start_info structure, where it
> has explicit struct size already.

With the #ifdef...#endif just above your addition a consumer using an
interface version < 0x00030206 could think gbl_caps is available instead
of ext_lfb_base.


Juergen

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

Re: [PATCH 1/5] xen/bitmap: fix bitmap_fill with zero-sized bitmap

[Jan Beulich]

>>> On 06.05.19 at 16:50, <marmarek@invisiblethingslab.com> wrote:
> When bitmap_fill(..., 0) is called, do not try to write anything. Before
> this patch, it tried to write almost LONG_MAX, surely overwriting
> something.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

I'm embarrassed, seeing that commit d8a7694e5a ("bitmap_*() should
cope with zero size bitmaps") changed the code to its present shape,
but left the issue un-addressed here despite its title.

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

> Found while debugging framebuffer located above 4GB. In that case 32bit
> variable for it overflows and framebuffer initialization zeroed
> unrelated memory. Specifically, it hit mbi->mods_count, so later on
> bitmap_fill(module_map, mbi->mods_count) in __start_xen() crashed.

The origin of your problem being a truncation one, it seems pretty
clear to me that if we want to be able to gracefully handle that,
then we need to stop using plain int in all the involved functions.
I'm curious though which bitmap_fill() it was that you saw misbehave:
There's no such call at all in xen/drivers/video/, and I'm also having
a hard time seeing how the address (rather than the size) of the
frame buffer could be involved here.

Jan


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

Re: [Xen-devel] [PATCH 1/5] xen/bitmap: fix bitmap_fill with zero-sized bitmap

[Jan Beulich]

>>> On 06.05.19 at 16:50, <marmarek@invisiblethingslab.com> wrote:
> When bitmap_fill(..., 0) is called, do not try to write anything. Before
> this patch, it tried to write almost LONG_MAX, surely overwriting
> something.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

I'm embarrassed, seeing that commit d8a7694e5a ("bitmap_*() should
cope with zero size bitmaps") changed the code to its present shape,
but left the issue un-addressed here despite its title.

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

> Found while debugging framebuffer located above 4GB. In that case 32bit
> variable for it overflows and framebuffer initialization zeroed
> unrelated memory. Specifically, it hit mbi->mods_count, so later on
> bitmap_fill(module_map, mbi->mods_count) in __start_xen() crashed.

The origin of your problem being a truncation one, it seems pretty
clear to me that if we want to be able to gracefully handle that,
then we need to stop using plain int in all the involved functions.
I'm curious though which bitmap_fill() it was that you saw misbehave:
There's no such call at all in xen/drivers/video/, and I'm also having
a hard time seeing how the address (rather than the size) of the
frame buffer could be involved here.

Jan


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

Re: [PATCH 2/5] drivers/video: drop unused limits

[Jan Beulich]

>>> On 06.05.19 at 16:50, <marmarek@invisiblethingslab.com> wrote:
> MAX_BPP, MAX_FONT_W, MAX_FONT_H are not used in the code at all.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

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


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

Re: [Xen-devel] [PATCH 2/5] drivers/video: drop unused limits

[Jan Beulich]

>>> On 06.05.19 at 16:50, <marmarek@invisiblethingslab.com> wrote:
> MAX_BPP, MAX_FONT_W, MAX_FONT_H are not used in the code at all.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

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


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

Re: [PATCH 3/5] drivers/video: Drop framebuffer size constraints

[Jan Beulich]

>>> On 06.05.19 at 16:50, <marmarek@invisiblethingslab.com> wrote:
> The limit 1900x1200 do not match real world devices (1900 looks like a
> typo, should be 1920). But in practice the limits are arbitrary and do
> not serve any real purpose. As discussed in "Increase framebuffer size
> to todays standards" thread, drop them completely.
> 
> This fixes graphic console on device with 3840x2160 native resolution.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

And unless I'm mis-remembering again
Suggested-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [Xen-devel] [PATCH 3/5] drivers/video: Drop framebuffer size constraints

[Jan Beulich]

>>> On 06.05.19 at 16:50, <marmarek@invisiblethingslab.com> wrote:
> The limit 1900x1200 do not match real world devices (1900 looks like a
> typo, should be 1920). But in practice the limits are arbitrary and do
> not serve any real purpose. As discussed in "Increase framebuffer size
> to todays standards" thread, drop them completely.
> 
> This fixes graphic console on device with 3840x2160 native resolution.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

And unless I'm mis-remembering again
Suggested-by: Jan Beulich <jbeulich@suse.com>

Jan


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

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

[Jan Beulich]

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

Jan



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

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

[Jan Beulich]

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

Jan



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

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

[Jan Beulich]

>>> On 06.05.19 at 17:32, <marmarek@invisiblethingslab.com> wrote:
> On Mon, May 06, 2019 at 05:15:19PM +0200, Juergen Gross wrote:
>> On 06/05/2019 16:50, Marek Marczykowski-Górecki wrote:
>> > diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
>> > index ccdffc0..b0f0f7e 100644
>> > --- a/xen/include/public/xen.h
>> > +++ b/xen/include/public/xen.h
>> > @@ -923,6 +923,8 @@ typedef struct dom0_vga_console_info {
>> >              /* Mode attributes (offset 0x0, VESA command 0x4f01). */
>> >              uint16_t mode_attrs;
>> >  #endif
>> > +            /* high 32 bits of lfb_base */
>> > +            uint32_t ext_lfb_base;
>> 
>> You will need to put this addition into an:
>> 
>> #if __XEN_INTERFACE_VERSION__ >= 0x00040d00
>> ...
>> #endif
>> 
>> section (only available from Xen 4.13 onwards).
> 
> Why exactly? I don't see this structure used in any hypercall argument.
> The only externally accessible place is start_info structure, where it
> has explicit struct size already.

In addition to Jürgen's reply: While the structure isn't meant to
be used that way, any consumer of the Xen headers could in
principle create instances of it (rather than just using pointers
to the Xen-provided instance), and without the consuming code
signaling its awareness such structure sizes may not change.

Jan


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

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

[Jan Beulich]

>>> On 06.05.19 at 17:32, <marmarek@invisiblethingslab.com> wrote:
> On Mon, May 06, 2019 at 05:15:19PM +0200, Juergen Gross wrote:
>> On 06/05/2019 16:50, Marek Marczykowski-Górecki wrote:
>> > diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
>> > index ccdffc0..b0f0f7e 100644
>> > --- a/xen/include/public/xen.h
>> > +++ b/xen/include/public/xen.h
>> > @@ -923,6 +923,8 @@ typedef struct dom0_vga_console_info {
>> >              /* Mode attributes (offset 0x0, VESA command 0x4f01). */
>> >              uint16_t mode_attrs;
>> >  #endif
>> > +            /* high 32 bits of lfb_base */
>> > +            uint32_t ext_lfb_base;
>> 
>> You will need to put this addition into an:
>> 
>> #if __XEN_INTERFACE_VERSION__ >= 0x00040d00
>> ...
>> #endif
>> 
>> section (only available from Xen 4.13 onwards).
> 
> Why exactly? I don't see this structure used in any hypercall argument.
> The only externally accessible place is start_info structure, where it
> has explicit struct size already.

In addition to Jürgen's reply: While the structure isn't meant to
be used that way, any consumer of the Xen headers could in
principle create instances of it (rather than just using pointers
to the Xen-provided instance), and without the consuming code
signaling its awareness such structure sizes may not change.

Jan


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

Re: [PATCH 1/5] xen/bitmap: fix bitmap_fill with zero-sized bitmap

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 1318 bytes --]

On Tue, May 07, 2019 at 02:10:20AM -0600, Jan Beulich wrote:
> >>> On 06.05.19 at 16:50, <marmarek@invisiblethingslab.com> wrote:
> > Found while debugging framebuffer located above 4GB. In that case 32bit
> > variable for it overflows and framebuffer initialization zeroed
> > unrelated memory. Specifically, it hit mbi->mods_count, so later on
> > bitmap_fill(module_map, mbi->mods_count) in __start_xen() crashed.
> 
> The origin of your problem being a truncation one, it seems pretty
> clear to me that if we want to be able to gracefully handle that,
> then we need to stop using plain int in all the involved functions.
> I'm curious though which bitmap_fill() it was that you saw misbehave:
> There's no such call at all in xen/drivers/video/, and I'm also having
> a hard time seeing how the address (rather than the size) of the
> frame buffer could be involved here.

Truncated framebuffer address (0x0) caused memset() in vesa_init() to
zero (among other things) mbi->mods_count. This triggered the crash as
described above.
Obviously, bitmap_fill() crash was just a fallout here, not the root
cause.

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [Xen-devel] [PATCH 1/5] xen/bitmap: fix bitmap_fill with zero-sized bitmap

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 1318 bytes --]

On Tue, May 07, 2019 at 02:10:20AM -0600, Jan Beulich wrote:
> >>> On 06.05.19 at 16:50, <marmarek@invisiblethingslab.com> wrote:
> > Found while debugging framebuffer located above 4GB. In that case 32bit
> > variable for it overflows and framebuffer initialization zeroed
> > unrelated memory. Specifically, it hit mbi->mods_count, so later on
> > bitmap_fill(module_map, mbi->mods_count) in __start_xen() crashed.
> 
> The origin of your problem being a truncation one, it seems pretty
> clear to me that if we want to be able to gracefully handle that,
> then we need to stop using plain int in all the involved functions.
> I'm curious though which bitmap_fill() it was that you saw misbehave:
> There's no such call at all in xen/drivers/video/, and I'm also having
> a hard time seeing how the address (rather than the size) of the
> frame buffer could be involved here.

Truncated framebuffer address (0x0) caused memset() in vesa_init() to
zero (among other things) mbi->mods_count. This triggered the crash as
described above.
Obviously, bitmap_fill() crash was just a fallout here, not the root
cause.

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 2149 bytes --]

On Tue, May 07, 2019 at 03:10:06AM -0600, Jan Beulich wrote:
> >>> On 06.05.19 at 17:32, <marmarek@invisiblethingslab.com> wrote:
> > On Mon, May 06, 2019 at 05:15:19PM +0200, Juergen Gross wrote:
> >> On 06/05/2019 16:50, Marek Marczykowski-Górecki wrote:
> >> > diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> >> > index ccdffc0..b0f0f7e 100644
> >> > --- a/xen/include/public/xen.h
> >> > +++ b/xen/include/public/xen.h
> >> > @@ -923,6 +923,8 @@ typedef struct dom0_vga_console_info {
> >> >              /* Mode attributes (offset 0x0, VESA command 0x4f01). */
> >> >              uint16_t mode_attrs;
> >> >  #endif
> >> > +            /* high 32 bits of lfb_base */
> >> > +            uint32_t ext_lfb_base;
> >> 
> >> You will need to put this addition into an:
> >> 
> >> #if __XEN_INTERFACE_VERSION__ >= 0x00040d00
> >> ...
> >> #endif
> >> 
> >> section (only available from Xen 4.13 onwards).
> > 
> > Why exactly? I don't see this structure used in any hypercall argument.
> > The only externally accessible place is start_info structure, where it
> > has explicit struct size already.
> 
> In addition to Jürgen's reply: While the structure isn't meant to
> be used that way, any consumer of the Xen headers could in
> principle create instances of it (rather than just using pointers
> to the Xen-provided instance), and without the consuming code
> signaling its awareness such structure sizes may not change.

Ok.

What do you think about adding something that could be backported?
Xen is quite insistent on initializing framebuffer, even with
console=com1 or console=none. Which means, there is no workaround for
this problem.

Maybe, as a first step, a change that abort framebuffer initialization
if lfb_base == 0 (I assume this is never valid value here, right?)?
If not, then at least abort boot when text console is still there
(blexit before efi_exit_boot). Any preference?

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 2149 bytes --]

On Tue, May 07, 2019 at 03:10:06AM -0600, Jan Beulich wrote:
> >>> On 06.05.19 at 17:32, <marmarek@invisiblethingslab.com> wrote:
> > On Mon, May 06, 2019 at 05:15:19PM +0200, Juergen Gross wrote:
> >> On 06/05/2019 16:50, Marek Marczykowski-Górecki wrote:
> >> > diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> >> > index ccdffc0..b0f0f7e 100644
> >> > --- a/xen/include/public/xen.h
> >> > +++ b/xen/include/public/xen.h
> >> > @@ -923,6 +923,8 @@ typedef struct dom0_vga_console_info {
> >> >              /* Mode attributes (offset 0x0, VESA command 0x4f01). */
> >> >              uint16_t mode_attrs;
> >> >  #endif
> >> > +            /* high 32 bits of lfb_base */
> >> > +            uint32_t ext_lfb_base;
> >> 
> >> You will need to put this addition into an:
> >> 
> >> #if __XEN_INTERFACE_VERSION__ >= 0x00040d00
> >> ...
> >> #endif
> >> 
> >> section (only available from Xen 4.13 onwards).
> > 
> > Why exactly? I don't see this structure used in any hypercall argument.
> > The only externally accessible place is start_info structure, where it
> > has explicit struct size already.
> 
> In addition to Jürgen's reply: While the structure isn't meant to
> be used that way, any consumer of the Xen headers could in
> principle create instances of it (rather than just using pointers
> to the Xen-provided instance), and without the consuming code
> signaling its awareness such structure sizes may not change.

Ok.

What do you think about adding something that could be backported?
Xen is quite insistent on initializing framebuffer, even with
console=com1 or console=none. Which means, there is no workaround for
this problem.

Maybe, as a first step, a change that abort framebuffer initialization
if lfb_base == 0 (I assume this is never valid value here, right?)?
If not, then at least abort boot when text console is still there
(blexit before efi_exit_boot). Any preference?

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Jan Beulich]

>>> On 07.05.19 at 17:38, <marmarek@invisiblethingslab.com> wrote:
> What do you think about adding something that could be backported?
> Xen is quite insistent on initializing framebuffer, even with
> console=com1 or console=none. Which means, there is no workaround for
> this problem.

When the system is in a simple text mode the /basevideo option of
xen.efi ought to help, but if it's in an LFB-based mode already (which
is the case on the systems I have) then indeed I can't see any
workaround.

> Maybe, as a first step, a change that abort framebuffer initialization
> if lfb_base == 0 (I assume this is never valid value here, right?)?

Yes, that would be an option. But it would help only in your specific
case, not if the truncation results in a non-zero value. I guess we'd
better avoid filling the structure if we'd truncate the value.

But what's wrong with backporting your change as is?

> If not, then at least abort boot when text console is still there
> (blexit before efi_exit_boot). Any preference?

What's wrong with the text console still active? Or maybe I'm
misunderstanding this point you make...

Jan



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

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

[Jan Beulich]

>>> On 07.05.19 at 17:38, <marmarek@invisiblethingslab.com> wrote:
> What do you think about adding something that could be backported?
> Xen is quite insistent on initializing framebuffer, even with
> console=com1 or console=none. Which means, there is no workaround for
> this problem.

When the system is in a simple text mode the /basevideo option of
xen.efi ought to help, but if it's in an LFB-based mode already (which
is the case on the systems I have) then indeed I can't see any
workaround.

> Maybe, as a first step, a change that abort framebuffer initialization
> if lfb_base == 0 (I assume this is never valid value here, right?)?

Yes, that would be an option. But it would help only in your specific
case, not if the truncation results in a non-zero value. I guess we'd
better avoid filling the structure if we'd truncate the value.

But what's wrong with backporting your change as is?

> If not, then at least abort boot when text console is still there
> (blexit before efi_exit_boot). Any preference?

What's wrong with the text console still active? Or maybe I'm
misunderstanding this point you make...

Jan



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

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

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 2184 bytes --]

On Tue, May 07, 2019 at 10:12:13AM -0600, Jan Beulich wrote:
> >>> On 07.05.19 at 17:38, <marmarek@invisiblethingslab.com> wrote:
> > What do you think about adding something that could be backported?
> > Xen is quite insistent on initializing framebuffer, even with
> > console=com1 or console=none. Which means, there is no workaround for
> > this problem.
> 
> When the system is in a simple text mode the /basevideo option of
> xen.efi ought to help, but if it's in an LFB-based mode already (which
> is the case on the systems I have) then indeed I can't see any
> workaround.
> 
> > Maybe, as a first step, a change that abort framebuffer initialization
> > if lfb_base == 0 (I assume this is never valid value here, right?)?
> 
> Yes, that would be an option. But it would help only in your specific
> case, not if the truncation results in a non-zero value. I guess we'd
> better avoid filling the structure if we'd truncate the value.

Yes, I was thinking about setting lfb_base=0 explicitly if it would
overflow otherwise.

> But what's wrong with backporting your change as is?

If this commit would be backported, what value you'd put in that #ifdef?
Also, one may argue that ABI changes should not be backported... But
since there is clear and independent of xen version method of detecting
it, I don't think this would be a big issue here.

> > If not, then at least abort boot when text console is still there
> > (blexit before efi_exit_boot). Any preference?
> 
> What's wrong with the text console still active? Or maybe I'm
> misunderstandint you make...

As soon as you call ExitBootServices(), you can't use
SIMPLE_TEXT_OUTPUT_INTERFACE anymore. Which means if a) framebuffer
address didn't fit, and b) you called ExitBootServices() already, you
don't have any means to tell the user what is wrong. Other than serial
console of course, if you're lucky enough to have one. So the idea was
to report the problem before ExitBootServices().

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 2184 bytes --]

On Tue, May 07, 2019 at 10:12:13AM -0600, Jan Beulich wrote:
> >>> On 07.05.19 at 17:38, <marmarek@invisiblethingslab.com> wrote:
> > What do you think about adding something that could be backported?
> > Xen is quite insistent on initializing framebuffer, even with
> > console=com1 or console=none. Which means, there is no workaround for
> > this problem.
> 
> When the system is in a simple text mode the /basevideo option of
> xen.efi ought to help, but if it's in an LFB-based mode already (which
> is the case on the systems I have) then indeed I can't see any
> workaround.
> 
> > Maybe, as a first step, a change that abort framebuffer initialization
> > if lfb_base == 0 (I assume this is never valid value here, right?)?
> 
> Yes, that would be an option. But it would help only in your specific
> case, not if the truncation results in a non-zero value. I guess we'd
> better avoid filling the structure if we'd truncate the value.

Yes, I was thinking about setting lfb_base=0 explicitly if it would
overflow otherwise.

> But what's wrong with backporting your change as is?

If this commit would be backported, what value you'd put in that #ifdef?
Also, one may argue that ABI changes should not be backported... But
since there is clear and independent of xen version method of detecting
it, I don't think this would be a big issue here.

> > If not, then at least abort boot when text console is still there
> > (blexit before efi_exit_boot). Any preference?
> 
> What's wrong with the text console still active? Or maybe I'm
> misunderstandint you make...

As soon as you call ExitBootServices(), you can't use
SIMPLE_TEXT_OUTPUT_INTERFACE anymore. Which means if a) framebuffer
address didn't fit, and b) you called ExitBootServices() already, you
don't have any means to tell the user what is wrong. Other than serial
console of course, if you're lucky enough to have one. So the idea was
to report the problem before ExitBootServices().

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Jan Beulich]

>>> On 07.05.19 at 18:43, <marmarek@invisiblethingslab.com> wrote:
> On Tue, May 07, 2019 at 10:12:13AM -0600, Jan Beulich wrote:
>> >>> On 07.05.19 at 17:38, <marmarek@invisiblethingslab.com> wrote:
>> > What do you think about adding something that could be backported?
>> > Xen is quite insistent on initializing framebuffer, even with
>> > console=com1 or console=none. Which means, there is no workaround for
>> > this problem.
>> 
>> When the system is in a simple text mode the /basevideo option of
>> xen.efi ought to help, but if it's in an LFB-based mode already (which
>> is the case on the systems I have) then indeed I can't see any
>> workaround.
>> 
>> > Maybe, as a first step, a change that abort framebuffer initialization
>> > if lfb_base == 0 (I assume this is never valid value here, right?)?
>> 
>> Yes, that would be an option. But it would help only in your specific
>> case, not if the truncation results in a non-zero value. I guess we'd
>> better avoid filling the structure if we'd truncate the value.
> 
> Yes, I was thinking about setting lfb_base=0 explicitly if it would
> overflow otherwise.
> 
>> But what's wrong with backporting your change as is?
> 
> If this commit would be backported, what value you'd put in that #ifdef?

I'd keep it as is. The field addition happens for 4.13. And as you say ...

> Also, one may argue that ABI changes should not be backported... But
> since there is clear and independent of xen version method of detecting
> it, I don't think this would be a big issue here.

... there's not really any issue with surfacing this also in older
versions.

>> > If not, then at least abort boot when text console is still there
>> > (blexit before efi_exit_boot). Any preference?
>> 
>> What's wrong with the text console still active? Or maybe I'm
>> misunderstandint you make...
> 
> As soon as you call ExitBootServices(), you can't use
> SIMPLE_TEXT_OUTPUT_INTERFACE anymore. Which means if a) framebuffer
> address didn't fit, and b) you called ExitBootServices() already, you
> don't have any means to tell the user what is wrong. Other than serial
> console of course, if you're lucky enough to have one. So the idea was
> to report the problem before ExitBootServices().

Oh, so be "text console" you meant the EFI interface, not a
console in text mode (which we can drive). Failing to boot in
such a case seems worse to me than booting effectively
headless.

Jan



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

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

[Jan Beulich]

>>> On 07.05.19 at 18:43, <marmarek@invisiblethingslab.com> wrote:
> On Tue, May 07, 2019 at 10:12:13AM -0600, Jan Beulich wrote:
>> >>> On 07.05.19 at 17:38, <marmarek@invisiblethingslab.com> wrote:
>> > What do you think about adding something that could be backported?
>> > Xen is quite insistent on initializing framebuffer, even with
>> > console=com1 or console=none. Which means, there is no workaround for
>> > this problem.
>> 
>> When the system is in a simple text mode the /basevideo option of
>> xen.efi ought to help, but if it's in an LFB-based mode already (which
>> is the case on the systems I have) then indeed I can't see any
>> workaround.
>> 
>> > Maybe, as a first step, a change that abort framebuffer initialization
>> > if lfb_base == 0 (I assume this is never valid value here, right?)?
>> 
>> Yes, that would be an option. But it would help only in your specific
>> case, not if the truncation results in a non-zero value. I guess we'd
>> better avoid filling the structure if we'd truncate the value.
> 
> Yes, I was thinking about setting lfb_base=0 explicitly if it would
> overflow otherwise.
> 
>> But what's wrong with backporting your change as is?
> 
> If this commit would be backported, what value you'd put in that #ifdef?

I'd keep it as is. The field addition happens for 4.13. And as you say ...

> Also, one may argue that ABI changes should not be backported... But
> since there is clear and independent of xen version method of detecting
> it, I don't think this would be a big issue here.

... there's not really any issue with surfacing this also in older
versions.

>> > If not, then at least abort boot when text console is still there
>> > (blexit before efi_exit_boot). Any preference?
>> 
>> What's wrong with the text console still active? Or maybe I'm
>> misunderstandint you make...
> 
> As soon as you call ExitBootServices(), you can't use
> SIMPLE_TEXT_OUTPUT_INTERFACE anymore. Which means if a) framebuffer
> address didn't fit, and b) you called ExitBootServices() already, you
> don't have any means to tell the user what is wrong. Other than serial
> console of course, if you're lucky enough to have one. So the idea was
> to report the problem before ExitBootServices().

Oh, so be "text console" you meant the EFI interface, not a
console in text mode (which we can drive). Failing to boot in
such a case seems worse to me than booting effectively
headless.

Jan



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

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

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 3809 bytes --]

On Wed, May 08, 2019 at 03:54:45AM -0600, Jan Beulich wrote:
> >>> On 07.05.19 at 18:43, <marmarek@invisiblethingslab.com> wrote:
> > On Tue, May 07, 2019 at 10:12:13AM -0600, Jan Beulich wrote:
> >> >>> On 07.05.19 at 17:38, <marmarek@invisiblethingslab.com> wrote:
> >> > What do you think about adding something that could be backported?
> >> > Xen is quite insistent on initializing framebuffer, even with
> >> > console=com1 or console=none. Which means, there is no workaround for
> >> > this problem.
> >> 
> >> When the system is in a simple text mode the /basevideo option of
> >> xen.efi ought to help, but if it's in an LFB-based mode already (which
> >> is the case on the systems I have) then indeed I can't see any
> >> workaround.
> >> 
> >> > Maybe, as a first step, a change that abort framebuffer initialization
> >> > if lfb_base == 0 (I assume this is never valid value here, right?)?
> >> 
> >> Yes, that would be an option. But it would help only in your specific
> >> case, not if the truncation results in a non-zero value. I guess we'd
> >> better avoid filling the structure if we'd truncate the value.
> > 
> > Yes, I was thinking about setting lfb_base=0 explicitly if it would
> > overflow otherwise.
> > 
> >> But what's wrong with backporting your change as is?
> > 
> > If this commit would be backported, what value you'd put in that #ifdef?
> 
> I'd keep it as is. The field addition happens for 4.13. And as you say ...
> 
> > Also, one may argue that ABI changes should not be backported... But
> > since there is clear and independent of xen version method of detecting
> > it, I don't think this would be a big issue here.
> 
> ... there's not really any issue with surfacing this also in older
> versions.

You mean to keep it without #ifdef then? I'm not following... If you add
#ifdef __XEN_INTERFACE_VERSION__ >= 0x00040d00 there, the field won't be
available in Xen < 4.13. Which effectively means the patch can't be
backported as it won't compile with Xen < 4.13. Note also that this
structure is the place that Xen use to keep that information internally
(xen_vga_console_info is another name for dom0_vga_console_info), it
isn't only about passing this information to dom0.

Maybe add #ifdef __XEN_INTERFACE_VERSION__ >= 0x00040a00, as the oldest
fully supported version? This will mitigate one of the issues with the
lack of #ifdef (potential conflict with gbl_caps with
__XEN_INTERFACE_VERSION__ < 0x00030206).

Or use some #if meaning Xen interface >= 4.13, or Xen internal build?

> >> > If not, then at least abort boot when text console is still there
> >> > (blexit before efi_exit_boot). Any preference?
> >> 
> >> What's wrong with the text console still active? Or maybe I'm
> >> misunderstandint you make...
> > 
> > As soon as you call ExitBootServices(), you can't use
> > SIMPLE_TEXT_OUTPUT_INTERFACE anymore. Which means if a) framebuffer
> > address didn't fit, and b) you called ExitBootServices() already, you
> > don't have any means to tell the user what is wrong. Other than serial
> > console of course, if you're lucky enough to have one. So the idea was
> > to report the problem before ExitBootServices().
> 
> Oh, so be "text console" you meant the EFI interface, not a
> console in text mode (which we can drive). Failing to boot in
> such a case seems worse to me than booting effectively
> headless.

Yes, if the alternative is booting headless, then indeed it's better
than refusing to boot with a message. But if the alternative is a
mysterious crash without any message...

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 3809 bytes --]

On Wed, May 08, 2019 at 03:54:45AM -0600, Jan Beulich wrote:
> >>> On 07.05.19 at 18:43, <marmarek@invisiblethingslab.com> wrote:
> > On Tue, May 07, 2019 at 10:12:13AM -0600, Jan Beulich wrote:
> >> >>> On 07.05.19 at 17:38, <marmarek@invisiblethingslab.com> wrote:
> >> > What do you think about adding something that could be backported?
> >> > Xen is quite insistent on initializing framebuffer, even with
> >> > console=com1 or console=none. Which means, there is no workaround for
> >> > this problem.
> >> 
> >> When the system is in a simple text mode the /basevideo option of
> >> xen.efi ought to help, but if it's in an LFB-based mode already (which
> >> is the case on the systems I have) then indeed I can't see any
> >> workaround.
> >> 
> >> > Maybe, as a first step, a change that abort framebuffer initialization
> >> > if lfb_base == 0 (I assume this is never valid value here, right?)?
> >> 
> >> Yes, that would be an option. But it would help only in your specific
> >> case, not if the truncation results in a non-zero value. I guess we'd
> >> better avoid filling the structure if we'd truncate the value.
> > 
> > Yes, I was thinking about setting lfb_base=0 explicitly if it would
> > overflow otherwise.
> > 
> >> But what's wrong with backporting your change as is?
> > 
> > If this commit would be backported, what value you'd put in that #ifdef?
> 
> I'd keep it as is. The field addition happens for 4.13. And as you say ...
> 
> > Also, one may argue that ABI changes should not be backported... But
> > since there is clear and independent of xen version method of detecting
> > it, I don't think this would be a big issue here.
> 
> ... there's not really any issue with surfacing this also in older
> versions.

You mean to keep it without #ifdef then? I'm not following... If you add
#ifdef __XEN_INTERFACE_VERSION__ >= 0x00040d00 there, the field won't be
available in Xen < 4.13. Which effectively means the patch can't be
backported as it won't compile with Xen < 4.13. Note also that this
structure is the place that Xen use to keep that information internally
(xen_vga_console_info is another name for dom0_vga_console_info), it
isn't only about passing this information to dom0.

Maybe add #ifdef __XEN_INTERFACE_VERSION__ >= 0x00040a00, as the oldest
fully supported version? This will mitigate one of the issues with the
lack of #ifdef (potential conflict with gbl_caps with
__XEN_INTERFACE_VERSION__ < 0x00030206).

Or use some #if meaning Xen interface >= 4.13, or Xen internal build?

> >> > If not, then at least abort boot when text console is still there
> >> > (blexit before efi_exit_boot). Any preference?
> >> 
> >> What's wrong with the text console still active? Or maybe I'm
> >> misunderstandint you make...
> > 
> > As soon as you call ExitBootServices(), you can't use
> > SIMPLE_TEXT_OUTPUT_INTERFACE anymore. Which means if a) framebuffer
> > address didn't fit, and b) you called ExitBootServices() already, you
> > don't have any means to tell the user what is wrong. Other than serial
> > console of course, if you're lucky enough to have one. So the idea was
> > to report the problem before ExitBootServices().
> 
> Oh, so be "text console" you meant the EFI interface, not a
> console in text mode (which we can drive). Failing to boot in
> such a case seems worse to me than booting effectively
> headless.

Yes, if the alternative is booting headless, then indeed it's better
than refusing to boot with a message. But if the alternative is a
mysterious crash without any message...

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Jan Beulich]

>>> On 08.05.19 at 14:06, <marmarek@invisiblethingslab.com> wrote:
> On Wed, May 08, 2019 at 03:54:45AM -0600, Jan Beulich wrote:
>> >>> On 07.05.19 at 18:43, <marmarek@invisiblethingslab.com> wrote:
>> > On Tue, May 07, 2019 at 10:12:13AM -0600, Jan Beulich wrote:
>> >> But what's wrong with backporting your change as is?
>> > 
>> > If this commit would be backported, what value you'd put in that #ifdef?
>> 
>> I'd keep it as is. The field addition happens for 4.13. And as you say ...
>> 
>> > Also, one may argue that ABI changes should not be backported... But
>> > since there is clear and independent of xen version method of detecting
>> > it, I don't think this would be a big issue here.
>> 
>> ... there's not really any issue with surfacing this also in older
>> versions.
> 
> You mean to keep it without #ifdef then? I'm not following... If you add
> #ifdef __XEN_INTERFACE_VERSION__ >= 0x00040d00 there, the field won't be
> available in Xen < 4.13. Which effectively means the patch can't be
> backported as it won't compile with Xen < 4.13. Note also that this
> structure is the place that Xen use to keep that information internally
> (xen_vga_console_info is another name for dom0_vga_console_info), it
> isn't only about passing this information to dom0.

Hmm, yes, I've been mixing up things. It would need to have
"|| defined(__XEN__)" added there.

> Maybe add #ifdef __XEN_INTERFACE_VERSION__ >= 0x00040a00, as the oldest
> fully supported version? This will mitigate one of the issues with the
> lack of #ifdef (potential conflict with gbl_caps with
> __XEN_INTERFACE_VERSION__ < 0x00030206).

That's not an option imo, as only some minor versions of those
major ones would support the new field.

Jan



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

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

[Jan Beulich]

>>> On 08.05.19 at 14:06, <marmarek@invisiblethingslab.com> wrote:
> On Wed, May 08, 2019 at 03:54:45AM -0600, Jan Beulich wrote:
>> >>> On 07.05.19 at 18:43, <marmarek@invisiblethingslab.com> wrote:
>> > On Tue, May 07, 2019 at 10:12:13AM -0600, Jan Beulich wrote:
>> >> But what's wrong with backporting your change as is?
>> > 
>> > If this commit would be backported, what value you'd put in that #ifdef?
>> 
>> I'd keep it as is. The field addition happens for 4.13. And as you say ...
>> 
>> > Also, one may argue that ABI changes should not be backported... But
>> > since there is clear and independent of xen version method of detecting
>> > it, I don't think this would be a big issue here.
>> 
>> ... there's not really any issue with surfacing this also in older
>> versions.
> 
> You mean to keep it without #ifdef then? I'm not following... If you add
> #ifdef __XEN_INTERFACE_VERSION__ >= 0x00040d00 there, the field won't be
> available in Xen < 4.13. Which effectively means the patch can't be
> backported as it won't compile with Xen < 4.13. Note also that this
> structure is the place that Xen use to keep that information internally
> (xen_vga_console_info is another name for dom0_vga_console_info), it
> isn't only about passing this information to dom0.

Hmm, yes, I've been mixing up things. It would need to have
"|| defined(__XEN__)" added there.

> Maybe add #ifdef __XEN_INTERFACE_VERSION__ >= 0x00040a00, as the oldest
> fully supported version? This will mitigate one of the issues with the
> lack of #ifdef (potential conflict with gbl_caps with
> __XEN_INTERFACE_VERSION__ < 0x00030206).

That's not an option imo, as only some minor versions of those
major ones would support the new field.

Jan



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

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

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 3102 bytes --]

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?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 3102 bytes --]

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?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Jan Beulich]

>>> On 09.05.19 at 02:22, <marmarek@invisiblethingslab.com> wrote:
> 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).

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Jan Beulich]

>>> On 09.05.19 at 02:22, <marmarek@invisiblethingslab.com> wrote:
> 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).

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel