[Qemu-devel] [Qemu-ppc][PATCH v5 0/4] Add USB option and enable vga on spapr

[Qemu-devel] [Qemu-ppc][PATCH v5 0/4] Add USB option and enable vga on spapr

[zhlcindy]

From: Li Zhang <zhlcindy@linux.vnet.ibm.com>

v1 -> v2:
    * Convert USB option from char to bool.

v2 -> v3:
    * Remove global USB option
    * Get USB option with qemu_opt_get_bool().
    * Send vga patch for sPAPR which requires USB enabled.

v3 -> v4:
    * Fix some English grammar and coding style faults
    * Replace usb_on with add_usb to be more functional.
    * Remove vga init functions' declearations from pc.h,
       and add one new file vga-pci.h for the declearations.
    * Cleanup pc.h on some platforms and add vga-pci.h
    * Remove graphic devices which are not supported on sPAPR platform.
      They will be added back when they are supported.

v4 -> v5:
    * Correct several English words
    * Add header file "qemu-comman.h" in vga-pci.h, which defines PCIBus 
      and DeviceState types.
      Move #endif to the end of the vga-pci.h and remove the trailing 
      white line.

Benjamin Herrenschmidt (1):
  spapr: Add support for -vga option

Li Zhang (3):
  Add usb option in machine options
  Add one new file vga-pci.h
  Cleanup pc.h on other platforms

 hw/alpha_pci.c    |    1 +
 hw/cirrus_vga.c   |    2 +-
 hw/mips_malta.c   |    1 +
 hw/pc.c           |    1 +
 hw/pc.h           |    4 ----
 hw/ppc_newworld.c |    2 +-
 hw/ppc_oldworld.c |    2 +-
 hw/ppc_prep.c     |    1 +
 hw/spapr.c        |   40 +++++++++++++++++++++++++++++++++++++++-
 hw/sun4u.c        |    1 +
 hw/vga-pci.c      |    2 +-
 hw/vga-pci.h      |   13 +++++++++++++
 qemu-config.c     |    4 ++++
 13 files changed, 65 insertions(+), 9 deletions(-)
 create mode 100644 hw/vga-pci.h

-- 
1.7.9.5

[Qemu-devel] [Qemu-ppc][PATCH v5 1/4] Add usb option in machine options

[zhlcindy]

From: Li Zhang <zhlcindy@linux.vnet.ibm.com>

pSeries machine needs to enable USB to add a USB
keyboard or USB mouse. -usb option won't be used in
the future, and machine options are a better way to
enable USB.

So this patch is to add USB option to machine options
(-machine type=pseries,usb=on/off) to enable/disable
USB controller. And machines will get the machine option
and create a USB controller if USB is on.

By the way, USB is on by default on pSeries machine.
So USB controller should be turned off explicitly through
the command line for "-nodefault" case as the following:
 -machine type=pseries,usb=off.

Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
---
 hw/spapr.c    |   11 +++++++++++
 qemu-config.c |    4 ++++
 2 files changed, 15 insertions(+)

diff --git a/hw/spapr.c b/hw/spapr.c
index 81c9343..973de1b 100644
--- a/hw/spapr.c
+++ b/hw/spapr.c
@@ -575,6 +575,8 @@ static void ppc_spapr_init(ram_addr_t ram_size,
     long load_limit, rtas_limit, fw_size;
     long pteg_shift = 17;
     char *filename;
+    QemuOpts *machine_opts;
+    bool add_usb = true;
 
     spapr = g_malloc0(sizeof(*spapr));
     QLIST_INIT(&spapr->phbs);
@@ -710,6 +712,15 @@ static void ppc_spapr_init(ram_addr_t ram_size,
         spapr_vscsi_create(spapr->vio_bus);
     }
 
+    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
+    if (machine_opts) {
+        add_usb = qemu_opt_get_bool(machine_opts, "usb", true);
+    }
+
+    if (add_usb) {
+        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
+                          -1, "pci-ohci");
+    }
     if (rma_size < (MIN_RMA_SLOF << 20)) {
         fprintf(stderr, "qemu: pSeries SLOF firmware requires >= "
                 "%ldM guest RMA (Real Mode Area memory)\n", MIN_RMA_SLOF);
diff --git a/qemu-config.c b/qemu-config.c
index 5c3296b..b86ee36 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -595,6 +595,10 @@ static QemuOptsList qemu_machine_opts = {
             .name = "dt_compatible",
             .type = QEMU_OPT_STRING,
             .help = "Overrides the \"compatible\" property of the dt root node",
+        },{
+            .name = "usb",
+            .type = QEMU_OPT_BOOL,
+            .help = "Set on/off to enable/disable usb",
         },
         { /* End of list */ }
     },
-- 
1.7.9.5

[Qemu-devel] [Qemu-ppc][PATCH v5 2/4] Add one new file vga-pci.h

[zhlcindy]

From: Li Zhang <zhlcindy@linux.vnet.ibm.com>

Functions pci_vga_init() and pci_cirrus_vga_init() are declared
in pc.h. That prevents other platforms (e.g. sPAPR) to use them.

This patch is to create one new file vga-pci.h and move the
declarations to vga-pci.h, so that they can be shared by
all the platforms.

Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
---
 hw/cirrus_vga.c |    2 +-
 hw/pc.h         |    4 ----
 hw/vga-pci.c    |    2 +-
 hw/vga-pci.h    |   13 +++++++++++++
 4 files changed, 15 insertions(+), 6 deletions(-)
 create mode 100644 hw/vga-pci.h

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 623dd68..3e8e5bb 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -27,11 +27,11 @@
  *   available at http://home.worldonline.dk/~finth/
  */
 #include "hw.h"
-#include "pc.h"
 #include "pci.h"
 #include "console.h"
 #include "vga_int.h"
 #include "loader.h"
