[PATCH] x86: add a user configurable Kconfig option for the VGA

[PATCH] x86: add a user configurable Kconfig option for the VGA

[Derek Straka]

Allows for the conditional inclusion of VGA driver on the x86 platform
rather than having it always enabled.

The default configuration for the CONFIG_VGA option remains 'y' on x86, so the
behavior out of the box remains unchanged.  The addition of the option allows
advanced users to enable/disable the inclusion of the VGA driver.

Signed-off-by: Derek Straka <derek@asterius.io>
---
 xen/arch/x86/Kconfig        | 1 -
 xen/arch/x86/efi/efi-boot.h | 7 +++++++
 xen/arch/x86/setup.c        | 5 +++++
 xen/drivers/video/Kconfig   | 3 ++-
 xen/include/asm-x86/setup.h | 5 +++++
 xen/include/xen/console.h   | 8 ++++++++
 6 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 265fd79..9e10591 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -20,7 +20,6 @@ config X86
 	select HAS_PCI
 	select HAS_PDX
 	select NUMA
-	select VGA
 
 config ARCH_DEFCONFIG
 	string
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 10985721..911fdfd 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -476,6 +476,7 @@ static void __init efi_arch_edd(void)
         boot_edd_info_nr = EDD_INFO_MAX;
 }
 
+#ifdef CONFIG_VGA
 static void __init efi_arch_console_init(UINTN cols, UINTN rows)
 {
     vga_console_info.video_type = XEN_VGATYPE_TEXT_MODE_3;
@@ -550,6 +551,12 @@ static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
             (gop->Mode->FrameBufferSize + 0xffff) >> 16;
     }
 }
+#else
+static inline void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
+                                       UINTN info_size,
+                                       EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info) {}
+static inline void __init efi_arch_console_init(UINTN cols, UINTN rows) {}
+#endif
 
 static void __init efi_arch_memory_setup(void)
 {
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 8ae897a..6358336 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -433,10 +433,12 @@ struct boot_video_info {
     u16 vesapm_off;         /* 0x26 */
     u16 vesa_attrib;        /* 0x28 */
 };
+
 extern struct boot_video_info boot_vid_info;
 
 static void __init parse_video_info(void)
 {
+#ifdef CONFIG_VGA
     struct boot_video_info *bvi = &bootsym(boot_vid_info);
 
     /* The EFI loader fills vga_console_info directly. */
@@ -472,6 +474,7 @@ static void __init parse_video_info(void)
         vga_console_info.u.vesa_lfb.gbl_caps = bvi->capabilities;
         vga_console_info.u.vesa_lfb.mode_attrs = bvi->vesa_attrib;
     }
+#endif
 }
 
 static void __init kexec_reserve_area(struct e820map *e820)
@@ -672,6 +675,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     printk("Command line: %s\n", cmdline);
 
+#ifdef CONFIG_VGA
     printk("Video information:\n");
 
     /* Print VGA display mode information. */
@@ -694,6 +698,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         printk(" No VGA detected\n");
         break;
     }
+#endif
 
     /* Print VBE/DDC EDID information. */
     if ( bootsym(boot_edid_caps) != 0x1313 )
diff --git a/xen/drivers/video/Kconfig b/xen/drivers/video/Kconfig
index 0ffbbd9..0f208fe 100644
--- a/xen/drivers/video/Kconfig
+++ b/xen/drivers/video/Kconfig
@@ -3,7 +3,8 @@ config VIDEO
 	bool
 
 config VGA
-	bool
+	bool "VGA"
+	default y if X86
 	select VIDEO
 
 config HAS_ARM_HDLCD
diff --git a/xen/include/asm-x86/setup.h b/xen/include/asm-x86/setup.h
index c65b79c..02e9b12 100644
--- a/xen/include/asm-x86/setup.h
+++ b/xen/include/asm-x86/setup.h
@@ -28,8 +28,13 @@ void arch_init_memory(void);
 void subarch_init_memory(void);
 
 void init_IRQ(void);
+#ifdef CONFIG_VGA
 void vesa_init(void);
 void vesa_mtrr_init(void);
+#else
+static inline void vesa_init(void) {}
+static inline void vesa_mtrr_init(void) {}
+#endif
 
 int construct_dom0(
     struct domain *d,
diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h
index ea06fd8..2e7c22c 100644
--- a/xen/include/xen/console.h
+++ b/xen/include/xen/console.h
@@ -19,7 +19,15 @@ void console_init_postirq(void);
 void console_endboot(void);
 int console_has(const char *device);
 
+#ifdef CONFIG_VGA
 int fill_console_start_info(struct dom0_vga_console_info *);
+#else
+#include <xen/string.h>
+static inline int fill_console_start_info(struct dom0_vga_console_info *ci) {
+    (void) memset(ci, 0, sizeof(*ci));
+    return 1;
+}
+#endif
 
 unsigned long console_lock_recursive_irqsave(void);
 void console_unlock_recursive_irqrestore(unsigned long flags);
-- 
2.7.4


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

Re: [PATCH] x86: add a user configurable Kconfig option for the VGA

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 2238 bytes --]

