[Qemu-devel] Assorted trivial bugfixes

[Qemu-devel] Assorted trivial bugfixes

[David Gibson]

This series contains 5 essentially unrelated trivial bugfixes for qemu
code.  Most are aimed at eventually getting virtio devices working on
the pseries target, the others are just annoying build bugs.

[Qemu-devel] [PATCH 1/5] ppce500_mpc8544ds: Fix compile with --enable-debug and --disable-kvm

[David Gibson]

From: Alexey Kardashevskiy <aik@ozlabs.ru>

When configured with --enable-debug, the makefile does not use any
optimization and compilation of hw/ppce500_mpc8544ds.c fails because gcc
does not remove the never called without kvm function
mpc8544_copy_soc_cell(), which will fail to link without the kvm code.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppce500_mpc8544ds.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ppce500_mpc8544ds.c b/hw/ppce500_mpc8544ds.c
index b7670ae..33ded44 100644
--- a/hw/ppce500_mpc8544ds.c
+++ b/hw/ppce500_mpc8544ds.c
@@ -50,7 +50,7 @@
 #define MPC8544_PCI_IO             0xE1000000
 #define MPC8544_PCI_IOLEN          0x10000
 
-#ifdef CONFIG_FDT
+#if defined(CONFIG_FDT) && defined(CONFIG_KVM)
 static int mpc8544_copy_soc_cell(void *fdt, const char *node, const char *prop)
 {
     uint32_t cell;
@@ -81,7 +81,7 @@ static int mpc8544_load_device_tree(target_phys_addr_t addr,
                                      const char *kernel_cmdline)
 {
     int ret = -1;
-#ifdef CONFIG_FDT
+#if defined(CONFIG_FDT) && defined(CONFIG_KVM)
     uint32_t mem_reg_property[] = {0, ramsize};
     char *filename;
     int fdt_size;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/5] ppce500_mpc8544ds: Fix compile with --enable-debug and --disable-kvm

[Alexander Graf]

On 07.04.2011, at 05:02, David Gibson wrote:

> From: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> When configured with --enable-debug, the makefile does not use any
> optimization and compilation of hw/ppce500_mpc8544ds.c fails because gcc
> does not remove the never called without kvm function
> mpc8544_copy_soc_cell(), which will fail to link without the kvm code.

I'm not sure I can follow. So the problem is that we call kvmppc_read_host_property and don't provide a stub wrapper for it? Let's provide a wrapper then instead of crippling the target code :)


Alex

Re: [Qemu-devel] [PATCH 1/5] ppce500_mpc8544ds: Fix compile with --enable-debug and --disable-kvm

[Alexey Kardashevskiy]

On 07.04.2011 18:42, Alexander Graf wrote:
> On 07.04.2011, at 05:02, David Gibson wrote:
>
>> From: Alexey Kardashevskiy<aik@ozlabs.ru>
>>
>> When configured with --enable-debug, the makefile does not use any
>> optimization and compilation of hw/ppce500_mpc8544ds.c fails because gcc
>> does not remove the never called without kvm function
>> mpc8544_copy_soc_cell(), which will fail to link without the kvm code.
> I'm not sure I can follow. So the problem is that we call kvmppc_read_host_property and don't provide a stub wrapper for it? Let's provide a wrapper then instead of crippling the target code :)

The problems exists only with --disable-kvm and --enable-debug. Without 
--enable-debug, qemu is compiled with -O2, gcc detects that 
kvm_enabled() always returns false and optimizes out 
mpc8544_copy_soc_cell calls which will not be called anyway. With 
--enable-debug, the linker cannot find symbols which it won't call anyway.


alexey.

Re: [Qemu-devel] [PATCH 1/5] ppce500_mpc8544ds: Fix compile with --enable-debug and --disable-kvm

[Alexander Graf]

On 07.04.2011, at 10:52, Alexey Kardashevskiy wrote:

> On 07.04.2011 18:42, Alexander Graf wrote:
>> On 07.04.2011, at 05:02, David Gibson wrote:
>> 
>>> From: Alexey Kardashevskiy<aik@ozlabs.ru>
>>> 
>>> When configured with --enable-debug, the makefile does not use any
>>> optimization and compilation of hw/ppce500_mpc8544ds.c fails because gcc
>>> does not remove the never called without kvm function
>>> mpc8544_copy_soc_cell(), which will fail to link without the kvm code.
>> I'm not sure I can follow. So the problem is that we call kvmppc_read_host_property and don't provide a stub wrapper for it? Let's provide a wrapper then instead of crippling the target code :)
> 
> The problems exists only with --disable-kvm and --enable-debug. Without --enable-debug, qemu is compiled with -O2, gcc detects that kvm_enabled() always returns false and optimizes out mpc8544_copy_soc_cell calls which will not be called anyway. With --enable-debug, the linker cannot find symbols which it won't call anyway.

Ah, there is the kvm_enabled path. I would still find it a superior approach to provide a stub function for the missing call. Potentially, the target should be built without knowledge of config options.


Alex

[Qemu-devel] [PATCH 2/5] spapr_llan: Fix warning when compiled with -dDEBUG

[David Gibson]

From: Alexey Kardashevskiy <aik@ozlabs.ru>

Compiling with the DEBUG macro causes leaves hw/spapr_llan.c with an
unused variable, which is treated as an error in the qemu build.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/spapr_llan.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/hw/spapr_llan.c b/hw/spapr_llan.c
index 1d83fd5..ff3a78f 100644
--- a/hw/spapr_llan.c
+++ b/hw/spapr_llan.c
@@ -274,9 +274,6 @@ static target_ulong h_register_logical_lan(CPUState *env,
     VIOsPAPRDevice *sdev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
     VIOsPAPRVLANDevice *dev = (VIOsPAPRVLANDevice *)sdev;
     vlan_bd_t filter_list_bd;
-#ifdef DEBUG
-    target_ulong mac_address = args[4];
-#endif
 
     if (!dev) {
         return H_PARAMETER;
-- 
1.7.1

[Qemu-devel] [PATCH 3/5] virtio-9p: fixed LE-to-host conversion bug when QEMU is called from guest

[David Gibson]

From: Alexey Kardashevskiy <aik@ozlabs.ru>

The 9p code already contains an attempt at the necessary endian
conversions, but it's broken.

The code which does conversion from host to guest does it correctly
and this code was copied to the function which does guest to host
conversion.  However the copied code hasn't been correctly updated, so
it first endian converts some garbage on the stack and then overwrites
it with a field from incoming packet without conversion.

The patch fixes the mistakes.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/virtio-9p.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
index 7c59988..7e29535 100644
--- a/hw/virtio-9p.c
+++ b/hw/virtio-9p.c
@@ -696,25 +696,22 @@ static size_t pdu_unmarshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
         case 'w': {
             uint16_t val, *valp;
             valp = va_arg(ap, uint16_t *);
-            val = le16_to_cpupu(valp);
             offset += pdu_unpack(&val, pdu, offset, sizeof(val));
-            *valp = val;
+            *valp = le16_to_cpu(val);
             break;
         }
         case 'd': {
             uint32_t val, *valp;
             valp = va_arg(ap, uint32_t *);
-            val = le32_to_cpupu(valp);
             offset += pdu_unpack(&val, pdu, offset, sizeof(val));
-            *valp = val;
+            *valp = le32_to_cpu(val);
             break;
         }
         case 'q': {
             uint64_t val, *valp;
             valp = va_arg(ap, uint64_t *);
-            val = le64_to_cpup(valp);
             offset += pdu_unpack(&val, pdu, offset, sizeof(val));
-            *valp = val;
+            *valp = le64_to_cpu(val);
             break;
         }
         case 'v': {
-- 
1.7.1

[Qemu-devel] [PATCH 4/5] virtio-balloon: fixed endianness bug in the config space

[David Gibson]

From: Alexey Kardashevskiy <aik@ozlabs.ru>

The specification for the virtio balloon device requres that the values
in the config space be encoded little-endian.  This differs from most
virtio things, where guest-native endian is the norm.

Currently, the qemu virtio-balloon code correctly makes the conversion
on get_config(), but doesn't on set_config for the 'actual' field.  The
kernel driver, on the other hand, correctly converts when setting the
actual field, but does not convert when reading the config space.  The
upshot is that virtio-balloon will only work correctly if both host and
guest are LE, making all the conversions nops.

This patch corrects the qemu side, correctly doing host-native <-> LE
conversions when accessing the config space.  This won't break any setups
that aren't already broken, and fixes the case of BE host, LE guest.
Fixing the BE guest case will require kernel fixes as well.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/virtio-balloon.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 8adddea..257baf8 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -191,7 +191,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
     VirtIOBalloon *dev = to_virtio_balloon(vdev);
     struct virtio_balloon_config config;
     memcpy(&config, config_data, 8);
-    dev->actual = config.actual;
+    dev->actual = le32_to_cpu(config.actual);
 }
 
 static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
-- 
1.7.1

[Qemu-devel] [PATCH 5/5] virtio-serial: Fix endianness bug in the config space

[David Gibson]

From: Alexey Kardashevskiy <aik@ozlabs.ru>

The virtio serial specification requres that the values in the config
space are encoded in native endian of the guest.

The qemu virtio-serial code did not do conversion to the guest endian
format what caused problems when host and guest use different format.

This patch corrects the qemu side, correctly doing host-native <->
guest-native conversions when accessing the config space. This won't
break any setups that aren't already broken, and fixes the case
of different host and guest endianness.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/virtio-serial-bus.c |   23 +++++++++++++----------
 1 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 6227379..f10d48f 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -494,7 +494,7 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
     VirtIOSerial *s = opaque;
     VirtIOSerialPort *port;
     uint32_t nr_active_ports;
-    unsigned int i;
+    unsigned int i, max_nr_ports;
 
     /* The virtio device */
     virtio_save(&s->vdev, f);
@@ -506,8 +506,8 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
     qemu_put_be32s(f, &s->config.max_nr_ports);
 
     /* The ports map */
-
-    for (i = 0; i < (s->config.max_nr_ports + 31) / 32; i++) {
+    max_nr_ports = tswap32(s->config.max_nr_ports);
+    for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
         qemu_put_be32s(f, &s->ports_map[i]);
     }
 
@@ -568,7 +568,8 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
     qemu_get_be16s(f, &s->config.rows);
 
     qemu_get_be32s(f, &max_nr_ports);
-    if (max_nr_ports > s->config.max_nr_ports) {
+    tswap32s(&max_nr_ports);
+    if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
         /* Source could have had more ports than us. Fail migration. */
         return -EINVAL;
     }
@@ -670,9 +671,10 @@ static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
 /* This function is only used if a port id is not provided by the user */
 static uint32_t find_free_port_id(VirtIOSerial *vser)
 {
-    unsigned int i;
+    unsigned int i, max_nr_ports;
 
-    for (i = 0; i < (vser->config.max_nr_ports + 31) / 32; i++) {
+    max_nr_ports = tswap32(vser->config.max_nr_ports);
+    for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
         uint32_t map, bit;
 
         map = vser->ports_map[i];
@@ -720,7 +722,7 @@ static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
     VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, qdev);
     VirtIOSerialPortInfo *info = DO_UPCAST(VirtIOSerialPortInfo, qdev, base);
     VirtIOSerialBus *bus = DO_UPCAST(VirtIOSerialBus, qbus, qdev->parent_bus);
-    int ret;
+    int ret, max_nr_ports;
     bool plugging_port0;
 
     port->vser = bus->vser;
@@ -750,9 +752,10 @@ static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
         }
     }
 
-    if (port->id >= port->vser->config.max_nr_ports) {
+    max_nr_ports = tswap32(port->vser->config.max_nr_ports);
+    if (port->id >= max_nr_ports) {
         error_report("virtio-serial-bus: Out-of-range port id specified, max. allowed: %u\n",
-                     port->vser->config.max_nr_ports - 1);
+                     max_nr_ports - 1);
         return -1;
     }
 
@@ -863,7 +866,7 @@ VirtIODevice *virtio_serial_init(DeviceState *dev, virtio_serial_conf *conf)
         vser->ovqs[i] = virtio_add_queue(vdev, 128, handle_output);
     }
 
-    vser->config.max_nr_ports = conf->max_virtserial_ports;
+    vser->config.max_nr_ports = tswap32(conf->max_virtserial_ports);
     vser->ports_map = qemu_mallocz(((conf->max_virtserial_ports + 31) / 32)
         * sizeof(vser->ports_map[0]));
     /*
-- 
1.7.1

Re: [Qemu-devel] Assorted trivial bugfixes

[Stefan Hajnoczi]

On Thu, Apr 07, 2011 at 01:02:00PM +1000, David Gibson wrote:
> This series contains 5 essentially unrelated trivial bugfixes for qemu
> code.  Most are aimed at eventually getting virtio devices working on
> the pseries target, the others are just annoying build bugs.

Thanks, patches 2-4 are in the trivial patches tree:
http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches

Patch 1 is being discussed and patch 5 needs review from Amit, it's too large
to throw in here.

I will send a pull request today that includes patches 2-4.  For more
information on the trivial patches tree:
http://qemu.org/Contribute/TrivialPatches

Stefan

Re: [Qemu-devel] Assorted trivial bugfixes

[David Gibson]

On Thu, Apr 07, 2011 at 09:57:41AM +0100, Stefan Hajnoczi wrote:
> On Thu, Apr 07, 2011 at 01:02:00PM +1000, David Gibson wrote:
> > This series contains 5 essentially unrelated trivial bugfixes for qemu
> > code.  Most are aimed at eventually getting virtio devices working on
> > the pseries target, the others are just annoying build bugs.
> 
> Thanks, patches 2-4 are in the trivial patches tree:
> http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches

Thanks.

> Patch 1 is being discussed and patch 5 needs review from Amit, it's too large
> to throw in here.

Sorry, I'm new here - what is Amit's address to contact him directly
about this?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] Assorted trivial bugfixes

[Stefan Hajnoczi]

On Fri, Apr 8, 2011 at 4:46 AM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> On Thu, Apr 07, 2011 at 09:57:41AM +0100, Stefan Hajnoczi wrote:
>> On Thu, Apr 07, 2011 at 01:02:00PM +1000, David Gibson wrote:
>> > This series contains 5 essentially unrelated trivial bugfixes for qemu
>> > code.  Most are aimed at eventually getting virtio devices working on
>> > the pseries target, the others are just annoying build bugs.
>>
>> Thanks, patches 2-4 are in the trivial patches tree:
>> http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches
>
> Thanks.
>
>> Patch 1 is being discussed and patch 5 needs review from Amit, it's too large
>> to throw in here.
>
> Sorry, I'm new here - what is Amit's address to contact him directly
> about this?
On Fri, Apr 08, 2011 at 01:46:13PM +1000, David Gibson wrote:
> On Thu, Apr 07, 2011 at 09:57:41AM +0100, Stefan Hajnoczi wrote:
> > On Thu, Apr 07, 2011 at 01:02:00PM +1000, David Gibson wrote:
> > > This series contains 5 essentially unrelated trivial bugfixes for qemu
> > > code.  Most are aimed at eventually getting virtio devices working on
> > > the pseries target, the others are just annoying build bugs.
> >
> > Thanks, patches 2-4 are in the trivial patches tree:
> > http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches
>
> Thanks.
>
> > Patch 1 is being discussed and patch 5 needs review from Amit, it's too large
> > to throw in here.
>
> Sorry, I'm new here - what is Amit's address to contact him directly
> about this?

$ git log hw/virtio-serial-bus.c
[...]
Author: Amit Shah <amit.shah@redhat.com>

Or:
$ cat MAINTAINERS
[...]
virtio-serial
M: Amit Shah <amit.shah@redhat.com>

Stefan