+#include "vga-pci.h"
 
 /*
  * TODO:
diff --git a/hw/pc.h b/hw/pc.h
index 31ccb6f..e4db071 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -189,14 +189,10 @@ static inline DeviceState *isa_vga_init(ISABus *bus)
     return &dev->qdev;
 }
 
-DeviceState *pci_vga_init(PCIBus *bus);
 int isa_vga_mm_init(target_phys_addr_t vram_base,
                     target_phys_addr_t ctrl_base, int it_shift,
                     MemoryRegion *address_space);
 
-/* cirrus_vga.c */
-DeviceState *pci_cirrus_vga_init(PCIBus *bus);
-
 /* ne2000.c */
 static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd)
 {
diff --git a/hw/vga-pci.c b/hw/vga-pci.c
index 37dc019..4872056 100644
--- a/hw/vga-pci.c
+++ b/hw/vga-pci.c
@@ -23,12 +23,12 @@
  */
 #include "hw.h"
 #include "console.h"
-#include "pc.h"
 #include "pci.h"
 #include "vga_int.h"
 #include "pixel_ops.h"
 #include "qemu-timer.h"
 #include "loader.h"
+#include "vga-pci.h"
 
 typedef struct PCIVGAState {
     PCIDevice dev;
diff --git a/hw/vga-pci.h b/hw/vga-pci.h
new file mode 100644
index 0000000..e272deb
--- /dev/null
+++ b/hw/vga-pci.h
@@ -0,0 +1,13 @@
+#ifndef VGA_PCI_H
+#define VGA_PCI_H
+
+#include "qemu-common.h"
+#include "qdev.h"
+
+/* vga-pci.c */
+DeviceState *pci_vga_init(PCIBus *bus);
+
+/* cirrus_vga.c */
+DeviceState *pci_cirrus_vga_init(PCIBus *bus);
+
+#endif
-- 
1.7.9.5

[Qemu-devel] [Qemu-ppc][PATCH v5 3/4] Cleanup pc.h on other platforms

[zhlcindy]

From: Li Zhang <zhlcindy@linux.vnet.ibm.com>

The declarations of pci_vga_init() and pci_cirrus_vga_init()
are moved to vga-pci.h to be called by all the platforms.
So it's necessary to cleanup pc.h on the platforms other than
PC which include the file and add vga-pci.h on all the plaforms
to call vga related functions.

This patch is to cleanup pc.h and add vga-pci.h.

Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
---
 hw/alpha_pci.c    |    1 +
 hw/mips_malta.c   |    1 +
 hw/pc.c           |    1 +
 hw/ppc_newworld.c |    2 +-
 hw/ppc_oldworld.c |    2 +-
 hw/ppc_prep.c     |    1 +
 hw/sun4u.c        |    1 +
 7 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/alpha_pci.c b/hw/alpha_pci.c
index 6735577..ea546f8 100644
--- a/hw/alpha_pci.c
+++ b/hw/alpha_pci.c
@@ -11,6 +11,7 @@
 #include "qemu-log.h"
 #include "sysemu.h"
 #include "vmware_vga.h"
+#include "vga-pci.h"
 
 
 /* PCI IO reads/writes, to byte-word addressable memory.  */
diff --git a/hw/mips_malta.c b/hw/mips_malta.c
index 351c88e..ad23f26 100644
--- a/hw/mips_malta.c
+++ b/hw/mips_malta.c
@@ -48,6 +48,7 @@
 #include "blockdev.h"
 #include "exec-memory.h"
 #include "sysbus.h"             /* SysBusDevice */
+#include "vga-pci.h"
 
 //#define DEBUG_BOARD_INIT
 
diff --git a/hw/pc.c b/hw/pc.c
index c7e9ab3..f43e817 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -48,6 +48,7 @@
 #include "memory.h"
 #include "exec-memory.h"
 #include "arch_init.h"
+#include "vga-pci.h"
 
 /* output Bochs bios info messages */
 //#define DEBUG_BIOS
diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
index 4e2a6e6..e95cfe8 100644
--- a/hw/ppc_newworld.c
+++ b/hw/ppc_newworld.c
@@ -52,7 +52,6 @@
 #include "adb.h"
 #include "mac_dbdma.h"
 #include "nvram.h"
-#include "pc.h"
 #include "pci.h"
 #include "net.h"
 #include "sysemu.h"
@@ -68,6 +67,7 @@
 #include "hw/usb.h"
 #include "blockdev.h"
 #include "exec-memory.h"
+#include "vga-pci.h"
 
 #define MAX_IDE_BUS 2
 #define CFG_ADDR 0xf0000510
diff --git a/hw/ppc_oldworld.c b/hw/ppc_oldworld.c
index f2c6908..1dcd8a6 100644
--- a/hw/ppc_oldworld.c
+++ b/hw/ppc_oldworld.c
@@ -29,7 +29,6 @@
 #include "adb.h"
 #include "mac_dbdma.h"
 #include "nvram.h"
-#include "pc.h"
 #include "sysemu.h"
 #include "net.h"
 #include "isa.h"
@@ -44,6 +43,7 @@
 #include "kvm_ppc.h"
 #include "blockdev.h"
 #include "exec-memory.h"
+#include "vga-pci.h"
 
 #define MAX_IDE_BUS 2
 #define CFG_ADDR 0xf0000510
diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
index be2b268..7a87616 100644
--- a/hw/ppc_prep.c
+++ b/hw/ppc_prep.c
@@ -39,6 +39,7 @@
 #include "blockdev.h"
 #include "arch_init.h"
 #include "exec-memory.h"
+#include "vga-pci.h"
 
 //#define HARD_DEBUG_PPC_IO
 //#define DEBUG_PPC_IO
diff --git a/hw/sun4u.c b/hw/sun4u.c
index 137a7c6..07cd042 100644
--- a/hw/sun4u.c
+++ b/hw/sun4u.c
@@ -39,6 +39,7 @@
 #include "elf.h"
 #include "blockdev.h"
 #include "exec-memory.h"
+#include "vga-pci.h"
 
 //#define DEBUG_IRQ
 //#define DEBUG_EBUS
-- 
1.7.9.5

[Qemu-devel] [Qemu-ppc][PATCH v5 4/4] spapr: Add support for -vga option

[zhlcindy]

From: Li Zhang <zhlcindy@linux.vnet.ibm.com>

Also instanciate the USB keyboard and mouse when that option is used
(you can still use -device to create individual devices without all
the defaults)

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
---
 hw/spapr.c |   29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/hw/spapr.c b/hw/spapr.c
index 973de1b..3648cad 100644
--- a/hw/spapr.c
+++ b/hw/spapr.c
@@ -45,6 +45,8 @@
 #include "kvm.h"
 #include "kvm_ppc.h"
 #include "pci.h"
+#include "vga-pci.h"
+#include "usb.h"
 
 #include "exec-memory.h"
 
@@ -82,6 +84,7 @@
 #define PHANDLE_XICP            0x00001111
 
 sPAPREnvironment *spapr;
+bool spapr_has_graphics;
 
 qemu_irq spapr_allocate_irq(uint32_t hint, uint32_t *irq_num,
                             enum xics_irq_type type)
@@ -257,6 +260,9 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
         _FDT((fdt_property(fdt, "qemu,boot-kernel", &kprop, sizeof(kprop))));
     }
     _FDT((fdt_property_string(fdt, "qemu,boot-device", boot_device)));
+    _FDT((fdt_property_cell(fdt, "qemu,graphic-width", graphic_width)));
+    _FDT((fdt_property_cell(fdt, "qemu,graphic-height", graphic_height)));
+    _FDT((fdt_property_cell(fdt, "qemu,graphic-depth", graphic_depth)));
 
     _FDT((fdt_end_node(fdt)));
 
@@ -503,7 +509,9 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
         }
     }
 
-    spapr_populate_chosen_stdout(fdt, spapr->vio_bus);
+    if (!spapr_has_graphics) {
+        spapr_populate_chosen_stdout(fdt, spapr->vio_bus);
+    }
 
     _FDT((fdt_pack(fdt)));
 
@@ -556,6 +564,16 @@ static void spapr_cpu_reset(void *opaque)
     cpu_reset(CPU(cpu));
 }
 