On 9/13/16 2:40 PM, Derek Straka wrote:

> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 10985721..911fdfd 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -476,6 +476,7 @@ static void __init efi_arch_edd(void)
>          boot_edd_info_nr = EDD_INFO_MAX;
>  }
>  
> +#ifdef CONFIG_VGA
>  static void __init efi_arch_console_init(UINTN cols, UINTN rows)
>  {
>      vga_console_info.video_type = XEN_VGATYPE_TEXT_MODE_3;
> @@ -550,6 +551,12 @@ static void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
>              (gop->Mode->FrameBufferSize + 0xffff) >> 16;
>      }
>  }
> +#else
> +static inline void __init efi_arch_video_init(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
> +                                       UINTN info_size,
> +                                       EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *mode_info) {}
> +static inline void __init efi_arch_console_init(UINTN cols, UINTN rows) {}
> +#endif

I'd technically intent these parameters to match the others.


> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 8ae897a..6358336 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -433,10 +433,12 @@ struct boot_video_info {
>      u16 vesapm_off;         /* 0x26 */
>      u16 vesa_attrib;        /* 0x28 */
>  };
> +
>  extern struct boot_video_info boot_vid_info;
>  

Extra white space. But honestly it makes it easier to read.


>  
>  static void __init kexec_reserve_area(struct e820map *e820)
> @@ -672,6 +675,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>      printk("Command line: %s\n", cmdline);
>  
> +#ifdef CONFIG_VGA
>      printk("Video information:\n");
>  
>      /* Print VGA display mode information. */
> @@ -694,6 +698,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>          printk(" No VGA detected\n");
>          break;
>      }
> +#endif

Not sure if the else case should be something like:
	printk("Video information: None\n");


I'll leave it up to others to concur or disagree but if people disagree
then:
Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

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

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

Re: [PATCH] x86: add a user configurable Kconfig option for the VGA

[Jan Beulich]

>>> On 13.09.16 at 21:40, <derek@asterius.io> wrote:
> Allows for the conditional inclusion of VGA driver on the x86 platform
> rather than having it always enabled.

So I guess with all three of these patches an overview mail is missing.
What are you trying to accomplish? Solely reducing the binary size of
Xen doesn't seem like a very important goal to me, and eliminating
these drivers from the build doesn't appear to help make Xen more
stable of secure.

> @@ -672,6 +675,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>      printk("Command line: %s\n", cmdline);
>  
> +#ifdef CONFIG_VGA
>      printk("Video information:\n");

Some of the other conditionals you add may be affected too, but
here it is most prominent at the first glance - considering that we also
have CONFIG_VIDEO, wouldn't it rather be that one to be used in a
place like this one?

