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

2016-09-20 Thread Derek Straka 
On Tue, Sep 20, 2016 at 9:12 AM, Jan Beulich  wrote:

> >>> On 20.09.16 at 14:35,  wrote:
> > On Wed, Sep 14, 2016 at 6:47 AM, Jan Beulich  wrote:
> >
> >> >>> On 13.09.16 at 21:40,  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
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

2016-09-20 Thread Jan Beulich 
>>> On 20.09.16 at 14:35,  wrote:
> On Wed, Sep 14, 2016 at 6:47 AM, Jan Beulich  wrote:
> 
>> >>> On 13.09.16 at 21:40,  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: [Xen-devel] [PATCH] x86: add a user configurable Kconfig option for the VGA

2016-09-20 Thread Derek Straka 
On Wed, Sep 14, 2016 at 6:47 AM, Jan Beulich  wrote:

> >>> On 13.09.16 at 21:40,  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 
> > +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: [Xen-devel] [PATCH] x86: add a user configurable Kconfig option for the VGA

2016-09-14 Thread 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: [Xen-devel] [PATCH] x86: add a user configurable Kconfig option for the VGA

2016-09-14 Thread Jan Beulich 
>>> On 13.09.16 at 21:40,  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 
> +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: [Xen-devel] [PATCH] x86: add a user configurable Kconfig option for the VGA

2016-09-13 Thread Doug Goldstein 
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 + 0x) >> 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 

-- 
Doug Goldstein



signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

2016-09-13 Thread 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 
---
 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 + 0x) >> 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 = (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 
+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