+static int spapr_vga_init(PCIBus *pci_bus)
+{
+    if (std_vga_enabled) {
+        pci_vga_init(pci_bus);
+    } else {
+        return 0;
+    }
+    return 1;
+}
+
 /* pSeries LPAR / sPAPR hardware init */
 static void ppc_spapr_init(ram_addr_t ram_size,
                            const char *boot_device,
@@ -712,6 +730,11 @@ static void ppc_spapr_init(ram_addr_t ram_size,
         spapr_vscsi_create(spapr->vio_bus);
     }
 
+    /* Graphics */
+    if (spapr_vga_init(QLIST_FIRST(&spapr->phbs)->host_state.bus)) {
+        spapr_has_graphics = true;
+    }
+
     machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
     if (machine_opts) {
         add_usb = qemu_opt_get_bool(machine_opts, "usb", true);
@@ -720,6 +743,10 @@ static void ppc_spapr_init(ram_addr_t ram_size,
     if (add_usb) {
         pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
                           -1, "pci-ohci");
+        if (spapr_has_graphics) {
+            usbdevice_create("keyboard");
+            usbdevice_create("mouse");
+        }
     }
     if (rma_size < (MIN_RMA_SLOF << 20)) {
         fprintf(stderr, "qemu: pSeries SLOF firmware requires >= "
-- 
1.7.9.5

Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 2/4] Add one new file vga-pci.h

[Li Zhang]

Hi Alex,

Would you please help review these patches, if you have time?
This patch should work with [3/4], which cleanup all the places where
pci_vga_init() and pci_cirrus_vga_init() are used.

Thanks.

On Mon, Jul 2, 2012 at 1:25 PM,  <zhlcindy@gmail.com> wrote:
> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>
> Functions pci_vga_init() and pci_cirrus_vga_init() are declared
> in pc.h. That prevents other platforms (e.g. sPAPR) to use them.
>
> This patch is to create one new file vga-pci.h and move the
> declarations to vga-pci.h, so that they can be shared by
> all the platforms.
>
> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
> ---
>  hw/cirrus_vga.c |    2 +-
>  hw/pc.h         |    4 ----
>  hw/vga-pci.c    |    2 +-
>  hw/vga-pci.h    |   13 +++++++++++++
>  4 files changed, 15 insertions(+), 6 deletions(-)
>  create mode 100644 hw/vga-pci.h
>
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index 623dd68..3e8e5bb 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -27,11 +27,11 @@
>   *   available at http://home.worldonline.dk/~finth/
>   */
>  #include "hw.h"
> -#include "pc.h"
>  #include "pci.h"
>  #include "console.h"
>  #include "vga_int.h"
>  #include "loader.h"
> +#include "vga-pci.h"
>
>  /*
>   * TODO:
> diff --git a/hw/pc.h b/hw/pc.h
> index 31ccb6f..e4db071 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -189,14 +189,10 @@ static inline DeviceState *isa_vga_init(ISABus *bus)
>      return &dev->qdev;
>  }
>
> -DeviceState *pci_vga_init(PCIBus *bus);
>  int isa_vga_mm_init(target_phys_addr_t vram_base,
>                      target_phys_addr_t ctrl_base, int it_shift,
>                      MemoryRegion *address_space);
>
> -/* cirrus_vga.c */
> -DeviceState *pci_cirrus_vga_init(PCIBus *bus);
> -
>  /* ne2000.c */
>  static inline bool isa_ne2000_init(ISABus *bus, int base, int irq, NICInfo *nd)
>  {
> diff --git a/hw/vga-pci.c b/hw/vga-pci.c
> index 37dc019..4872056 100644
> --- a/hw/vga-pci.c
> +++ b/hw/vga-pci.c
> @@ -23,12 +23,12 @@
>   */
>  #include "hw.h"
>  #include "console.h"
> -#include "pc.h"
>  #include "pci.h"
>  #include "vga_int.h"
>  #include "pixel_ops.h"
>  #include "qemu-timer.h"
>  #include "loader.h"
> +#include "vga-pci.h"
>
>  typedef struct PCIVGAState {
>      PCIDevice dev;
> diff --git a/hw/vga-pci.h b/hw/vga-pci.h
> new file mode 100644
> index 0000000..e272deb
> --- /dev/null
> +++ b/hw/vga-pci.h
> @@ -0,0 +1,13 @@
> +#ifndef VGA_PCI_H
> +#define VGA_PCI_H
> +
> +#include "qemu-common.h"
> +#include "qdev.h"
> +
> +/* vga-pci.c */
> +DeviceState *pci_vga_init(PCIBus *bus);
> +
> +/* cirrus_vga.c */
> +DeviceState *pci_cirrus_vga_init(PCIBus *bus);
> +
> +#endif
> --
> 1.7.9.5
>
>



-- 

Best Regards
-Li

Re: [Qemu-devel] [Qemu-ppc][PATCH v5 1/4] Add usb option in machine options

[Alexander Graf]

On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:

> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
> 
> pSeries machine needs to enable USB to add a USB
> keyboard or USB mouse. -usb option won't be used in
> the future, and machine options are a better way to
> enable USB.
> 
> So this patch is to add USB option to machine options
> (-machine type=pseries,usb=on/off) to enable/disable
> USB controller. And machines will get the machine option
> and create a USB controller if USB is on.
> 
> By the way, USB is on by default on pSeries machine.
> So USB controller should be turned off explicitly through
> the command line for "-nodefault" case as the following:
> -machine type=pseries,usb=off.
> 
> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
> Reviewed-by: Andreas Färber <afaerber@suse.de>

Shouldn't you also convert -usb over to the new syntax, so that -usb basically is the same as -machine x,usb=on?


Alex

> ---
> hw/spapr.c    |   11 +++++++++++
> qemu-config.c |    4 ++++
> 2 files changed, 15 insertions(+)
> 
> diff --git a/hw/spapr.c b/hw/spapr.c
> index 81c9343..973de1b 100644
> --- a/hw/spapr.c
> +++ b/hw/spapr.c
> @@ -575,6 +575,8 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>     long load_limit, rtas_limit, fw_size;
>     long pteg_shift = 17;
>     char *filename;
> +    QemuOpts *machine_opts;
> +    bool add_usb = true;
> 
>     spapr = g_malloc0(sizeof(*spapr));
>     QLIST_INIT(&spapr->phbs);
> @@ -710,6 +712,15 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>         spapr_vscsi_create(spapr->vio_bus);
>     }
> 
> +    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
> +    if (machine_opts) {
> +        add_usb = qemu_opt_get_bool(machine_opts, "usb", true);
> +    }
> +
> +    if (add_usb) {
> +        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
> +                          -1, "pci-ohci");
> +    }

Didn't I ask you to extract this out to generic code? I don't want to have a "usb" machine opt that is only available for -M pseries.


Alex

Re: [Qemu-devel] [Qemu-ppc][PATCH v5 4/4] spapr: Add support for -vga option

[Alexander Graf]

On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:

> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
> 
> Also instanciate the USB keyboard and mouse when that option is used
> (you can still use -device to create individual devices without all
> the defaults)
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
> ---
> hw/spapr.c |   29 ++++++++++++++++++++++++++++-
> 1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/spapr.c b/hw/spapr.c
> index 973de1b..3648cad 100644
> --- a/hw/spapr.c
> +++ b/hw/spapr.c
> @@ -45,6 +45,8 @@
> #include "kvm.h"
> #include "kvm_ppc.h"
> #include "pci.h"
> +#include "vga-pci.h"
> +#include "usb.h"
> 
> #include "exec-memory.h"
> 
> @@ -82,6 +84,7 @@
> #define PHANDLE_XICP            0x00001111
> 
> sPAPREnvironment *spapr;
> +bool spapr_has_graphics;

Globals are a big no-no :). Please move this into sPAPREnvironment.

> 
> qemu_irq spapr_allocate_irq(uint32_t hint, uint32_t *irq_num,
>                             enum xics_irq_type type)
> @@ -257,6 +260,9 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
>         _FDT((fdt_property(fdt, "qemu,boot-kernel", &kprop, sizeof(kprop))));
>     }
>     _FDT((fdt_property_string(fdt, "qemu,boot-device", boot_device)));
> +    _FDT((fdt_property_cell(fdt, "qemu,graphic-width", graphic_width)));
> +    _FDT((fdt_property_cell(fdt, "qemu,graphic-height", graphic_height)));
> +    _FDT((fdt_property_cell(fdt, "qemu,graphic-depth", graphic_depth)));

Are you sure we want to set these in -nographic or -vga none mode as well?

> 
>     _FDT((fdt_end_node(fdt)));
> 
> @@ -503,7 +509,9 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
>         }
>     }
> 
> -    spapr_populate_chosen_stdout(fdt, spapr->vio_bus);
> +    if (!spapr_has_graphics) {
> +        spapr_populate_chosen_stdout(fdt, spapr->vio_bus);
> +    }
> 
>     _FDT((fdt_pack(fdt)));
> 
> @@ -556,6 +564,16 @@ static void spapr_cpu_reset(void *opaque)
>     cpu_reset(CPU(cpu));
> }
> 
> +static int spapr_vga_init(PCIBus *pci_bus)
> +{
> +    if (std_vga_enabled) {
> +        pci_vga_init(pci_bus);
> +    } else {
> +        return 0;
> +    }
> +    return 1;

This still silently ignores unsupported -vga options, right? Please error out properly if the user passes in -vga cirrus or xql. We need to let him know that what he selected is not available.

> +}
> +
> /* pSeries LPAR / sPAPR hardware init */
> static void ppc_spapr_init(ram_addr_t ram_size,
>                            const char *boot_device,
> @@ -712,6 +730,11 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>         spapr_vscsi_create(spapr->vio_bus);
>     }
> 
> +    /* Graphics */
> +    if (spapr_vga_init(QLIST_FIRST(&spapr->phbs)->host_state.bus)) {
> +        spapr_has_graphics = true;
> +    }

