[Qemu-devel] [PULL for-1.7 v2 0/5] pc very last minute fixes for 1.7

[Qemu-devel] [PULL for-1.7 v2 0/5] pc very last minute fixes for 1.7

[Michael S. Tsirkin]

Changes from v1:
    added --iasl configure fix.
    Note: I didn't rebase so if you pulled already, just redoing
    the pull will do the right thing adding the single new patch.

The following changes since commit 394cfa39ba24dd838ace1308ae24961243947fb8:

  Merge remote-tracking branch 'quintela/migration.next' into staging (2013-11-19 13:03:06 -0800)

are available in the git repository at:


  git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_anthony

for you to fetch changes up to e007dbece5fc4e55e10116c6cb42753e35a945bf:

  configure: make --iasl option actually work (2013-11-24 15:43:06 +0200)

----------------------------------------------------------------
pc very last minute fixes for 1.7

This has a fix for a crasher bug with pci bridges,
boot failure fix for s390 on 32 bit hosts,
and fixes build for hosts with old glib.

There's also a fix for --iasl configure flag - it can be used
to work around broken iasl on some systems either
by using a non-standard iasl or by disabling it.

I've also reverted a e1000/rtl mac programming change
that seems slightly wrong and too risky for 1.8.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

----------------------------------------------------------------
Bandan Das (1):
      pci: unregister vmstate_pcibus on unplug

Michael S. Tsirkin (5):
      s390x: fix flat file load on 32 bit systems
      acpi-build: fix build on glib < 2.22
      acpi-build: fix build on glib < 2.14
      Revert "e1000/rtl8139: update HMP NIC when every bit is written"
      configure: make --iasl option actually work

 configure                    |  4 ++--
 hw/i386/acpi-build.c         | 16 ++++++++++------
 hw/i386/bios-linker-loader.c |  8 ++++----
 hw/net/e1000.c               |  2 +-
 hw/net/rtl8139.c             |  5 ++++-
 hw/pci/pci.c                 |  8 ++++++++
 hw/s390x/ipl.c               | 17 +++++++++--------
 7 files changed, 38 insertions(+), 22 deletions(-)

Bandan Das (1):
  pci: unregister vmstate_pcibus on unplug

Michael S. Tsirkin (5):
  s390x: fix flat file load on 32 bit systems
  acpi-build: fix build on glib < 2.22
  acpi-build: fix build on glib < 2.14
  Revert "e1000/rtl8139: update HMP NIC when every bit is written"
  configure: make --iasl option actually work

 configure                    |  4 ++--
 hw/i386/acpi-build.c         | 16 ++++++++++------
 hw/i386/bios-linker-loader.c |  8 ++++----
 hw/net/e1000.c               |  2 +-
 hw/net/rtl8139.c             |  5 ++++-
 hw/pci/pci.c                 |  8 ++++++++
 hw/s390x/ipl.c               | 17 +++++++++--------
 7 files changed, 38 insertions(+), 22 deletions(-)

-- 
MST

[Qemu-devel] [PULL for-1.7 v2 1/6] s390x: fix flat file load on 32 bit systems

[Michael S. Tsirkin]

pc-bios/s390-zipl.rom is a flat image so it's expected that
loading it as elf will fail.
It should fall back on loading a flat file, but doesn't
on 32 bit systems, instead it fails printing:
    qemu: hardware error: could not load bootloader 's390-zipl.rom'

The result is boot failure.

The reason is that a 64 bit unsigned interger which is set
to -1 on error is compared to -1UL which on a 32 bit system
with gcc is a 32 bit unsigned interger.
Since both are unsigned, no sign extension takes place and
comparison evaluates to non-equal.

There's no reason to do clever tricks: all functions
we call actually return int so just use int.
And then we can use == -1 everywhere, consistently.