> --- a/xen/include/xen/console.h
> +++ b/xen/include/xen/console.h
> @@ -19,7 +19,15 @@ void console_init_postirq(void);
>  void console_endboot(void);
>  int console_has(const char *device);
>  
> +#ifdef CONFIG_VGA
>  int fill_console_start_info(struct dom0_vga_console_info *);
> +#else
> +#include <xen/string.h>
> +static inline int fill_console_start_info(struct dom0_vga_console_info *ci) 
> {
> +    (void) memset(ci, 0, sizeof(*ci));

What is this cast to void goo for?

Jan


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

Re: [PATCH] x86: add a user configurable Kconfig option for the VGA

[Julien Grall]

Hello Derek,

On 13/09/16 20:40, Derek Straka wrote:
> diff --git a/xen/drivers/video/Kconfig b/xen/drivers/video/Kconfig
> index 0ffbbd9..0f208fe 100644
> --- a/xen/drivers/video/Kconfig
> +++ b/xen/drivers/video/Kconfig
> @@ -3,7 +3,8 @@ config VIDEO
>  	bool
>
>  config VGA
> -	bool
> +	bool "VGA"
> +	default y if X86

Same remark as for the NS16550 change. This option cannot be enabled on 
ARM and will break compilation of randconfig.

Regards,

>  	select VIDEO
>
>  config HAS_ARM_HDLCD

Regards,

-- 
Julien Grall

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

Re: [PATCH] x86: add a user configurable Kconfig option for the VGA

[Derek Straka]

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

On Wed, Sep 14, 2016 at 6:47 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 13.09.16 at 21:40, <derek@asterius.io> wrote:
> > Allows for the conditional inclusion of VGA driver on the x86 platform
> > rather than having it always enabled.
>
> So I guess with all three of these patches an overview mail is missing.
> What are you trying to accomplish? Solely reducing the binary size of
> Xen doesn't seem like a very important goal to me, and eliminating
> these drivers from the build doesn't appear to help make Xen more
> stable of secure.
>
I agree with your assessment on the stability and security standpoint.  Our
customer has asked us to remove
unused drivers based on functionality of a set of boards.  Each of the
boards has a subset of the available hardware functionality
brought out to accessible headers.  I decided to try to make these items
optional via the Kconfig mechanisms already
in place in Xen and contribute the modifications upstream.  I appreciate
all of the feedback on the patches, but if they won't
get accepted upstream because they aren't useful, I don't want to continue
to waste people's time reviewing these changes.

-Derek

>
> > @@ -672,6 +675,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >
> >      printk("Command line: %s\n", cmdline);
> >
> > +#ifdef CONFIG_VGA
> >      printk("Video information:\n");
>
> Some of the other conditionals you add may be affected too, but
> here it is most prominent at the first glance - considering that we also
> have CONFIG_VIDEO, wouldn't it rather be that one to be used in a
> place like this one?
>
> > --- a/xen/include/xen/console.h
> > +++ b/xen/include/xen/console.h
> > @@ -19,7 +19,15 @@ void console_init_postirq(void);
> >  void console_endboot(void);
> >  int console_has(const char *device);
> >
> > +#ifdef CONFIG_VGA
> >  int fill_console_start_info(struct dom0_vga_console_info *);
> > +#else
> > +#include <xen/string.h>
> > +static inline int fill_console_start_info(struct dom0_vga_console_info
> *ci)
> > {
> > +    (void) memset(ci, 0, sizeof(*ci));
>
> What is this cast to void goo for?
>
> Jan
>

[-- Attachment #1.2: Type: text/html, Size: 2989 bytes --]

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

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

Re: [PATCH] x86: add a user configurable Kconfig option for the VGA

[Jan Beulich]

>>> On 20.09.16 at 14:35, <derek@asterius.io> wrote:
> On Wed, Sep 14, 2016 at 6:47 AM, Jan Beulich <JBeulich@suse.com> wrote:
> 
>> >>> On 13.09.16 at 21:40, <derek@asterius.io> wrote:
>> > Allows for the conditional inclusion of VGA driver on the x86 platform
>> > rather than having it always enabled.
>>
>> So I guess with all three of these patches an overview mail is missing.
>> What are you trying to accomplish? Solely reducing the binary size of
>> Xen doesn't seem like a very important goal to me, and eliminating
>> these drivers from the build doesn't appear to help make Xen more
>> stable of secure.
>>
> I agree with your assessment on the stability and security standpoint.  Our
> customer has asked us to remove
> unused drivers based on functionality of a set of boards.  Each of the
> boards has a subset of the available hardware functionality
> brought out to accessible headers.

Well, does that mean that's just to reduce the size of the hypervisor?
If so, I'm honestly not sure we want to set a precedent here, since
if we do, people could come and suggest to make all sorts of code
build conditionally, and I don't think our plans with Kconfig so far were
going in that direction (but others may disagree with me here).

Jan


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

Re: [PATCH] x86: add a user configurable Kconfig option for the VGA

[Derek Straka]

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

On Tue, Sep 20, 2016 at 9:12 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 20.09.16 at 14:35, <derek@asterius.io> wrote:
> > On Wed, Sep 14, 2016 at 6:47 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >
> >> >>> On 13.09.16 at 21:40, <derek@asterius.io> wrote:
> >> > Allows for the conditional inclusion of VGA driver on the x86 platform
> >> > rather than having it always enabled.
> >>
> >> So I guess with all three of these patches an overview mail is missing.
> >> What are you trying to accomplish? Solely reducing the binary size of
> >> Xen doesn't seem like a very important goal to me, and eliminating
> >> these drivers from the build doesn't appear to help make Xen more
> >> stable of secure.
> >>
> > I agree with your assessment on the stability and security standpoint.
> Our
> > customer has asked us to remove
> > unused drivers based on functionality of a set of boards.  Each of the
> > boards has a subset of the available hardware functionality
> > brought out to accessible headers.
>
> Well, does that mean that's just to reduce the size of the hypervisor?
> If so, I'm honestly not sure we want to set a precedent here, since
> if we do, people could come and suggest to make all sorts of code
> build conditionally, and I don't think our plans with Kconfig so far were
> going in that direction (but others may disagree with me here).
>
> For the most part: yes.  At the end of the day, my customer wants to
reduce the size of hypervisor.  I disagree a bit that these specific
changes
would set a poor precedent of for configuration.  The reason I proposed it
in the first place was the mechanisms for conditional compilation
were already implicitly available via HAS_*.  I thought adding the ability
for the user to explicitly define the configuration option
would be a permissible extension of the capability already present.  That
said, I do see your point about limiting the scope of conditional
code to avoid an eventual mess.  Thanks.

-Derek

> Jan
>

[-- Attachment #1.2: Type: text/html, Size: 2976 bytes --]

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

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