How about

spapr_has_graphics = spapr_vga_init(...);

If that gets you above 80 characters, just shove the parameter to spapr_vga_init into a variable.


Alex

Re: [Qemu-devel] [Qemu-ppc][PATCH v5 4/4] spapr: Add support for -vga option

[Andreas Färber]

Am 06.07.2012 15:50, schrieb Alexander Graf:
> 
> On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:
> 
>> @@ -712,6 +730,11 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>         spapr_vscsi_create(spapr->vio_bus);
>>     }
>>
>> +    /* Graphics */
>> +    if (spapr_vga_init(QLIST_FIRST(&spapr->phbs)->host_state.bus)) {
>> +        spapr_has_graphics = true;
>> +    }
> 
> How about
> 
> spapr_has_graphics = spapr_vga_init(...);
> 
> If that gets you above 80 characters, just shove the parameter to spapr_vga_init into a variable.

Further, that expression could use
PCIHostState *phb = PCI_HOST_BRIDGE(QLIST_FIRST(&spapr->phbs));
spapr_vga_init(phb->bus)
once introduced through the pci_host series. :)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [Qemu-ppc][PATCH v5 1/4] Add usb option in machine options

[Andreas Färber]

Am 06.07.2012 15:43, schrieb Alexander Graf:
> 
> On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:
> 
>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>
>> pSeries machine needs to enable USB to add a USB
>> keyboard or USB mouse. -usb option won't be used in
>> the future, and machine options are a better way to
>> enable USB.
>>
>> So this patch is to add USB option to machine options
>> (-machine type=pseries,usb=on/off) to enable/disable
>> USB controller. And machines will get the machine option
>> and create a USB controller if USB is on.
>>
>> By the way, USB is on by default on pSeries machine.
>> So USB controller should be turned off explicitly through
>> the command line for "-nodefault" case as the following:
>> -machine type=pseries,usb=off.
>>
>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>> ---
>> hw/spapr.c    |   11 +++++++++++
>> qemu-config.c |    4 ++++
>> 2 files changed, 15 insertions(+)
>>
>> diff --git a/hw/spapr.c b/hw/spapr.c
>> index 81c9343..973de1b 100644
>> --- a/hw/spapr.c
>> +++ b/hw/spapr.c
[...]
>> @@ -710,6 +712,15 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>         spapr_vscsi_create(spapr->vio_bus);
>>     }
>>
>> +    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>> +    if (machine_opts) {
>> +        add_usb = qemu_opt_get_bool(machine_opts, "usb", true);
>> +    }
>> +
>> +    if (add_usb) {
>> +        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>> +                          -1, "pci-ohci");
>> +    }
> 
> Didn't I ask you to extract this out to generic code? I don't want to have a "usb" machine opt that is only available for -M pseries.

Isn't the mapping from "usb=on" to device-level actions
machine-specific? We have ohci, uhci, ehci, xhci to choose from. And the
bus to place it on is machine-specific, too.

So did you rather mean adding usb= awareness to more machines? If we
generalize usb=on to usb=none/ohci/... plus some usbbus= we get -device.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [Qemu-ppc][PATCH v5 1/4] Add usb option in machine options

[Alexander Graf]

On 06.07.2012, at 17:58, Andreas Färber wrote:

> Am 06.07.2012 15:43, schrieb Alexander Graf:
>> 
>> On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:
>> 
>>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>> 
>>> pSeries machine needs to enable USB to add a USB
>>> keyboard or USB mouse. -usb option won't be used in
>>> the future, and machine options are a better way to
>>> enable USB.
>>> 
>>> So this patch is to add USB option to machine options
>>> (-machine type=pseries,usb=on/off) to enable/disable
>>> USB controller. And machines will get the machine option
>>> and create a USB controller if USB is on.
>>> 
>>> By the way, USB is on by default on pSeries machine.
>>> So USB controller should be turned off explicitly through
>>> the command line for "-nodefault" case as the following:
>>> -machine type=pseries,usb=off.
>>> 
>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>> ---
>>> hw/spapr.c    |   11 +++++++++++
>>> qemu-config.c |    4 ++++
>>> 2 files changed, 15 insertions(+)
>>> 
>>> diff --git a/hw/spapr.c b/hw/spapr.c
>>> index 81c9343..973de1b 100644
>>> --- a/hw/spapr.c
>>> +++ b/hw/spapr.c
> [...]
>>> @@ -710,6 +712,15 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>        spapr_vscsi_create(spapr->vio_bus);
>>>    }
>>> 
>>> +    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>> +    if (machine_opts) {
>>> +        add_usb = qemu_opt_get_bool(machine_opts, "usb", true);
>>> +    }
>>> +
>>> +    if (add_usb) {
>>> +        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>> +                          -1, "pci-ohci");
>>> +    }
>> 
>> Didn't I ask you to extract this out to generic code? I don't want to have a "usb" machine opt that is only available for -M pseries.
> 
> Isn't the mapping from "usb=on" to device-level actions
> machine-specific? We have ohci, uhci, ehci, xhci to choose from. And the
> bus to place it on is machine-specific, too.
> 
> So did you rather mean adding usb= awareness to more machines? If we
> generalize usb=on to usb=none/ohci/... plus some usbbus= we get -device.

