diff mbox series

spapr: select default vty

Message ID 20170926121750.9015-1-lvivier@redhat.com
State New
Headers show
Series spapr: select default vty | expand

Commit Message

Laurent Vivier Sept. 26, 2017, 12:17 p.m. UTC
SLOF uses the "/chosen/linux,stdout-path" variable to
choose its console. This variable is provided by QEMU.
QEMU selects the spapr-vty using the "reg" property:
it takes the vty with the lowest reg number.
This patch allows the user to define "linux,stdout-path"
from the command line by adding a keyword 'default' to
the spapr-vty device.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/char/spapr_vty.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

David Gibson Sept. 27, 2017, 7:59 a.m. UTC | #1
On Tue, Sep 26, 2017 at 02:17:50PM +0200, Laurent Vivier wrote:
> SLOF uses the "/chosen/linux,stdout-path" variable to
> choose its console. This variable is provided by QEMU.
> QEMU selects the spapr-vty using the "reg" property:
> it takes the vty with the lowest reg number.
> This patch allows the user to define "linux,stdout-path"
> from the command line by adding a keyword 'default' to
> the spapr-vty device.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

I'm a bit dubious about the worth of this.  With your SLOF fixes it
should still be possible to redirect output correctly using -prom-env,
yes?  Using a secondary vty is a pretty niche case, so I'm not
convinced we need extra properties just to make it simpler.
> ---
>  hw/char/spapr_vty.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
> index 0fa416ca6b..aa56a9a6cb 100644
> --- a/hw/char/spapr_vty.c
> +++ b/hw/char/spapr_vty.c
> @@ -15,6 +15,7 @@ typedef struct VIOsPAPRVTYDevice {
>      CharBackend chardev;
>      uint32_t in, out;
>      uint8_t buf[VTERM_BUFSIZE];
> +    bool is_default;
>  } VIOsPAPRVTYDevice;
>  
>  #define TYPE_VIO_SPAPR_VTY_DEVICE "spapr-vty"
> @@ -153,6 +154,7 @@ void spapr_vty_create(VIOsPAPRBus *bus, Chardev *chardev)
>  static Property spapr_vty_properties[] = {
>      DEFINE_SPAPR_PROPERTIES(VIOsPAPRVTYDevice, sdev),
>      DEFINE_PROP_CHR("chardev", VIOsPAPRVTYDevice, chardev),
> +    DEFINE_PROP_BOOL("default", VIOsPAPRVTYDevice, is_default, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -194,11 +196,13 @@ static const TypeInfo spapr_vty_info = {
>  VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus)
>  {
>      VIOsPAPRDevice *sdev, *selected;
> +    VIOsPAPRVTYDevice *dev;
>      BusChild *kid;
>  
>      /*
>       * To avoid the console bouncing around we want one VTY to be
> -     * the "default". We haven't really got anything to go on, so
> +     * the "default". If the user doesn't provide the information
> +     * we haven't really got anything to go on, so
>       * arbitrarily choose the one with the lowest reg value.
>       */
>  
> @@ -213,6 +217,13 @@ VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus)
>  
>          sdev = VIO_SPAPR_DEVICE(iter);
>  
> +        /* The user can provide the default console to use */
> +
> +        dev = VIO_SPAPR_VTY_DEVICE(sdev);
> +        if (dev->is_default) {
> +            return sdev;
> +        }
> +
>          /* First VTY we've found, so it is selected for now */
>          if (!selected) {
>              selected = sdev;
Laurent Vivier Sept. 27, 2017, 8:30 a.m. UTC | #2
On 27/09/2017 09:59, David Gibson wrote:
> On Tue, Sep 26, 2017 at 02:17:50PM +0200, Laurent Vivier wrote:
>> SLOF uses the "/chosen/linux,stdout-path" variable to
>> choose its console. This variable is provided by QEMU.
>> QEMU selects the spapr-vty using the "reg" property:
>> it takes the vty with the lowest reg number.
>> This patch allows the user to define "linux,stdout-path"
>> from the command line by adding a keyword 'default' to
>> the spapr-vty device.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> 
> I'm a bit dubious about the worth of this.  With your SLOF fixes it
> should still be possible to redirect output correctly using -prom-env,
> yes?  Using a secondary vty is a pretty niche case, so I'm not
> convinced we need extra properties just to make it simpler.

I agree, but there is a difference between the SLOF fix and the QEMU change:
- with the QEMU fix, all logs go immediately to the selected console,
- with SLOF fix, logs start to go on the console with the lowest reg
value, and switch later to the selected console.

Moreover, Thomas has pointed out that the SLOF fix slows the characters
output (x 6).

Thanks,
Laurent
diff mbox series

Patch

diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
index 0fa416ca6b..aa56a9a6cb 100644
--- a/hw/char/spapr_vty.c
+++ b/hw/char/spapr_vty.c
@@ -15,6 +15,7 @@  typedef struct VIOsPAPRVTYDevice {
     CharBackend chardev;
     uint32_t in, out;
     uint8_t buf[VTERM_BUFSIZE];
+    bool is_default;
 } VIOsPAPRVTYDevice;
 
 #define TYPE_VIO_SPAPR_VTY_DEVICE "spapr-vty"
@@ -153,6 +154,7 @@  void spapr_vty_create(VIOsPAPRBus *bus, Chardev *chardev)
 static Property spapr_vty_properties[] = {
     DEFINE_SPAPR_PROPERTIES(VIOsPAPRVTYDevice, sdev),
     DEFINE_PROP_CHR("chardev", VIOsPAPRVTYDevice, chardev),
+    DEFINE_PROP_BOOL("default", VIOsPAPRVTYDevice, is_default, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -194,11 +196,13 @@  static const TypeInfo spapr_vty_info = {
 VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus)
 {
     VIOsPAPRDevice *sdev, *selected;
+    VIOsPAPRVTYDevice *dev;
     BusChild *kid;
 
     /*
      * To avoid the console bouncing around we want one VTY to be
-     * the "default". We haven't really got anything to go on, so
+     * the "default". If the user doesn't provide the information
+     * we haven't really got anything to go on, so
      * arbitrarily choose the one with the lowest reg value.
      */
 
@@ -213,6 +217,13 @@  VIOsPAPRDevice *spapr_vty_get_default(VIOsPAPRBus *bus)
 
         sdev = VIO_SPAPR_DEVICE(iter);
 
+        /* The user can provide the default console to use */
+
+        dev = VIO_SPAPR_VTY_DEVICE(sdev);
+        if (dev->is_default) {
+            return sdev;
+        }
+
         /* First VTY we've found, so it is selected for now */
         if (!selected) {
             selected = sdev;