Reviewed-by: Alexander Graf <agraf@suse.de>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/s390x/ipl.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index d69adb2..65d39da 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -62,10 +62,10 @@ typedef struct S390IPLState {
 static int s390_ipl_init(SysBusDevice *dev)
 {
     S390IPLState *ipl = S390_IPL(dev);
-    ram_addr_t kernel_size = 0;
+    int kernel_size;
 
     if (!ipl->kernel) {
-        ram_addr_t bios_size = 0;
+        int bios_size;
         char *bios_filename;
 
         /* Load zipl bootloader */
@@ -80,7 +80,7 @@ static int s390_ipl_init(SysBusDevice *dev)
 
         bios_size = load_elf(bios_filename, NULL, NULL, &ipl->start_addr, NULL,
                              NULL, 1, ELF_MACHINE, 0);
-        if (bios_size == -1UL) {
+        if (bios_size == -1) {
             bios_size = load_image_targphys(bios_filename, ZIPL_IMAGE_START,
                                             4096);
             ipl->start_addr = ZIPL_IMAGE_START;
@@ -90,17 +90,17 @@ static int s390_ipl_init(SysBusDevice *dev)
         }
         g_free(bios_filename);
 
-        if ((long)bios_size < 0) {
+        if (bios_size == -1) {
             hw_error("could not load bootloader '%s'\n", bios_name);
         }
         return 0;
     } else {
         kernel_size = load_elf(ipl->kernel, NULL, NULL, NULL, NULL,
                                NULL, 1, ELF_MACHINE, 0);
-        if (kernel_size == -1UL) {
+        if (kernel_size == -1) {
             kernel_size = load_image_targphys(ipl->kernel, 0, ram_size);
         }
-        if (kernel_size == -1UL) {
+        if (kernel_size == -1) {
             fprintf(stderr, "could not load kernel '%s'\n", ipl->kernel);
             return -1;
         }
@@ -115,7 +115,8 @@ static int s390_ipl_init(SysBusDevice *dev)
         ipl->start_addr = KERN_IMAGE_START;
     }
     if (ipl->initrd) {
-        ram_addr_t initrd_offset, initrd_size;
+        ram_addr_t initrd_offset;
+        int initrd_size;
 
         initrd_offset = INITRD_START;
         while (kernel_size + 0x100000 > initrd_offset) {
@@ -123,7 +124,7 @@ static int s390_ipl_init(SysBusDevice *dev)
         }
         initrd_size = load_image_targphys(ipl->initrd, initrd_offset,
                                           ram_size - initrd_offset);
-        if (initrd_size == -1UL) {
+        if (initrd_size == -1) {
             fprintf(stderr, "qemu: could not load initrd '%s'\n", ipl->initrd);
             exit(1);
         }
-- 
MST

[Qemu-devel] [PULL for-1.7 v2 2/6] pci: unregister vmstate_pcibus on unplug

[Michael S. Tsirkin]

From: Bandan Das <bsd@redhat.com>

PCIBus registers a vmstate during init. Unregister it upon
removal/unplug.

Signed-off-by: Bandan Das <bsd@redhat.com>
Cc: qemu-stable@nongnu.org
Reviewed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci/pci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index ed32059..49eca95 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -47,6 +47,7 @@ static void pcibus_dev_print(Monitor *mon, DeviceState *dev, int indent);
 static char *pcibus_get_dev_path(DeviceState *dev);
 static char *pcibus_get_fw_dev_path(DeviceState *dev);
 static int pcibus_reset(BusState *qbus);
+static void pci_bus_finalize(Object *obj);
 
 static Property pci_props[] = {
     DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
@@ -73,6 +74,7 @@ static const TypeInfo pci_bus_info = {
     .name = TYPE_PCI_BUS,
     .parent = TYPE_BUS,
     .instance_size = sizeof(PCIBus),
+    .instance_finalize = pci_bus_finalize,
     .class_init = pci_bus_class_init,
 };
 
@@ -375,6 +377,12 @@ int pci_bus_num(PCIBus *s)
     return s->parent_dev->config[PCI_SECONDARY_BUS];
 }
 
+static void pci_bus_finalize(Object *obj)
+{
+    PCIBus *bus = PCI_BUS(obj);
+    vmstate_unregister(NULL, &vmstate_pcibus, bus);
+}
+
 static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
 {
     PCIDevice *s = container_of(pv, PCIDevice, config);
-- 
MST

[Qemu-devel] [PULL for-1.7 v2 3/6] acpi-build: fix build on glib < 2.22

[Michael S. Tsirkin]

g_string_vprintf was only introduced in 2.24 so switch to vsnprintf
instead.  A bit uglier but name size is fixed at 4 bytes here so it's
easy.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reported-by: Richard Henderson <rth@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/acpi-build.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 486e705..59a17df 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -287,16 +287,17 @@ static inline void build_append_array(GArray *array, GArray *val)
 
 static void build_append_nameseg(GArray *array, const char *format, ...)
 {
-    GString *s = g_string_new("");
+    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
+    char s[] = "XXXX";
+    int len;
     va_list args;
 
     va_start(args, format);
-    g_string_vprintf(s, format, args);
+    len = vsnprintf(s, sizeof s, format, args);
     va_end(args);
 
-    assert(s->len == 4);
-    g_array_append_vals(array, s->str, s->len);
-    g_string_free(s, true);
+    assert(len == 4);
+    g_array_append_vals(array, s, len);
 }
 
 /* 5.4 Definition Block Encoding */
-- 
MST

[Qemu-devel] [PULL for-1.7 v2 4/6] acpi-build: fix build on glib < 2.14

[Michael S. Tsirkin]

g_array_get_element_size was only added in glib 2.14,
there's no way to find element size in with an older glib.

Fortunately we only use a single table (linker) where element size > 1.
Switch element size to 1 everywhere, then we can just look at len field
to get table size in bytes.

Add an assert to make sure we catch any violations of this rule.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reported-by: Richard Henderson <rth@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/acpi-build.c         | 5 ++++-
 hw/i386/bios-linker-loader.c | 8 ++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 59a17df..5f36e7e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -425,7 +425,10 @@ static inline void *acpi_data_push(GArray *table_data, unsigned size)
 
 static unsigned acpi_data_len(GArray *table)
 {
-    return table->len * g_array_get_element_size(table);
+#if GLIB_CHECK_VERSION(2, 14, 0)
+    assert(g_array_get_element_size(table) == 1);
+#endif
+    return table->len;
 }
 
 static void acpi_align_size(GArray *blob, unsigned align)
diff --git a/hw/i386/bios-linker-loader.c b/hw/i386/bios-linker-loader.c
index 0833853..fd23611 100644
--- a/hw/i386/bios-linker-loader.c
+++ b/hw/i386/bios-linker-loader.c
@@ -90,7 +90,7 @@ enum {
 
 GArray *bios_linker_loader_init(void)
 {
-    return g_array_new(false, true /* clear */, sizeof(BiosLinkerLoaderEntry));
+    return g_array_new(false, true /* clear */, 1);
 }
 
 /* Free linker wrapper and return the linker array. */
@@ -115,7 +115,7 @@ void bios_linker_loader_alloc(GArray *linker,
                                     BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH);
 
     /* Alloc entries must come first, so prepend them */
-    g_array_prepend_val(linker, entry);
+    g_array_prepend_vals(linker, &entry, sizeof entry);
 }
 
 void bios_linker_loader_add_checksum(GArray *linker, const char *file,
@@ -132,7 +132,7 @@ void bios_linker_loader_add_checksum(GArray *linker, const char *file,
     entry.cksum.start = cpu_to_le32((uint8_t *)start - (uint8_t *)table);
     entry.cksum.length = cpu_to_le32(size);
 
-    g_array_append_val(linker, entry);
+    g_array_append_vals(linker, &entry, sizeof entry);
 }
 
 void bios_linker_loader_add_pointer(GArray *linker,
@@ -154,5 +154,5 @@ void bios_linker_loader_add_pointer(GArray *linker,
     assert(pointer_size == 1 || pointer_size == 2 ||
            pointer_size == 4 || pointer_size == 8);
 
-    g_array_append_val(linker, entry);
+    g_array_append_vals(linker, &entry, sizeof entry);
 }
-- 
MST

[Qemu-devel] [PULL for-1.7 v2 5/6] Revert "e1000/rtl8139: update HMP NIC when every bit is written"

[Michael S. Tsirkin]

This reverts commit cd5be5829c1ce87aa6b3a7806524fac07ac9a757.
Digging into hardware specs shows this does not
actually make QEMU behave more like hardware:

There are valid arguments backed by the spec to indicate why the version
of e1000 prior to cd5be582 was more correct: the high byte actually
includes a valid bit, this is why all guests write it last.

For rtl8139 there's actually a separate undocumented valid bit, but we
don't implement it yet.

To summarize all the drivers we know about behave in one way
that allows us to make an assumption about write order and avoid
spurious, incorrect mac address updates to the monitor.

Let's stick to the tried heuristic for 1.7 and
possibly revisit for 1.8.

Reported-by: Vlad Yasevich <vyasevic@redhat.com>
Reviewed-by: Vlad Yasevich <vyasevic@redhat.com>
Cc: Amos Kong <akong@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/net/e1000.c   | 2 +-
 hw/net/rtl8139.c | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index ae63591..8387443 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1106,7 +1106,7 @@ mac_writereg(E1000State *s, int index, uint32_t val)
 
     s->mac_reg[index] = val;
 
-    if (index == RA || index == RA + 1) {
+    if (index == RA + 1) {
         macaddr[0] = cpu_to_le32(s->mac_reg[RA]);
         macaddr[1] = cpu_to_le32(s->mac_reg[RA + 1]);
         qemu_format_nic_info_str(qemu_get_queue(s->nic), (uint8_t *)macaddr);
diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 7f2b4db..5329f44 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -2741,7 +2741,10 @@ static void rtl8139_io_writeb(void *opaque, uint8_t addr, uint32_t val)
 
     switch (addr)
     {
-        case MAC0 ... MAC0+5:
+        case MAC0 ... MAC0+4:
+            s->phys[addr - MAC0] = val;
+            break;
+        case MAC0+5:
             s->phys[addr - MAC0] = val;
             qemu_format_nic_info_str(qemu_get_queue(s->nic), s->phys);
             break;
-- 
MST

[Qemu-devel] [PULL for-1.7 v2 6/6] configure: make --iasl option actually work

[Michael S. Tsirkin]

--iasl option was added to CC option parsing section by mistake,
it's not effective there and attempts to use cause
an 'unknown option' error.

Fix this up.

Tested-by: Marcel Apfelbaum <marcel.a@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 configure | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 508f6a5..9b9b9fa 100755
--- a/configure
+++ b/configure
@@ -272,8 +272,6 @@ for opt do
   ;;
   --cxx=*) CXX="$optarg"
   ;;
-  --iasl=*) iasl="$optarg"
-  ;;
   --source-path=*) source_path="$optarg"
   ;;
   --cpu=*) cpu="$optarg"
@@ -649,6 +647,8 @@ for opt do
   ;;
   --cxx=*)
   ;;
+  --iasl=*) iasl="$optarg"
+  ;;
   --objcc=*) objcc="$optarg"
   ;;
   --make=*) make="$optarg"
-- 
MST

Re: [Qemu-devel] [PULL for-1.7 v2 4/6] acpi-build: fix build on glib < 2.14

[Erik Rull]

Michael S. Tsirkin wrote:
> g_array_get_element_size was only added in glib 2.14,
> there's no way to find element size in with an older glib.
>
> Fortunately we only use a single table (linker) where element size > 1.
> Switch element size to 1 everywhere, then we can just look at len field
> to get table size in bytes.
>
> Add an assert to make sure we catch any violations of this rule.
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Reported-by: Richard Henderson <rth@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   hw/i386/acpi-build.c         | 5 ++++-
>   hw/i386/bios-linker-loader.c | 8 ++++----
>   2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 59a17df..5f36e7e 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -425,7 +425,10 @@ static inline void *acpi_data_push(GArray *table_data, unsigned size)
>
>   static unsigned acpi_data_len(GArray *table)
>   {
> -    return table->len * g_array_get_element_size(table);
> +#if GLIB_CHECK_VERSION(2, 14, 0)
> +    assert(g_array_get_element_size(table) == 1);
> +#endif
> +    return table->len;
>   }
>
>   static void acpi_align_size(GArray *blob, unsigned align)
> diff --git a/hw/i386/bios-linker-loader.c b/hw/i386/bios-linker-loader.c
> index 0833853..fd23611 100644
> --- a/hw/i386/bios-linker-loader.c
> +++ b/hw/i386/bios-linker-loader.c
> @@ -90,7 +90,7 @@ enum {
>
>   GArray *bios_linker_loader_init(void)
>   {
> -    return g_array_new(false, true /* clear */, sizeof(BiosLinkerLoaderEntry));
> +    return g_array_new(false, true /* clear */, 1);
>   }
>
>   /* Free linker wrapper and return the linker array. */
> @@ -115,7 +115,7 @@ void bios_linker_loader_alloc(GArray *linker,
>                                       BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH);
>
>       /* Alloc entries must come first, so prepend them */
> -    g_array_prepend_val(linker, entry);
> +    g_array_prepend_vals(linker, &entry, sizeof entry);
>   }
>
>   void bios_linker_loader_add_checksum(GArray *linker, const char *file,
> @@ -132,7 +132,7 @@ void bios_linker_loader_add_checksum(GArray *linker, const char *file,
>       entry.cksum.start = cpu_to_le32((uint8_t *)start - (uint8_t *)table);
>       entry.cksum.length = cpu_to_le32(size);
>
> -    g_array_append_val(linker, entry);
> +    g_array_append_vals(linker, &entry, sizeof entry);
>   }
>
>   void bios_linker_loader_add_pointer(GArray *linker,
> @@ -154,5 +154,5 @@ void bios_linker_loader_add_pointer(GArray *linker,
>       assert(pointer_size == 1 || pointer_size == 2 ||
>              pointer_size == 4 || pointer_size == 8);
>
> -    g_array_append_val(linker, entry);
> +    g_array_append_vals(linker, &entry, sizeof entry);
>   }
>

Hi all,

acpi-build.c has another occurence of g_array_get_element_size in 
acpi_align_size. Please put another ifdef for the older glib here, too.

Thanks.

Best regards,

Erik

Re: [Qemu-devel] [PULL for-1.7 v2 3/6] acpi-build: fix build on glib < 2.22

[Richard Henderson]

On 11/25/2013 09:48 PM, Michael S. Tsirkin wrote:
> g_string_vprintf was only introduced in 2.24 so switch to vsnprintf
> instead.  A bit uglier but name size is fixed at 4 bytes here so it's
> easy.

You list 2.24 here,

> -    GString *s = g_string_new("");
> +    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */

... 2.22 here.

But https://developer.gnome.org/glib/2.28/glib-Strings.html#g-string-vprintf

says "since 2.14".

> +    char s[] = "XXXX";

char s[5];

Initializing it is a waste of time.


r~

Re: [Qemu-devel] [PULL for-1.7 v2 4/6] acpi-build: fix build on glib < 2.14

[Richard Henderson]

On 11/25/2013 09:48 PM, Michael S. Tsirkin wrote:
> +#if GLIB_CHECK_VERSION(2, 14, 0)
> +    assert(g_array_get_element_size(table) == 1);
> +#endif

https://developer.gnome.org/glib/2.28/glib-Arrays.html#g-array-get-element-size

says "Since 2.22", not 2.14.


r~

Re: [Qemu-devel] [PULL for-1.7 v2 3/6] acpi-build: fix build on glib < 2.22

[Michael S. Tsirkin]

On Tue, Nov 26, 2013 at 06:24:53AM +1000, Richard Henderson wrote:
> On 11/25/2013 09:48 PM, Michael S. Tsirkin wrote:
> > g_string_vprintf was only introduced in 2.24 so switch to vsnprintf
> > instead.  A bit uglier but name size is fixed at 4 bytes here so it's
> > easy.
> 
> You list 2.24 here,
> 
> > -    GString *s = g_string_new("");
> > +    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> 
> ... 2.22 here.
> 
> But https://developer.gnome.org/glib/2.28/glib-Strings.html#g-string-vprintf
> 
> says "since 2.14".
> 
> > +    char s[] = "XXXX";
> 
> char s[5];
> 
> Initializing it is a waste of time.
> 
> 
> r~

It's sets the length in a nice way.

Re: [Qemu-devel] [PULL for-1.7 v2 3/6] acpi-build: fix build on glib < 2.22

[Richard Henderson]

On 11/26/2013 06:31 AM, Michael S. Tsirkin wrote:
> On Tue, Nov 26, 2013 at 06:24:53AM +1000, Richard Henderson wrote:
>> On 11/25/2013 09:48 PM, Michael S. Tsirkin wrote:
>>> g_string_vprintf was only introduced in 2.24 so switch to vsnprintf
>>> instead.  A bit uglier but name size is fixed at 4 bytes here so it's
>>> easy.
>>
>> You list 2.24 here,
>>
>>> -    GString *s = g_string_new("");
>>> +    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
>>
>> ... 2.22 here.
>>
>> But https://developer.gnome.org/glib/2.28/glib-Strings.html#g-string-vprintf
>>
>> says "since 2.14".
>>
>>> +    char s[] = "XXXX";
>>
>> char s[5];
>>
>> Initializing it is a waste of time.
>>
>>
>> r~
> 
> It's sets the length in a nice way.
> 

Then do something like

  char s[sizeof("XXXX")];

so that the actual initialization doesn't happen.


r~

Re: [Qemu-devel] [PULL for-1.7 v2 3/6] acpi-build: fix build on glib < 2.22

[Michael S. Tsirkin]

On Tue, Nov 26, 2013 at 06:37:43AM +1000, Richard Henderson wrote:
> On 11/26/2013 06:31 AM, Michael S. Tsirkin wrote:
> > On Tue, Nov 26, 2013 at 06:24:53AM +1000, Richard Henderson wrote:
> >> On 11/25/2013 09:48 PM, Michael S. Tsirkin wrote:
> >>> g_string_vprintf was only introduced in 2.24 so switch to vsnprintf
> >>> instead.  A bit uglier but name size is fixed at 4 bytes here so it's
> >>> easy.
> >>
> >> You list 2.24 here,
> >>
> >>> -    GString *s = g_string_new("");
> >>> +    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> >>
> >> ... 2.22 here.
> >>
> >> But https://developer.gnome.org/glib/2.28/glib-Strings.html#g-string-vprintf
> >>
> >> says "since 2.14".
> >>
> >>> +    char s[] = "XXXX";
> >>
> >> char s[5];
> >>
> >> Initializing it is a waste of time.
> >>
> >>
> >> r~
> > 
> > It's sets the length in a nice way.
> > 
> 
> Then do something like
> 
>   char s[sizeof("XXXX")];
> 
> so that the actual initialization doesn't happen.
> 
> 
> r~

Why? As an optimization?
I'm not quite sure this doesn't mean we are using VLA which I'd rather not.
Would need to look at language spec ... simple initialization is shorter
and more obviously correct.

Re: [Qemu-devel] [PULL for-1.7 v2 4/6] acpi-build: fix build on glib < 2.14

[Michael S. Tsirkin]

On Mon, Nov 25, 2013 at 09:14:19PM +0100, Erik Rull wrote:
> Michael S. Tsirkin wrote:
> >g_array_get_element_size was only added in glib 2.14,
> >there's no way to find element size in with an older glib.
> >
> >Fortunately we only use a single table (linker) where element size > 1.
> >Switch element size to 1 everywhere, then we can just look at len field
> >to get table size in bytes.
> >
> >Add an assert to make sure we catch any violations of this rule.
> >
> >Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> >Reported-by: Richard Henderson <rth@redhat.com>
> >Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >---
> >  hw/i386/acpi-build.c         | 5 ++++-
> >  hw/i386/bios-linker-loader.c | 8 ++++----
> >  2 files changed, 8 insertions(+), 5 deletions(-)
> >
> >diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >index 59a17df..5f36e7e 100644
> >--- a/hw/i386/acpi-build.c
> >+++ b/hw/i386/acpi-build.c
> >@@ -425,7 +425,10 @@ static inline void *acpi_data_push(GArray *table_data, unsigned size)
> >
> >  static unsigned acpi_data_len(GArray *table)
> >  {
> >-    return table->len * g_array_get_element_size(table);
> >+#if GLIB_CHECK_VERSION(2, 14, 0)
> >+    assert(g_array_get_element_size(table) == 1);
> >+#endif
> >+    return table->len;
> >  }
> >
> >  static void acpi_align_size(GArray *blob, unsigned align)
> >diff --git a/hw/i386/bios-linker-loader.c b/hw/i386/bios-linker-loader.c
> >index 0833853..fd23611 100644
> >--- a/hw/i386/bios-linker-loader.c
> >+++ b/hw/i386/bios-linker-loader.c
> >@@ -90,7 +90,7 @@ enum {
> >
> >  GArray *bios_linker_loader_init(void)
> >  {
> >-    return g_array_new(false, true /* clear */, sizeof(BiosLinkerLoaderEntry));
> >+    return g_array_new(false, true /* clear */, 1);
> >  }
> >
> >  /* Free linker wrapper and return the linker array. */
> >@@ -115,7 +115,7 @@ void bios_linker_loader_alloc(GArray *linker,
> >                                      BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH);
> >
> >      /* Alloc entries must come first, so prepend them */
> >-    g_array_prepend_val(linker, entry);
> >+    g_array_prepend_vals(linker, &entry, sizeof entry);
> >  }
> >
> >  void bios_linker_loader_add_checksum(GArray *linker, const char *file,
> >@@ -132,7 +132,7 @@ void bios_linker_loader_add_checksum(GArray *linker, const char *file,
> >      entry.cksum.start = cpu_to_le32((uint8_t *)start - (uint8_t *)table);
> >      entry.cksum.length = cpu_to_le32(size);
> >
> >-    g_array_append_val(linker, entry);
> >+    g_array_append_vals(linker, &entry, sizeof entry);
> >  }
> >
> >  void bios_linker_loader_add_pointer(GArray *linker,
> >@@ -154,5 +154,5 @@ void bios_linker_loader_add_pointer(GArray *linker,
> >      assert(pointer_size == 1 || pointer_size == 2 ||
> >             pointer_size == 4 || pointer_size == 8);
> >
> >-    g_array_append_val(linker, entry);
> >+    g_array_append_vals(linker, &entry, sizeof entry);
> >  }
> >
> 
> Hi all,
> 
> acpi-build.c has another occurence of g_array_get_element_size in
> acpi_align_size. Please put another ifdef for the older glib here,
> too.
> 
> Thanks.
> 
> Best regards,
> 
> Erik
> 

Do you have a system where this fails then?

Re: [Qemu-devel] [PULL for-1.7 v2 4/6] acpi-build: fix build on glib < 2.14

[Michael S. Tsirkin]

On Mon, Nov 25, 2013 at 09:14:19PM +0100, Erik Rull wrote:
> Michael S. Tsirkin wrote:
> >g_array_get_element_size was only added in glib 2.14,
> >there's no way to find element size in with an older glib.
> >
> >Fortunately we only use a single table (linker) where element size > 1.
> >Switch element size to 1 everywhere, then we can just look at len field
> >to get table size in bytes.
> >
> >Add an assert to make sure we catch any violations of this rule.
> >
> >Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> >Reported-by: Richard Henderson <rth@redhat.com>
> >Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >---
> >  hw/i386/acpi-build.c         | 5 ++++-
> >  hw/i386/bios-linker-loader.c | 8 ++++----
> >  2 files changed, 8 insertions(+), 5 deletions(-)
> >
> >diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >index 59a17df..5f36e7e 100644
> >--- a/hw/i386/acpi-build.c
> >+++ b/hw/i386/acpi-build.c
> >@@ -425,7 +425,10 @@ static inline void *acpi_data_push(GArray *table_data, unsigned size)
> >
> >  static unsigned acpi_data_len(GArray *table)
> >  {
> >-    return table->len * g_array_get_element_size(table);
> >+#if GLIB_CHECK_VERSION(2, 14, 0)
> >+    assert(g_array_get_element_size(table) == 1);
> >+#endif
> >+    return table->len;
> >  }
> >
> >  static void acpi_align_size(GArray *blob, unsigned align)
> >diff --git a/hw/i386/bios-linker-loader.c b/hw/i386/bios-linker-loader.c
> >index 0833853..fd23611 100644
> >--- a/hw/i386/bios-linker-loader.c
> >+++ b/hw/i386/bios-linker-loader.c
> >@@ -90,7 +90,7 @@ enum {
> >
> >  GArray *bios_linker_loader_init(void)
> >  {
> >-    return g_array_new(false, true /* clear */, sizeof(BiosLinkerLoaderEntry));
> >+    return g_array_new(false, true /* clear */, 1);
> >  }
> >
> >  /* Free linker wrapper and return the linker array. */
> >@@ -115,7 +115,7 @@ void bios_linker_loader_alloc(GArray *linker,
> >                                      BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH);
> >
> >      /* Alloc entries must come first, so prepend them */
> >-    g_array_prepend_val(linker, entry);
> >+    g_array_prepend_vals(linker, &entry, sizeof entry);
> >  }
> >
> >  void bios_linker_loader_add_checksum(GArray *linker, const char *file,
> >@@ -132,7 +132,7 @@ void bios_linker_loader_add_checksum(GArray *linker, const char *file,
> >      entry.cksum.start = cpu_to_le32((uint8_t *)start - (uint8_t *)table);
> >      entry.cksum.length = cpu_to_le32(size);
> >
> >-    g_array_append_val(linker, entry);
> >+    g_array_append_vals(linker, &entry, sizeof entry);
> >  }
> >
> >  void bios_linker_loader_add_pointer(GArray *linker,
> >@@ -154,5 +154,5 @@ void bios_linker_loader_add_pointer(GArray *linker,
> >      assert(pointer_size == 1 || pointer_size == 2 ||
> >             pointer_size == 4 || pointer_size == 8);
> >
> >-    g_array_append_val(linker, entry);
> >+    g_array_append_vals(linker, &entry, sizeof entry);
> >  }
> >
> 
> Hi all,
> 
> acpi-build.c has another occurence of g_array_get_element_size in
> acpi_align_size. Please put another ifdef for the older glib here,
> too.
> 
> Thanks.
> 
> Best regards,
> 
> Erik
> 


Can you confirm this works?


diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 5f36e7e..1f22fb6 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -425,7 +425,7 @@ static inline void *acpi_data_push(GArray *table_data, unsigned size)
 
 static unsigned acpi_data_len(GArray *table)
 {
-#if GLIB_CHECK_VERSION(2, 14, 0)
+#if GLIB_CHECK_VERSION(2, 22, 0)
     assert(g_array_get_element_size(table) == 1);
 #endif
     return table->len;
@@ -436,9 +436,7 @@ static void acpi_align_size(GArray *blob, unsigned align)
     /* Align size to multiple of given size. This reduces the chance
      * we need to change size in the future (breaking cross version migration).
      */
-    g_array_set_size(blob, (ROUND_UP(acpi_data_len(blob), align) +
-                            g_array_get_element_size(blob) - 1) /
-                             g_array_get_element_size(blob));
+    g_array_set_size(blob, ROUND_UP(acpi_data_len(blob), align));
 }
 
 /* Get pointer within table in a safe manner */

Re: [Qemu-devel] [PULL for-1.7 v2 3/6] acpi-build: fix build on glib < 2.22

[Michael S. Tsirkin]

On Tue, Nov 26, 2013 at 06:37:43AM +1000, Richard Henderson wrote:
> On 11/26/2013 06:31 AM, Michael S. Tsirkin wrote:
> > On Tue, Nov 26, 2013 at 06:24:53AM +1000, Richard Henderson wrote:
> >> On 11/25/2013 09:48 PM, Michael S. Tsirkin wrote:
> >>> g_string_vprintf was only introduced in 2.24 so switch to vsnprintf
> >>> instead.  A bit uglier but name size is fixed at 4 bytes here so it's
> >>> easy.
> >>
> >> You list 2.24 here,
> >>
> >>> -    GString *s = g_string_new("");
> >>> +    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> >>
> >> ... 2.22 here.
> >>
> >> But https://developer.gnome.org/glib/2.28/glib-Strings.html#g-string-vprintf
> >>
> >> says "since 2.14".
> >>
> >>> +    char s[] = "XXXX";
> >>
> >> char s[5];
> >>
> >> Initializing it is a waste of time.
> >>
> >>
> >> r~
> > 
> > It's sets the length in a nice way.
> > 
> 
> Then do something like
> 
>   char s[sizeof("XXXX")];
> 
> so that the actual initialization doesn't happen.
> 
> 
> r~

In any case it's too late to play with cosmetic changes for 1.7.
We can revisit for 1.8.

Re: [Qemu-devel] [PULL for-1.7 v2 3/6] acpi-build: fix build on glib < 2.22

[Laszlo Ersek]

On 11/25/13 21:54, Michael S. Tsirkin wrote:
> On Tue, Nov 26, 2013 at 06:37:43AM +1000, Richard Henderson wrote:
>> On 11/26/2013 06:31 AM, Michael S. Tsirkin wrote:
>>> On Tue, Nov 26, 2013 at 06:24:53AM +1000, Richard Henderson wrote:
>>>> On 11/25/2013 09:48 PM, Michael S. Tsirkin wrote:
>>>>> g_string_vprintf was only introduced in 2.24 so switch to vsnprintf
>>>>> instead.  A bit uglier but name size is fixed at 4 bytes here so it's
>>>>> easy.
>>>>
>>>> You list 2.24 here,
>>>>
>>>>> -    GString *s = g_string_new("");
>>>>> +    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
>>>>
>>>> ... 2.22 here.
>>>>
>>>> But https://developer.gnome.org/glib/2.28/glib-Strings.html#g-string-vprintf
>>>>
>>>> says "since 2.14".
>>>>
>>>>> +    char s[] = "XXXX";
>>>>
>>>> char s[5];
>>>>
>>>> Initializing it is a waste of time.
>>>>
>>>>
>>>> r~
>>>
>>> It's sets the length in a nice way.
>>>
>>
>> Then do something like
>>
>>   char s[sizeof("XXXX")];
>>
>> so that the actual initialization doesn't happen.
>>
>>
>> r~
> 
> Why? As an optimization?
> I'm not quite sure this doesn't mean we are using VLA which I'd rather not.
> Would need to look at language spec ... simple initialization is shorter
> and more obviously correct.

No, it's not a VLA.

C99 6.7.5.2 Array declarators, p4:

    [...] If the size is an integer constant expression and the element
    type has a known constant size, the array type is not a variable
    length array type; [...]

6.6 Constant expressions, p6:

    An integer constant expression [...] shall have integer type and
    shall only have operands that are [...] sizeof expressions whose
    results are integer constants, [...]

6.5.3.4 The sizeof operator, p2:

    [...] The result is an integer. If the type of the operand is a
    variable length array type, the operand is evaluated; otherwise,
    the operand is not evaluated and the result is an integer constant.

6.4.5 String literals, p2:

    [...] The multibyte character sequence is then used to initialize
    an array of static storage duration and length just sufficient to
    contain the sequence. [...]

6.7.5.2 Array declarators, p2:

    [...] If an identifier is declared to be an object with static
    storage duration, it shall not have a variable length array type.

6.7.5.2 Array declarators, p10:

    EXAMPLE 4 [...] Array objects declared with the static or extern
    storage-class specifier cannot have a variable length array (VLA)
    type. [...]

So,

    char s[sizeof("XXXX")]
                  ^^^^^^
                  string literal, static storage duration, not a VLA

           ^^^^^^^^^^^^^^
           operand not evaluated, result is integer constant
           ... which qualifies as integer constant expression

    ^^^^^^^^^^^^^^^^^^^^^^
    size is an integer constant expression,
    element type has a known constant size: not a VLA

(Admittedly, EXAMPLE 4 in 6.7.5.2 Array declarators, p10, is informative
(not normative), and 6.7.5.2 Array declarators, p2, speaks about an
"identifier". We don't have an identifier for "XXXX", but I think we can
still derive that static storage duration implies non-variable length
for the array that holds the string.)

Laszlo

Re: [Qemu-devel] [PULL for-1.7 v2 3/6] acpi-build: fix build on glib < 2.22

[Richard Henderson]

On 11/26/2013 06:54 AM, Michael S. Tsirkin wrote:
>>>>> +    char s[] = "XXXX";
>>>>
>>>> char s[5];
>>>>
>>
>> Then do something like
>>
>>   char s[sizeof("XXXX")];
>>
>> so that the actual initialization doesn't happen.
> Why? As an optimization?

How about failing to pessimize?

With your initialization you're forcing the compiler to do:

  char s[5];
  memcpy(s, "XXXX\0", 5);

possibly with the memcpy expanded inline.

Since we pass the address of S to vnsprintf, the compiler has to assume that
memory is read, and thus the initialization is needed.  It can never be
optimized away.

> I'm not quite sure this doesn't mean we are using VLA which I'd rather not.
> Would need to look at language spec ... simple initialization is shorter
> and more obviously correct.

I'm quite sure that using sizeof does not imply a VLA.



r~

Re: [Qemu-devel] [PULL for-1.7 v2 3/6] acpi-build: fix build on glib < 2.22

[Richard Henderson]

On 11/26/2013 07:02 AM, Michael S. Tsirkin wrote:
> In any case it's too late to play with cosmetic changes for 1.7.
> We can revisit for 1.8.

I beg your pardon?  I didn't realize your patch had already been applied.  And
since it has mistakes that require respin anyway, why can't we change this too?


r~

Re: [Qemu-devel] [PULL for-1.7 v2 3/6] acpi-build: fix build on glib < 2.22

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1109 bytes --]

On 11/25/2013 02:15 PM, Laszlo Ersek wrote:

>>>>>> +    char s[] = "XXXX";
>>>>>
>>>>> char s[5];
>>>>>
>>>>> Initializing it is a waste of time.

And storage - the string literal occupies space in the binary image.

>>> Then do something like
>>>
>>>   char s[sizeof("XXXX")];

Not to mention we already have at least one example of that idiom:

disas/cris.c:  char temp[sizeof (".d [$r13=$r12-2147483648],$r10") * 2];

> (Admittedly, EXAMPLE 4 in 6.7.5.2 Array declarators, p10, is informative
> (not normative), and 6.7.5.2 Array declarators, p2, speaks about an
> "identifier". We don't have an identifier for "XXXX", but I think we can
> still derive that static storage duration implies non-variable length
> for the array that holds the string.)

Yes, use of char s[sizeof("")] is a fairly common idiom in C
programming, when you want to size the array large enough for it's
worst-case contents without actually wasting initialization/storage to
those contents.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PULL for-1.7 v2 3/6] acpi-build: fix build on glib < 2.22

[Michael S. Tsirkin]

On Tue, Nov 26, 2013 at 07:18:04AM +1000, Richard Henderson wrote:
> On 11/26/2013 06:54 AM, Michael S. Tsirkin wrote:
> >>>>> +    char s[] = "XXXX";
> >>>>
> >>>> char s[5];
> >>>>
> >>
> >> Then do something like
> >>
> >>   char s[sizeof("XXXX")];
> >>
> >> so that the actual initialization doesn't happen.
> > Why? As an optimization?
> 
> How about failing to pessimize?
> 
> With your initialization you're forcing the compiler to do:
> 
>   char s[5];
>   memcpy(s, "XXXX\0", 5);
> 
> possibly with the memcpy expanded inline.
> Since we pass the address of S to vnsprintf, the compiler has to assume that
> memory is read, and thus the initialization is needed.  It can never be
> optimized away.

Who cares?
It runs once on initialization.
You worry about a cost of memcpy?
FYI we are doing a crazy number of memory
allocations in this code.
I'm much more concerned with the fact that unlike the
glib based variant we had before this is doing
pointer math and relies on correct parameters
to be passed to snprintf to avoid buffer overruns.

I haven't found a way around that that will also
keep old systems happy, and the snippet is tiny so
maybe it's not too bad ...

> 
> > I'm not quite sure this doesn't mean we are using VLA which I'd rather not.
> > Would need to look at language spec ... simple initialization is shorter
> > and more obviously correct.
> 
> I'm quite sure that using sizeof does not imply a VLA.
> 
> 
> 
> r~

Re: [Qemu-devel] [PULL for-1.7 v2 3/6] acpi-build: fix build on glib < 2.22

[Michael S. Tsirkin]

On Tue, Nov 26, 2013 at 07:21:52AM +1000, Richard Henderson wrote:
> On 11/26/2013 07:02 AM, Michael S. Tsirkin wrote:
> > In any case it's too late to play with cosmetic changes for 1.7.
> > We can revisit for 1.8.
> 
> I beg your pardon?  I didn't realize your patch had already been applied.

When you see PATCH you should review.
When you see PULL it's been applied and pushed
to a public branch.

>  And
> since it has mistakes that require respin anyway, why can't we change this too?
> 
> 
> r~

The patch we discuss is correct, the patch removing get element
size didn't remove it in all places, but it's not making
things any worse so I'll fix them with applying a patch on top.

I queued other things on top and it's 11:30pm here.
Guys, go discuss the color of some other bikeshed.

-- 
MST

Re: [Qemu-devel] [PULL for-1.7 v2 4/6] acpi-build: fix build on glib < 2.14

[Richard Henderson]

On 11/26/2013 07:01 AM, Michael S. Tsirkin wrote:
> Can you confirm this works?

I can confirm that with this follow-on I can once again build on RHEL 5.3.


r~

Re: [Qemu-devel] [PULL for-1.7 v2 3/6] acpi-build: fix build on glib < 2.22

[Laszlo Ersek]

On 11/25/13 22:33, Michael S. Tsirkin wrote:

> I queued other things on top and it's 11:30pm here.
> Guys, go discuss the color of some other bikeshed.

(Just to be clear, I deliberately didn't address how the code in
question *should* be formulated. I'll leave that to you all to figure
out. I only cared about the question whether VLAs were involved in that
declaration or not.)

Laszlo

Re: [Qemu-devel] [PULL for-1.7 v2 4/6] acpi-build: fix build on glib < 2.14

[Erik Rull]

Richard Henderson wrote:
> On 11/26/2013 07:01 AM, Michael S. Tsirkin wrote:
>> Can you confirm this works?
>
> I can confirm that with this follow-on I can once again build on RHEL 5.3.
>
>
> r~
>

Sorry, a bit late, but yes, it compiles on my Debian 4.0. My test target is 
down at the moment, I try to get it running again tomorrow.

Thanks for your help. I will let you know if the execution of the binary 
was successful.

Best regards,

Erik