I was thinking of replacing usb_enabled with

static inline int usb_enabled(bool default) {
    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
    if (machine_opts) {
        return qemu_opt_get_bool(machine_opts, "usb", default);
    }
    return default;
}

Whereas -usb in vl.c would set machine.usb = on. Then all users of usb_enabled would automatically be converted to the new syntax and we could even use the same logic in spapr.c.


Alex

Re: [Qemu-devel] [Qemu-ppc][PATCH v5 1/4] Add usb option in machine options

[Andreas Färber]

Am 06.07.2012 18:03, schrieb Alexander Graf:
> 
> On 06.07.2012, at 17:58, Andreas Färber wrote:
> 
>> Am 06.07.2012 15:43, schrieb Alexander Graf:
>>>
>>> On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:
>>>
>>>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>
>>>> pSeries machine needs to enable USB to add a USB
>>>> keyboard or USB mouse. -usb option won't be used in
>>>> the future, and machine options are a better way to
>>>> enable USB.
>>>>
>>>> So this patch is to add USB option to machine options
>>>> (-machine type=pseries,usb=on/off) to enable/disable
>>>> USB controller. And machines will get the machine option
>>>> and create a USB controller if USB is on.
>>>>
>>>> By the way, USB is on by default on pSeries machine.
>>>> So USB controller should be turned off explicitly through
>>>> the command line for "-nodefault" case as the following:
>>>> -machine type=pseries,usb=off.
>>>>
>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>>> ---
>>>> hw/spapr.c    |   11 +++++++++++
>>>> qemu-config.c |    4 ++++
>>>> 2 files changed, 15 insertions(+)
>>>>
>>>> diff --git a/hw/spapr.c b/hw/spapr.c
>>>> index 81c9343..973de1b 100644
>>>> --- a/hw/spapr.c
>>>> +++ b/hw/spapr.c
>> [...]
>>>> @@ -710,6 +712,15 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>>        spapr_vscsi_create(spapr->vio_bus);
>>>>    }
>>>>
>>>> +    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>>> +    if (machine_opts) {
>>>> +        add_usb = qemu_opt_get_bool(machine_opts, "usb", true);
>>>> +    }
>>>> +
>>>> +    if (add_usb) {
>>>> +        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>>> +                          -1, "pci-ohci");
>>>> +    }
>>>
>>> Didn't I ask you to extract this out to generic code? I don't want to have a "usb" machine opt that is only available for -M pseries.
>>
>> Isn't the mapping from "usb=on" to device-level actions
>> machine-specific? We have ohci, uhci, ehci, xhci to choose from. And the
>> bus to place it on is machine-specific, too.
>>
>> So did you rather mean adding usb= awareness to more machines? If we
>> generalize usb=on to usb=none/ohci/... plus some usbbus= we get -device.
> 
> I was thinking of replacing usb_enabled with
> 
> static inline int usb_enabled(bool default) {
>     machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>     if (machine_opts) {
>         return qemu_opt_get_bool(machine_opts, "usb", default);
>     }
>     return default;
> }
> 
> Whereas -usb in vl.c would set machine.usb = on. Then all users of usb_enabled would automatically be converted to the new syntax and we could even use the same logic in spapr.c.

Oh sure, if we return bool and keep QemuOpts* locally it's perfect. :)

I understood you wanted to generalize the if (add_usb) {} that you
replied under. ;)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [Qemu-ppc][PATCH v5 1/4] Add usb option in machine options

[Li Zhang]

On Fri, Jul 6, 2012 at 9:43 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:
>
>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>
>> pSeries machine needs to enable USB to add a USB
>> keyboard or USB mouse. -usb option won't be used in
>> the future, and machine options are a better way to
>> enable USB.
>>
>> So this patch is to add USB option to machine options
>> (-machine type=pseries,usb=on/off) to enable/disable
>> USB controller. And machines will get the machine option
>> and create a USB controller if USB is on.
>>
>> By the way, USB is on by default on pSeries machine.
>> So USB controller should be turned off explicitly through
>> the command line for "-nodefault" case as the following:
>> -machine type=pseries,usb=off.
>>
>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>
> Shouldn't you also convert -usb over to the new syntax, so that -usb basically is the same as -machine x,usb=on?
>
   Assumely, -usb and -machine x, usb=on shouldn't be used at the same time.
   When using -usb, and -machine x, usb=off, it's a little like to
disable the default USB of machine,
   and use another specific USB with -usb option.

   That's what my understanding. :)

>
> Alex
>
>> ---
>> hw/spapr.c    |   11 +++++++++++
>> qemu-config.c |    4 ++++
>> 2 files changed, 15 insertions(+)
>>
>> diff --git a/hw/spapr.c b/hw/spapr.c
>> index 81c9343..973de1b 100644
>> --- a/hw/spapr.c
>> +++ b/hw/spapr.c
>> @@ -575,6 +575,8 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>     long load_limit, rtas_limit, fw_size;
>>     long pteg_shift = 17;
>>     char *filename;
>> +    QemuOpts *machine_opts;
>> +    bool add_usb = true;
>>
>>     spapr = g_malloc0(sizeof(*spapr));
>>     QLIST_INIT(&spapr->phbs);
>> @@ -710,6 +712,15 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>         spapr_vscsi_create(spapr->vio_bus);
>>     }
>>
>> +    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>> +    if (machine_opts) {
>> +        add_usb = qemu_opt_get_bool(machine_opts, "usb", true);
>> +    }
>> +
>> +    if (add_usb) {
>> +        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>> +                          -1, "pci-ohci");
>> +    }
>
> Didn't I ask you to extract this out to generic code? I don't want to have a "usb" machine opt that is only available for -M pseries.
>
  OK. I will do that. :)
>
> Alex
>



-- 

Best Regards
-Li

Re: [Qemu-devel] [Qemu-ppc][PATCH v5 1/4] Add usb option in machine options

[Alexander Graf]

On 08.07.2012, at 16:21, Li Zhang <zhlcindy@gmail.com> wrote:

> On Fri, Jul 6, 2012 at 9:43 PM, Alexander Graf <agraf@suse.de> wrote:
>> 
>> On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:
>> 
>>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>> 
>>> pSeries machine needs to enable USB to add a USB
>>> keyboard or USB mouse. -usb option won't be used in
>>> the future, and machine options are a better way to
>>> enable USB.
>>> 
>>> So this patch is to add USB option to machine options
>>> (-machine type=pseries,usb=on/off) to enable/disable
>>> USB controller. And machines will get the machine option
>>> and create a USB controller if USB is on.
>>> 
>>> By the way, USB is on by default on pSeries machine.
>>> So USB controller should be turned off explicitly through
>>> the command line for "-nodefault" case as the following:
>>> -machine type=pseries,usb=off.
>>> 
>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>> 
>> Shouldn't you also convert -usb over to the new syntax, so that -usb basically is the same as -machine x,usb=on?
>> 
>   Assumely, -usb and -machine x, usb=on shouldn't be used at the same time.
>   When using -usb, and -machine x, usb=off, it's a little like to
> disable the default USB of machine,
>   and use another specific USB with -usb option.
> 
>   That's what my understanding. :)

It's not about passing them in at the same time, but about having a single semantic instance where "Do we expose USB?" gets stored. That makes the code easier.

Also if you change things the way I suggested, every platform will be able to use -usb and -machine usb=x. So things become a lot more logical from the command line perspective as well, since a management app can always pass in -machine usb=x and have the same semantics, regardless of the machine we're running on.


Alex

> 
>> 
>> Alex
>> 
>>> ---
>>> hw/spapr.c    |   11 +++++++++++
>>> qemu-config.c |    4 ++++
>>> 2 files changed, 15 insertions(+)
>>> 
>>> diff --git a/hw/spapr.c b/hw/spapr.c
>>> index 81c9343..973de1b 100644
>>> --- a/hw/spapr.c
>>> +++ b/hw/spapr.c
>>> @@ -575,6 +575,8 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>    long load_limit, rtas_limit, fw_size;
>>>    long pteg_shift = 17;
>>>    char *filename;
>>> +    QemuOpts *machine_opts;
>>> +    bool add_usb = true;
>>> 
>>>    spapr = g_malloc0(sizeof(*spapr));
>>>    QLIST_INIT(&spapr->phbs);
>>> @@ -710,6 +712,15 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>        spapr_vscsi_create(spapr->vio_bus);
>>>    }
>>> 
>>> +    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>> +    if (machine_opts) {
>>> +        add_usb = qemu_opt_get_bool(machine_opts, "usb", true);
>>> +    }
>>> +
>>> +    if (add_usb) {
>>> +        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>> +                          -1, "pci-ohci");
>>> +    }
>> 
>> Didn't I ask you to extract this out to generic code? I don't want to have a "usb" machine opt that is only available for -M pseries.
>> 
>  OK. I will do that. :)
>> 
>> Alex
>> 
> 
> 
> 
> -- 
> 
> Best Regards
> -Li

Re: [Qemu-devel] [Qemu-ppc][PATCH v5 1/4] Add usb option in machine options

[Li Zhang]

On Sat, Jul 7, 2012 at 12:03 AM, Alexander Graf <agraf@suse.de> wrote:
>
> On 06.07.2012, at 17:58, Andreas Färber wrote:
>
>> Am 06.07.2012 15:43, schrieb Alexander Graf:
>>>
>>> On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:
>>>
>>>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>
>>>> pSeries machine needs to enable USB to add a USB
>>>> keyboard or USB mouse. -usb option won't be used in
>>>> the future, and machine options are a better way to
>>>> enable USB.
>>>>
>>>> So this patch is to add USB option to machine options
>>>> (-machine type=pseries,usb=on/off) to enable/disable
>>>> USB controller. And machines will get the machine option
>>>> and create a USB controller if USB is on.
>>>>
>>>> By the way, USB is on by default on pSeries machine.
>>>> So USB controller should be turned off explicitly through
>>>> the command line for "-nodefault" case as the following:
>>>> -machine type=pseries,usb=off.
>>>>
>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>>> ---
>>>> hw/spapr.c    |   11 +++++++++++
>>>> qemu-config.c |    4 ++++
>>>> 2 files changed, 15 insertions(+)
>>>>
>>>> diff --git a/hw/spapr.c b/hw/spapr.c
>>>> index 81c9343..973de1b 100644
>>>> --- a/hw/spapr.c
>>>> +++ b/hw/spapr.c
>> [...]
>>>> @@ -710,6 +712,15 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>>        spapr_vscsi_create(spapr->vio_bus);
>>>>    }
>>>>
>>>> +    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>>> +    if (machine_opts) {
>>>> +        add_usb = qemu_opt_get_bool(machine_opts, "usb", true);
>>>> +    }
>>>> +
>>>> +    if (add_usb) {
>>>> +        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>>> +                          -1, "pci-ohci");
>>>> +    }
>>>
>>> Didn't I ask you to extract this out to generic code? I don't want to have a "usb" machine opt that is only available for -M pseries.
>>
>> Isn't the mapping from "usb=on" to device-level actions
>> machine-specific? We have ohci, uhci, ehci, xhci to choose from. And the
>> bus to place it on is machine-specific, too.
>>
>> So did you rather mean adding usb= awareness to more machines? If we
>> generalize usb=on to usb=none/ohci/... plus some usbbus= we get -device.
>
> I was thinking of replacing usb_enabled with
>
> static inline int usb_enabled(bool default) {
>     machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>     if (machine_opts) {
>         return qemu_opt_get_bool(machine_opts, "usb", default);
>     }
>     return default;
> }
>
> Whereas -usb in vl.c would set machine.usb = on. Then all users of usb_enabled would automatically be converted to the new syntax and we could even use the same logic in spapr.c.
>

I see, you mean global varible usb_enabled is still used, and when
-usb is passed to qemu command line,
use_enabled will be assigned by the above function and set machine.usb=on.

So all the places where usb_enabled is used still can work correctly.

I think that's great, I will do that.

Thanks for your suggestions.

>
> Alex
>



-- 

Best Regards
-Li

Re: [Qemu-devel] [Qemu-ppc][PATCH v5 1/4] Add usb option in machine options

[Li Zhang]

On Sun, Jul 8, 2012 at 10:45 PM, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 08.07.2012, at 16:21, Li Zhang <zhlcindy@gmail.com> wrote:
>
>> On Fri, Jul 6, 2012 at 9:43 PM, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:
>>>
>>>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>
>>>> pSeries machine needs to enable USB to add a USB
>>>> keyboard or USB mouse. -usb option won't be used in
>>>> the future, and machine options are a better way to
>>>> enable USB.
>>>>
>>>> So this patch is to add USB option to machine options
>>>> (-machine type=pseries,usb=on/off) to enable/disable
>>>> USB controller. And machines will get the machine option
>>>> and create a USB controller if USB is on.
>>>>
>>>> By the way, USB is on by default on pSeries machine.
>>>> So USB controller should be turned off explicitly through
>>>> the command line for "-nodefault" case as the following:
>>>> -machine type=pseries,usb=off.
>>>>
>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>>
>>> Shouldn't you also convert -usb over to the new syntax, so that -usb basically is the same as -machine x,usb=on?
>>>
>>   Assumely, -usb and -machine x, usb=on shouldn't be used at the same time.
>>   When using -usb, and -machine x, usb=off, it's a little like to
>> disable the default USB of machine,
>>   and use another specific USB with -usb option.
>>
>>   That's what my understanding. :)
>
> It's not about passing them in at the same time, but about having a single semantic instance where "Do we expose USB?" gets stored. That makes the code easier.
>
> Also if you change things the way I suggested, every platform will be able to use -usb and -machine usb=x. So things become a lot more logical from the command line perspective as well, since a management app can always pass in -machine usb=x and have the same semantics, regardless of the machine we're running on.
>
Got it. I  will change it.
>
> Alex
>
>>
>>>
>>> Alex
>>>
>>>> ---
>>>> hw/spapr.c    |   11 +++++++++++
>>>> qemu-config.c |    4 ++++
>>>> 2 files changed, 15 insertions(+)
>>>>
>>>> diff --git a/hw/spapr.c b/hw/spapr.c
>>>> index 81c9343..973de1b 100644
>>>> --- a/hw/spapr.c
>>>> +++ b/hw/spapr.c
>>>> @@ -575,6 +575,8 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>>    long load_limit, rtas_limit, fw_size;
>>>>    long pteg_shift = 17;
>>>>    char *filename;
>>>> +    QemuOpts *machine_opts;
>>>> +    bool add_usb = true;
>>>>
>>>>    spapr = g_malloc0(sizeof(*spapr));
>>>>    QLIST_INIT(&spapr->phbs);
>>>> @@ -710,6 +712,15 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>>        spapr_vscsi_create(spapr->vio_bus);
>>>>    }
>>>>
>>>> +    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>>> +    if (machine_opts) {
>>>> +        add_usb = qemu_opt_get_bool(machine_opts, "usb", true);
>>>> +    }
>>>> +
>>>> +    if (add_usb) {
>>>> +        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>>> +                          -1, "pci-ohci");
>>>> +    }
>>>
>>> Didn't I ask you to extract this out to generic code? I don't want to have a "usb" machine opt that is only available for -M pseries.
>>>
>>  OK. I will do that. :)
>>>
>>> Alex
>>>
>>
>>
>>
>> --
>>
>> Best Regards
>> -Li



-- 

Best Regards
-Li

Re: [Qemu-devel] [Qemu-ppc][PATCH v5 1/4] Add usb option in machine options

[Alexander Graf]

On 08.07.2012, at 16:46, Li Zhang <zhlcindy@gmail.com> wrote:

> On Sat, Jul 7, 2012 at 12:03 AM, Alexander Graf <agraf@suse.de> wrote:
>> 
>> On 06.07.2012, at 17:58, Andreas Färber wrote:
>> 
>>> Am 06.07.2012 15:43, schrieb Alexander Graf:
>>>> 
>>>> On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:
>>>> 
>>>>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>> 
>>>>> pSeries machine needs to enable USB to add a USB
>>>>> keyboard or USB mouse. -usb option won't be used in
>>>>> the future, and machine options are a better way to
>>>>> enable USB.
>>>>> 
>>>>> So this patch is to add USB option to machine options
>>>>> (-machine type=pseries,usb=on/off) to enable/disable
>>>>> USB controller. And machines will get the machine option
>>>>> and create a USB controller if USB is on.
>>>>> 
>>>>> By the way, USB is on by default on pSeries machine.
>>>>> So USB controller should be turned off explicitly through
>>>>> the command line for "-nodefault" case as the following:
>>>>> -machine type=pseries,usb=off.
>>>>> 
>>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>>>> ---
>>>>> hw/spapr.c    |   11 +++++++++++
>>>>> qemu-config.c |    4 ++++
>>>>> 2 files changed, 15 insertions(+)
>>>>> 
>>>>> diff --git a/hw/spapr.c b/hw/spapr.c
>>>>> index 81c9343..973de1b 100644
>>>>> --- a/hw/spapr.c
>>>>> +++ b/hw/spapr.c
>>> [...]
>>>>> @@ -710,6 +712,15 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>>>       spapr_vscsi_create(spapr->vio_bus);
>>>>>   }
>>>>> 
>>>>> +    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>>>> +    if (machine_opts) {
>>>>> +        add_usb = qemu_opt_get_bool(machine_opts, "usb", true);
>>>>> +    }
>>>>> +
>>>>> +    if (add_usb) {
>>>>> +        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>>>> +                          -1, "pci-ohci");
>>>>> +    }
>>>> 
>>>> Didn't I ask you to extract this out to generic code? I don't want to have a "usb" machine opt that is only available for -M pseries.
>>> 
>>> Isn't the mapping from "usb=on" to device-level actions
>>> machine-specific? We have ohci, uhci, ehci, xhci to choose from. And the
>>> bus to place it on is machine-specific, too.
>>> 
>>> So did you rather mean adding usb= awareness to more machines? If we
>>> generalize usb=on to usb=none/ohci/... plus some usbbus= we get -device.
>> 
>> I was thinking of replacing usb_enabled with
>> 
>> static inline int usb_enabled(bool default) {
>>    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>    if (machine_opts) {
>>        return qemu_opt_get_bool(machine_opts, "usb", default);
>>    }
>>    return default;
>> }
>> 
>> Whereas -usb in vl.c would set machine.usb = on. Then all users of usb_enabled would automatically be converted to the new syntax and we could even use the same logic in spapr.c.
>> 
> 
> I see, you mean global varible usb_enabled is still used, and when
> -usb is passed to qemu command line,
> use_enabled will be assigned by the above function and set machine.usb=on.
> 
> So all the places where usb_enabled is used still can work correctly.

I mean convert usb_enabled into a function that checks machine.usb :). The only global state should live in machine opts.


Alex

Re: [Qemu-devel] [Qemu-ppc][PATCH v5 4/4] spapr: Add support for -vga option

[Li Zhang]

On Fri, Jul 6, 2012 at 9:50 PM, Alexander Graf <agraf@suse.de> wrote:
>
> On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:
>
>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>
>> Also instanciate the USB keyboard and mouse when that option is used
>> (you can still use -device to create individual devices without all
>> the defaults)
>>
>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>> ---
>> hw/spapr.c |   29 ++++++++++++++++++++++++++++-
>> 1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/spapr.c b/hw/spapr.c
>> index 973de1b..3648cad 100644
>> --- a/hw/spapr.c
>> +++ b/hw/spapr.c
>> @@ -45,6 +45,8 @@
>> #include "kvm.h"
>> #include "kvm_ppc.h"
>> #include "pci.h"
>> +#include "vga-pci.h"
>> +#include "usb.h"
>>
>> #include "exec-memory.h"
>>
>> @@ -82,6 +84,7 @@
>> #define PHANDLE_XICP            0x00001111
>>
>> sPAPREnvironment *spapr;
>> +bool spapr_has_graphics;
>
> Globals are a big no-no :). Please move this into sPAPREnvironment.
>
>>
>> qemu_irq spapr_allocate_irq(uint32_t hint, uint32_t *irq_num,
>>                             enum xics_irq_type type)
>> @@ -257,6 +260,9 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
>>         _FDT((fdt_property(fdt, "qemu,boot-kernel", &kprop, sizeof(kprop))));
>>     }
>>     _FDT((fdt_property_string(fdt, "qemu,boot-device", boot_device)));
>> +    _FDT((fdt_property_cell(fdt, "qemu,graphic-width", graphic_width)));
>> +    _FDT((fdt_property_cell(fdt, "qemu,graphic-height", graphic_height)));
>> +    _FDT((fdt_property_cell(fdt, "qemu,graphic-depth", graphic_depth)));
>
> Are you sure we want to set these in -nographic or -vga none mode as well?

I am not sure about this.

Ben, would you please give more information about this?

Thanks.

>
>>
>>     _FDT((fdt_end_node(fdt)));
>>
>> @@ -503,7 +509,9 @@ static void spapr_finalize_fdt(sPAPREnvironment *spapr,
>>         }
>>     }
>>
>> -    spapr_populate_chosen_stdout(fdt, spapr->vio_bus);
>> +    if (!spapr_has_graphics) {
>> +        spapr_populate_chosen_stdout(fdt, spapr->vio_bus);
>> +    }
>>
>>     _FDT((fdt_pack(fdt)));
>>
>> @@ -556,6 +564,16 @@ static void spapr_cpu_reset(void *opaque)
>>     cpu_reset(CPU(cpu));
>> }
>>
>> +static int spapr_vga_init(PCIBus *pci_bus)
>> +{
>> +    if (std_vga_enabled) {
>> +        pci_vga_init(pci_bus);
>> +    } else {
>> +        return 0;
>> +    }
>> +    return 1;
>
> This still silently ignores unsupported -vga options, right? Please error out properly if the user passes in -vga cirrus or xql. We need to let him know that what he selected is not available.
Yes, right. It seems that it's not good.
OK, I will do that.
>
>> +}
>> +
>> /* pSeries LPAR / sPAPR hardware init */
>> static void ppc_spapr_init(ram_addr_t ram_size,
>>                            const char *boot_device,
>> @@ -712,6 +730,11 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>         spapr_vscsi_create(spapr->vio_bus);
>>     }
>>
>> +    /* Graphics */
>> +    if (spapr_vga_init(QLIST_FIRST(&spapr->phbs)->host_state.bus)) {
>> +        spapr_has_graphics = true;
>> +    }
>
> How about
>
> spapr_has_graphics = spapr_vga_init(...);
>
> If that gets you above 80 characters, just shove the parameter to spapr_vga_init into a variable.
OK. I see.
Thanks.
>
>
> Alex
>



-- 

Best Regards
-Li

Re: [Qemu-devel] [Qemu-ppc][PATCH v5 1/4] Add usb option in machine options

[Li Zhang]

On Sun, Jul 8, 2012 at 11:02 PM, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 08.07.2012, at 16:46, Li Zhang <zhlcindy@gmail.com> wrote:
>
>> On Sat, Jul 7, 2012 at 12:03 AM, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> On 06.07.2012, at 17:58, Andreas Färber wrote:
>>>
>>>> Am 06.07.2012 15:43, schrieb Alexander Graf:
>>>>>
>>>>> On 02.07.2012, at 07:25, zhlcindy@gmail.com wrote:
>>>>>
>>>>>> From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>>>
>>>>>> pSeries machine needs to enable USB to add a USB
>>>>>> keyboard or USB mouse. -usb option won't be used in
>>>>>> the future, and machine options are a better way to
>>>>>> enable USB.
>>>>>>
>>>>>> So this patch is to add USB option to machine options
>>>>>> (-machine type=pseries,usb=on/off) to enable/disable
>>>>>> USB controller. And machines will get the machine option
>>>>>> and create a USB controller if USB is on.
>>>>>>
>>>>>> By the way, USB is on by default on pSeries machine.
>>>>>> So USB controller should be turned off explicitly through
>>>>>> the command line for "-nodefault" case as the following:
>>>>>> -machine type=pseries,usb=off.
>>>>>>
>>>>>> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com>
>>>>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>>>>> ---
>>>>>> hw/spapr.c    |   11 +++++++++++
>>>>>> qemu-config.c |    4 ++++
>>>>>> 2 files changed, 15 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/spapr.c b/hw/spapr.c
>>>>>> index 81c9343..973de1b 100644
>>>>>> --- a/hw/spapr.c
>>>>>> +++ b/hw/spapr.c
>>>> [...]
>>>>>> @@ -710,6 +712,15 @@ static void ppc_spapr_init(ram_addr_t ram_size,
>>>>>>       spapr_vscsi_create(spapr->vio_bus);
>>>>>>   }
>>>>>>
>>>>>> +    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>>>>> +    if (machine_opts) {
>>>>>> +        add_usb = qemu_opt_get_bool(machine_opts, "usb", true);
>>>>>> +    }
>>>>>> +
>>>>>> +    if (add_usb) {
>>>>>> +        pci_create_simple(QLIST_FIRST(&spapr->phbs)->host_state.bus,
>>>>>> +                          -1, "pci-ohci");
>>>>>> +    }
>>>>>
>>>>> Didn't I ask you to extract this out to generic code? I don't want to have a "usb" machine opt that is only available for -M pseries.
>>>>
>>>> Isn't the mapping from "usb=on" to device-level actions
>>>> machine-specific? We have ohci, uhci, ehci, xhci to choose from. And the
>>>> bus to place it on is machine-specific, too.
>>>>
>>>> So did you rather mean adding usb= awareness to more machines? If we
>>>> generalize usb=on to usb=none/ohci/... plus some usbbus= we get -device.
>>>
>>> I was thinking of replacing usb_enabled with
>>>
>>> static inline int usb_enabled(bool default) {
>>>    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>>    if (machine_opts) {
>>>        return qemu_opt_get_bool(machine_opts, "usb", default);
>>>    }
>>>    return default;
>>> }
>>>
>>> Whereas -usb in vl.c would set machine.usb = on. Then all users of usb_enabled would automatically be converted to the new syntax and we could even use the same logic in spapr.c.
>>>
>>
>> I see, you mean global varible usb_enabled is still used, and when
>> -usb is passed to qemu command line,
>> use_enabled will be assigned by the above function and set machine.usb=on.
>>
>> So all the places where usb_enabled is used still can work correctly.
>
> I mean convert usb_enabled into a function that checks machine.usb :). The only global state should live in machine opts.
>
Oh, got it.
So usb_enabled should be cleared up in the places where it is used. :)

>
> Alex
>



-- 

Best Regards
-Li

Re: [Qemu-devel] [Qemu-ppc][PATCH v5 4/4] spapr: Add support for -vga option

[Benjamin Herrenschmidt]

On Sun, 2012-07-08 at 23:08 +0800, Li Zhang wrote:
> > Are you sure we want to set these in -nographic or -vga none mode as
> well?
> 
> I am not sure about this.
> 
> Ben, would you please give more information about this?

Doesn't matter much. They are only useful when there is a vga adapter,
they don't hurt if there isn't and it shouldn't depend on the presence
of -vga since once can add an adapter with -device, so it becomes really
tricky to figure out whether to expose them or not, so I made it
unconditional.

Cheers,
Ben.