[PATCH v2 0/9] hw, ui, virtfs-proxy-helper: Reduce QEMU .data/.rodata/.bss footprint

[PATCH v2 0/9] hw, ui, virtfs-proxy-helper: Reduce QEMU .data/.rodata/.bss footprint

[Philippe Mathieu-Daudé]

Since v1:
- merged 2 series
- reworked hw/usb/quirks
- added R-b/A-b tags

This series reduce the footprint of the QEMU binary:
.bss: 106KiB (moved to .heap)
.data: 1MiB
.rodata: 4.34MiB
(sizes on x86_64 building with -Os)

The elf-dissector tool [1] [2] helped to notice the big array.

[1] https://phabricator.kde.org/source/elf-dissector/
[2] https://www.volkerkrause.eu/2019/06/22/elf-dissector-aarch64-support.html
[heap equivalent tool working with QEMU: https://github.com/KDE/heaptrack]

Supersedes: <20200304221807.25212-1-philmd@redhat.com>
Supersedes: <20200305010446.17029-1-philmd@redhat.com>

Philippe Mathieu-Daudé (9):
  hw/audio/fmopl: Fix a typo twice
  hw/audio/fmopl: Move ENV_CURVE to .heap to save 32KiB of .bss
  hw/audio/intel-hda: Use memory region alias to reduce .rodata by
    4.34MB
  hw/net/e1000: Add readops/writeops typedefs
  hw/net/e1000: Move macreg[] arrays to .rodata to save 1MiB of .data
  hw/usb/quirks: Use smaller types to reduce .rodata by 10KiB
  ui/curses: Make control_characters[] array const
  ui/curses: Move arrays to .heap to save 74KiB of .bss
  virtfs-proxy-helper: Make the helper_opts[] array const

 hw/usb/quirks.h             | 22 +++++++++++++---------
 fsdev/virtfs-proxy-helper.c |  2 +-
 hw/audio/fmopl.c            |  8 +++++---
 hw/audio/intel-hda.c        | 24 ++++++++++--------------
 hw/net/e1000.c              |  6 ++++--
 hw/net/e1000e_core.c        |  6 ++++--
 hw/usb/quirks.c             |  4 ++--
 ui/curses.c                 | 10 +++++++---
 8 files changed, 46 insertions(+), 36 deletions(-)

-- 
2.21.1

[PATCH v2 1/9] hw/audio/fmopl: Fix a typo twice

[Philippe Mathieu-Daudé]

Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/audio/fmopl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
index 9f50a89b4a..173a7521f2 100644
--- a/hw/audio/fmopl.c
+++ b/hw/audio/fmopl.c
@@ -1066,7 +1066,7 @@ static void OPLResetChip(FM_OPL *OPL)
 	}
 }
 
-/* ----------  Create one of vietual YM3812 ----------       */
+/* ----------  Create one of virtual YM3812 ----------       */
 /* 'rate'  is sampling rate and 'bufsiz' is the size of the  */
 FM_OPL *OPLCreate(int clock, int rate)
 {
@@ -1115,7 +1115,7 @@ FM_OPL *OPLCreate(int clock, int rate)
 	return OPL;
 }
 
-/* ----------  Destroy one of vietual YM3812 ----------       */
+/* ----------  Destroy one of virtual YM3812 ----------       */
 void OPLDestroy(FM_OPL *OPL)
 {
 #ifdef OPL_OUTPUT_LOG
-- 
2.21.1

[PATCH v2 2/9] hw/audio/fmopl: Move ENV_CURVE to .heap to save 32KiB of .bss

[Philippe Mathieu-Daudé]

This buffer is only used by the adlib audio device. Move it to
the .heap to release 32KiB of .bss (size reported on x86_64 host).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/audio/fmopl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
index 173a7521f2..356d4dfbca 100644
--- a/hw/audio/fmopl.c
+++ b/hw/audio/fmopl.c
@@ -186,7 +186,7 @@ static int32_t *VIB_TABLE;
 
 /* envelope output curve table */
 /* attack + decay + OFF */
-static int32_t ENV_CURVE[2*EG_ENT+1];
+static int32_t *ENV_CURVE;
 
 /* multiple table */
 #define ML 2
@@ -1090,6 +1090,7 @@ FM_OPL *OPLCreate(int clock, int rate)
 	OPL->clock = clock;
 	OPL->rate  = rate;
 	OPL->max_ch = max_ch;
+    ENV_CURVE = g_new(int32_t, 2 * EG_ENT + 1);
 	/* init grobal tables */
 	OPL_initialize(OPL);
 	/* reset chip */
@@ -1127,6 +1128,7 @@ void OPLDestroy(FM_OPL *OPL)
 #endif
 	OPL_UnLockTable();
 	free(OPL);
+    g_free(ENV_CURVE);
 }
 
 /* ----------  Option handlers ----------       */
-- 
2.21.1

[PATCH v2 3/9] hw/audio/intel-hda: Use memory region alias to reduce .rodata by 4.34MB

[Philippe Mathieu-Daudé]

The intel-hda model uses an array of register indexed by the
register address. This array also contains a pair of aliased
registers at offset 0x2000. This creates a huge hole in the
array, which ends up eating 4.6MiB of .rodata (size reported
on x86_64 host, building with --extra-cflags=-Os).

By using a memory region alias, we reduce this array to 132kB.

Before:

  (qemu) info mtree
    00000000febd4000-00000000febd7fff (prio 1, i/o): intel-hda

After:

  (qemu) info mtree
    00000000febd4000-00000000febd7fff (prio 1, i/o): intel-hda
    00000000febd4000-00000000febd7fff (prio 1, i/o): intel-hda-container
      00000000febd4000-00000000febd5fff (prio 0, i/o): intel-hda
      00000000febd6000-00000000febd7fff (prio 0, i/o): alias intel-hda-alias @intel-hda 0000000000000000-0000000000001fff

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/audio/intel-hda.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 1bcc3e5cf8..e8d18b7c58 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -181,7 +181,9 @@ struct IntelHDAState {
     IntelHDAStream st[8];
 
     /* state */
+    MemoryRegion container;
     MemoryRegion mmio;
+    MemoryRegion alias;
     uint32_t rirb_count;
     int64_t wall_base_ns;
 
@@ -670,12 +672,6 @@ static const struct IntelHDAReg regtab[] = {
         .offset   = offsetof(IntelHDAState, wall_clk),
         .rhandler = intel_hda_get_wall_clk,
     },
-    [ ICH6_REG_WALLCLK + 0x2000 ] = {
-        .name     = "WALLCLK(alias)",
-        .size     = 4,
-        .offset   = offsetof(IntelHDAState, wall_clk),
-        .rhandler = intel_hda_get_wall_clk,
-    },
 
     /* dma engine */
     [ ICH6_REG_CORBLBASE ] = {
@@ -837,12 +833,6 @@ static const struct IntelHDAReg regtab[] = {
         .size     = 4,                                                \
         .offset   = offsetof(IntelHDAState, st[_i].lpib),             \
     },                                                                \
-    [ ST_REG(_i, ICH6_REG_SD_LPIB) + 0x2000 ] = {                     \
-        .stream   = _i,                                               \
-        .name     = _t stringify(_i) " LPIB(alias)",                  \
-        .size     = 4,                                                \
-        .offset   = offsetof(IntelHDAState, st[_i].lpib),             \
-    },                                                                \
     [ ST_REG(_i, ICH6_REG_SD_CBL) ] = {                               \
         .stream   = _i,                                               \
         .name     = _t stringify(_i) " CBL",                          \
@@ -1125,9 +1115,15 @@ static void intel_hda_realize(PCIDevice *pci, Error **errp)
         error_free(err);
     }
 
+    memory_region_init(&d->container, OBJECT(d),
+                       "intel-hda-container", 0x4000);
     memory_region_init_io(&d->mmio, OBJECT(d), &intel_hda_mmio_ops, d,
-                          "intel-hda", 0x4000);
-    pci_register_bar(&d->pci, 0, 0, &d->mmio);
+                          "intel-hda", 0x2000);
+    memory_region_add_subregion(&d->container, 0x0000, &d->mmio);
+    memory_region_init_alias(&d->alias, OBJECT(d), "intel-hda-alias",
+                             &d->mmio, 0, 0x2000);
+    memory_region_add_subregion(&d->container, 0x2000, &d->alias);
+    pci_register_bar(&d->pci, 0, 0, &d->container);
 
     hda_codec_bus_init(DEVICE(pci), &d->codecs, sizeof(d->codecs),
                        intel_hda_response, intel_hda_xfer);
-- 
2.21.1

[PATCH v2 4/9] hw/net/e1000: Add readops/writeops typedefs

[Philippe Mathieu-Daudé]

Express the macreg[] arrays using typedefs.
No logical changes introduced here.

Reviewed-by: Dmitry Fleytman <dmitry.fleytman@gmail.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/net/e1000.c       | 6 ++++--
 hw/net/e1000e_core.c | 6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 0b833d5a15..972d9b5083 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1150,7 +1150,8 @@ set_ims(E1000State *s, int index, uint32_t val)
 }
 
 #define getreg(x)    [x] = mac_readreg
-static uint32_t (*macreg_readops[])(E1000State *, int) = {
+typedef uint32_t (*readops)(E1000State *, int);
+static readops macreg_readops[] = {
     getreg(PBA),      getreg(RCTL),     getreg(TDH),      getreg(TXDCTL),
     getreg(WUFC),     getreg(TDT),      getreg(CTRL),     getreg(LEDCTL),
     getreg(MANC),     getreg(MDIC),     getreg(SWSM),     getreg(STATUS),
@@ -1205,7 +1206,8 @@ static uint32_t (*macreg_readops[])(E1000State *, int) = {
 enum { NREADOPS = ARRAY_SIZE(macreg_readops) };
 
 #define putreg(x)    [x] = mac_writereg
-static void (*macreg_writeops[])(E1000State *, int, uint32_t) = {
+typedef void (*writeops)(E1000State *, int, uint32_t);
+static writeops macreg_writeops[] = {
     putreg(PBA),      putreg(EERD),     putreg(SWSM),     putreg(WUFC),
     putreg(TDBAL),    putreg(TDBAH),    putreg(TXDCTL),   putreg(RDBAH),
     putreg(RDBAL),    putreg(LEDCTL),   putreg(VET),      putreg(FCRUC),
diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index 94ea34dca5..38bdb90114 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -2855,7 +2855,8 @@ e1000e_set_gcr(E1000ECore *core, int index, uint32_t val)
 }
 
 #define e1000e_getreg(x)    [x] = e1000e_mac_readreg
-static uint32_t (*e1000e_macreg_readops[])(E1000ECore *, int) = {
+typedef uint32_t (*readops)(E1000ECore *, int);
+static readops e1000e_macreg_readops[] = {
     e1000e_getreg(PBA),
     e1000e_getreg(WUFC),
     e1000e_getreg(MANC),
@@ -3061,7 +3062,8 @@ static uint32_t (*e1000e_macreg_readops[])(E1000ECore *, int) = {
 enum { E1000E_NREADOPS = ARRAY_SIZE(e1000e_macreg_readops) };
 
 #define e1000e_putreg(x)    [x] = e1000e_mac_writereg
-static void (*e1000e_macreg_writeops[])(E1000ECore *, int, uint32_t) = {
+typedef void (*writeops)(E1000ECore *, int, uint32_t);
+static writeops e1000e_macreg_writeops[] = {
     e1000e_putreg(PBA),
     e1000e_putreg(SWSM),
     e1000e_putreg(WUFC),
-- 
2.21.1

[PATCH v2 5/9] hw/net/e1000: Move macreg[] arrays to .rodata to save 1MiB of .data

[Philippe Mathieu-Daudé]

Each array consumes 256KiB of .data. As we do not reassign entries,
we can move it to the .rodata section, and save a total of 1MiB of
.data (size reported on x86_64 host).

Reviewed-by: Dmitry Fleytman <dmitry.fleytman@gmail.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/net/e1000.c       | 4 ++--
 hw/net/e1000e_core.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 972d9b5083..9233248c9a 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1151,7 +1151,7 @@ set_ims(E1000State *s, int index, uint32_t val)
 
 #define getreg(x)    [x] = mac_readreg
 typedef uint32_t (*readops)(E1000State *, int);
-static readops macreg_readops[] = {
+static const readops macreg_readops[] = {
     getreg(PBA),      getreg(RCTL),     getreg(TDH),      getreg(TXDCTL),
     getreg(WUFC),     getreg(TDT),      getreg(CTRL),     getreg(LEDCTL),
     getreg(MANC),     getreg(MDIC),     getreg(SWSM),     getreg(STATUS),
@@ -1207,7 +1207,7 @@ enum { NREADOPS = ARRAY_SIZE(macreg_readops) };
 
 #define putreg(x)    [x] = mac_writereg
 typedef void (*writeops)(E1000State *, int, uint32_t);
-static writeops macreg_writeops[] = {
+static const writeops macreg_writeops[] = {
     putreg(PBA),      putreg(EERD),     putreg(SWSM),     putreg(WUFC),
     putreg(TDBAL),    putreg(TDBAH),    putreg(TXDCTL),   putreg(RDBAH),
     putreg(RDBAL),    putreg(LEDCTL),   putreg(VET),      putreg(FCRUC),
diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index 38bdb90114..df957e0c1a 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -2856,7 +2856,7 @@ e1000e_set_gcr(E1000ECore *core, int index, uint32_t val)
 
 #define e1000e_getreg(x)    [x] = e1000e_mac_readreg
 typedef uint32_t (*readops)(E1000ECore *, int);
-static readops e1000e_macreg_readops[] = {
+static const readops e1000e_macreg_readops[] = {
     e1000e_getreg(PBA),
     e1000e_getreg(WUFC),
     e1000e_getreg(MANC),
@@ -3063,7 +3063,7 @@ enum { E1000E_NREADOPS = ARRAY_SIZE(e1000e_macreg_readops) };
 
 #define e1000e_putreg(x)    [x] = e1000e_mac_writereg
 typedef void (*writeops)(E1000ECore *, int, uint32_t);
-static writeops e1000e_macreg_writeops[] = {
+static const writeops e1000e_macreg_writeops[] = {
     e1000e_putreg(PBA),
     e1000e_putreg(SWSM),
     e1000e_putreg(WUFC),
-- 
2.21.1

[PATCH v2 6/9] hw/usb/quirks: Use smaller types to reduce .rodata by 10KiB

[Philippe Mathieu-Daudé]

The USB descriptor sizes are specified as 16-bit for idVendor /
idProduct, and 8-bit for bInterfaceClass / bInterfaceSubClass /
bInterfaceProtocol. Doing so we reduce the usbredir_raw_serial_ids[]
and usbredir_ftdi_serial_ids[] arrays from 16KiB to 6KiB (size
reported on x86_64 host, building with --extra-cflags=-Os).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2: Add bitfield values and use unsigned types (Gerd)
---
 hw/usb/quirks.h | 22 +++++++++++++---------
 hw/usb/quirks.c |  4 ++--
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/hw/usb/quirks.h b/hw/usb/quirks.h
index 89480befd7..50ef2f9c2e 100644
--- a/hw/usb/quirks.h
+++ b/hw/usb/quirks.h
@@ -21,19 +21,23 @@
 #include "quirks-pl2303-ids.h"
 
 struct usb_device_id {
-    int vendor_id;
-    int product_id;
-    int interface_class;
-    int interface_subclass;
-    int interface_protocol;
+    uint16_t vendor_id;
+    uint16_t product_id;
+    uint8_t interface_class;
+    uint8_t interface_subclass;
+    uint8_t interface_protocol;
+    uint8_t interface_protocol_used:1,
+            terminating_entry:1,
+            reserved:6;
 };
 
 #define USB_DEVICE(vendor, product) \
-    .vendor_id = vendor, .product_id = product, .interface_class = -1,
+    .vendor_id = vendor, .product_id = product, .interface_protocol_used = 0,
 
 #define USB_DEVICE_AND_INTERFACE_INFO(vend, prod, iclass, isubclass, iproto) \
     .vendor_id = vend, .product_id = prod, .interface_class = iclass, \
-    .interface_subclass = isubclass, .interface_protocol = iproto
+    .interface_subclass = isubclass, .interface_protocol = iproto, \
+    .interface_protocol_used = 1
 
 static const struct usb_device_id usbredir_raw_serial_ids[] = {
     /*
@@ -206,7 +210,7 @@ static const struct usb_device_id usbredir_raw_serial_ids[] = {
     { USB_DEVICE(ADLINK_VENDOR_ID, ADLINK_ND6530_PRODUCT_ID) },
     { USB_DEVICE(SMART_VENDOR_ID, SMART_PRODUCT_ID) },
 
-    { USB_DEVICE(-1, -1) } /* Terminating Entry */
+    { .terminating_entry = 1 } /* Terminating Entry */
 };
 
 static const struct usb_device_id usbredir_ftdi_serial_ids[] = {
@@ -906,7 +910,7 @@ static const struct usb_device_id usbredir_ftdi_serial_ids[] = {
     { USB_DEVICE(FTDI_VID, FTDI_DISTORTEC_JTAG_LOCK_PICK_PID) },
     { USB_DEVICE(FTDI_VID, FTDI_LUMEL_PD12_PID) },
 
-    { USB_DEVICE(-1, -1) } /* Terminating Entry */
+    { .terminating_entry = 1 } /* Terminating Entry */
 };
 
 #undef USB_DEVICE
diff --git a/hw/usb/quirks.c b/hw/usb/quirks.c
index 38a9c5634a..23ea7a23ea 100644
--- a/hw/usb/quirks.c
+++ b/hw/usb/quirks.c
@@ -22,10 +22,10 @@ static bool usb_id_match(const struct usb_device_id *ids,
                          uint8_t interface_protocol) {
     int i;
 
-    for (i = 0; ids[i].vendor_id != -1; i++) {
+    for (i = 0; ids[i].terminating_entry == 0; i++) {
         if (ids[i].vendor_id  == vendor_id &&
             ids[i].product_id == product_id &&
-            (ids[i].interface_class == -1 ||
+            (ids[i].interface_protocol_used == 0 ||
              (ids[i].interface_class == interface_class &&
               ids[i].interface_subclass == interface_subclass &&
               ids[i].interface_protocol == interface_protocol))) {
-- 
2.21.1

[PATCH v2 7/9] ui/curses: Make control_characters[] array const

[Philippe Mathieu-Daudé]

As we only use this array as input, make it const.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 ui/curses.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/curses.c b/ui/curses.c
index 3a1b71451c..3bafc10c1c 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -529,7 +529,7 @@ static void font_setup(void)
      * Control characters are normally non-printable, but VGA does have
      * well-known glyphs for them.
      */
-    static uint16_t control_characters[0x20] = {
+    static const uint16_t control_characters[0x20] = {
       0x0020,
       0x263a,
       0x263b,
-- 
2.21.1

[PATCH v2 8/9] ui/curses: Move arrays to .heap to save 74KiB of .bss

[Philippe Mathieu-Daudé]

We only need these arrays when using the curses display.
Move them from the .bss to the .heap (sizes reported on
x86_64 host: screen[] is 64KiB, vga_to_curses 7KiB).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 ui/curses.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/ui/curses.c b/ui/curses.c
index 3bafc10c1c..a59b23a9cf 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -54,13 +54,13 @@ enum maybe_keycode {
 };
 
 static DisplayChangeListener *dcl;
-static console_ch_t screen[160 * 100];
+static console_ch_t *screen;
 static WINDOW *screenpad = NULL;
 static int width, height, gwidth, gheight, invalidate;
 static int px, py, sminx, sminy, smaxx, smaxy;
 
 static const char *font_charset = "CP437";
-static cchar_t vga_to_curses[256];
+static cchar_t *vga_to_curses;
 
 static void curses_update(DisplayChangeListener *dcl,
                           int x, int y, int w, int h)
@@ -405,6 +405,8 @@ static void curses_refresh(DisplayChangeListener *dcl)
 static void curses_atexit(void)
 {
     endwin();
+    g_free(vga_to_curses);
+    g_free(screen);
 }
 
 /*
@@ -783,6 +785,8 @@ static void curses_display_init(DisplayState *ds, DisplayOptions *opts)
     if (opts->u.curses.charset) {
         font_charset = opts->u.curses.charset;
     }
+    screen = g_new0(console_ch_t, 160 * 100);
+    vga_to_curses = g_new0(cchar_t, 256);
     curses_setup();
     curses_keyboard_setup();
     atexit(curses_atexit);
-- 
2.21.1

[PATCH v2 9/9] virtfs-proxy-helper: Make the helper_opts[] array const

[Philippe Mathieu-Daudé]

Reduce a bit the memory footprint by making the helper_opts[]
array const.

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Acked-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 fsdev/virtfs-proxy-helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index aa1ab2590d..de061a8a0e 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -43,7 +43,7 @@
 #define BTRFS_SUPER_MAGIC 0x9123683E
 #endif
 
-static struct option helper_opts[] = {
+static const struct option helper_opts[] = {
     {"fd", required_argument, NULL, 'f'},
     {"path", required_argument, NULL, 'p'},
     {"nodaemon", no_argument, NULL, 'n'},
-- 
2.21.1

Re: [PATCH v2 0/9] hw, ui, virtfs-proxy-helper: Reduce QEMU .data/.rodata/.bss footprint

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200305124525.14555-1-philmd@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
Cloning into 'dtc'...
remote: Counting objects: 5394, done.        
error: RPC failed; result=18, HTTP code = 200
fatal: The remote end hung up unexpectedly
fatal: protocol error: bad pack header
Clone of 'https://git.qemu.org/git/dtc.git' into submodule path 'dtc' failed
failed to update submodule dtc
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) unregistered for path 'dtc'
make[1]: *** [/var/tmp/patchew-tester-tmp-aw2p1w3p/src/docker-src.2020-03-05-08.09.04.27630] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-aw2p1w3p/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    6m3.459s
user    0m2.484s


The full log is available at
http://patchew.org/logs/20200305124525.14555-1-philmd@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2 0/9] hw, ui, virtfs-proxy-helper: Reduce QEMU .data/.rodata/.bss footprint

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200305124525.14555-1-philmd@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
Cloning into 'dtc'...
remote: Counting objects: 5394, done.        
error: RPC failed; result=18, HTTP code = 200
fatal: The remote end hung up unexpectedly
fatal: protocol error: bad pack header
Clone of 'https://git.qemu.org/git/dtc.git' into submodule path 'dtc' failed
failed to update submodule dtc
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) unregistered for path 'dtc'
make[1]: *** [/var/tmp/patchew-tester-tmp-l4s5r6pv/src/docker-src.2020-03-05-08.15.46.28319] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-l4s5r6pv/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    9m17.768s
user    0m2.937s


The full log is available at
http://patchew.org/logs/20200305124525.14555-1-philmd@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2 1/9] hw/audio/fmopl: Fix a typo twice

[Stefano Garzarella]

On Thu, Mar 05, 2020 at 01:45:17PM +0100, Philippe Mathieu-Daudé wrote:
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/audio/fmopl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
> index 9f50a89b4a..173a7521f2 100644
> --- a/hw/audio/fmopl.c
> +++ b/hw/audio/fmopl.c
> @@ -1066,7 +1066,7 @@ static void OPLResetChip(FM_OPL *OPL)
>  	}
>  }
>  
> -/* ----------  Create one of vietual YM3812 ----------       */
> +/* ----------  Create one of virtual YM3812 ----------       */
>  /* 'rate'  is sampling rate and 'bufsiz' is the size of the  */
                               ^
If you need to respin, what do you think about removing also "and 'bufsiz' is
the size of the"?

With or without:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>  FM_OPL *OPLCreate(int clock, int rate)
>  {
> @@ -1115,7 +1115,7 @@ FM_OPL *OPLCreate(int clock, int rate)
>  	return OPL;
>  }
>  
> -/* ----------  Destroy one of vietual YM3812 ----------       */
> +/* ----------  Destroy one of virtual YM3812 ----------       */
>  void OPLDestroy(FM_OPL *OPL)
>  {
>  #ifdef OPL_OUTPUT_LOG
> -- 
> 2.21.1
> 

Re: [PATCH v2 0/9] hw, ui, virtfs-proxy-helper: Reduce QEMU .data/.rodata/.bss footprint

[Daniel P. Berrangé]

On Thu, Mar 05, 2020 at 01:45:16PM +0100, Philippe Mathieu-Daudé wrote:
> Since v1:
> - merged 2 series
> - reworked hw/usb/quirks
> - added R-b/A-b tags
> 
> This series reduce the footprint of the QEMU binary:
> .bss: 106KiB (moved to .heap)

Did this actually have an impact on the binary size, or just on the
size the elf-dissector reports ?  I'm not very familiar with ELF,
but Wikipedia's description of BSS makes me question it...

  "Typically only the length of the bss section, but no data, 
   is stored in the object file. The program loader allocates 
   memory for the bss section when it loads the program. On
   some platforms, some or all of the bss section is initialized
   to zeroes. Unix-like systems and Windows initialize the bss 
   section to zero"

This suggests .bss has no on-disk overhead, only runtime overhead,
which is presumably going to be the same with heap allocations.

> .data: 1MiB
> .rodata: 4.34MiB

These looks useful though in terms of disk footprint.

> (sizes on x86_64 building with -Os)
> 
> The elf-dissector tool [1] [2] helped to notice the big array.
> 
> [1] https://phabricator.kde.org/source/elf-dissector/
> [2] https://www.volkerkrause.eu/2019/06/22/elf-dissector-aarch64-support.html
> [heap equivalent tool working with QEMU: https://github.com/KDE/heaptrack]
> 
> Supersedes: <20200304221807.25212-1-philmd@redhat.com>
> Supersedes: <20200305010446.17029-1-philmd@redhat.com>
> 
> Philippe Mathieu-Daudé (9):
>   hw/audio/fmopl: Fix a typo twice
>   hw/audio/fmopl: Move ENV_CURVE to .heap to save 32KiB of .bss
>   hw/audio/intel-hda: Use memory region alias to reduce .rodata by
>     4.34MB
>   hw/net/e1000: Add readops/writeops typedefs
>   hw/net/e1000: Move macreg[] arrays to .rodata to save 1MiB of .data
>   hw/usb/quirks: Use smaller types to reduce .rodata by 10KiB
>   ui/curses: Make control_characters[] array const
>   ui/curses: Move arrays to .heap to save 74KiB of .bss
>   virtfs-proxy-helper: Make the helper_opts[] array const
> 
>  hw/usb/quirks.h             | 22 +++++++++++++---------
>  fsdev/virtfs-proxy-helper.c |  2 +-
>  hw/audio/fmopl.c            |  8 +++++---
>  hw/audio/intel-hda.c        | 24 ++++++++++--------------
>  hw/net/e1000.c              |  6 ++++--
>  hw/net/e1000e_core.c        |  6 ++++--
>  hw/usb/quirks.c             |  4 ++--
>  ui/curses.c                 | 10 +++++++---
>  8 files changed, 46 insertions(+), 36 deletions(-)
> 
> -- 
> 2.21.1
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 2/9] hw/audio/fmopl: Move ENV_CURVE to .heap to save 32KiB of .bss

[Stefano Garzarella]

On Thu, Mar 05, 2020 at 01:45:18PM +0100, Philippe Mathieu-Daudé wrote:
> This buffer is only used by the adlib audio device. Move it to
> the .heap to release 32KiB of .bss (size reported on x86_64 host).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/audio/fmopl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
> index 173a7521f2..356d4dfbca 100644
> --- a/hw/audio/fmopl.c
> +++ b/hw/audio/fmopl.c
> @@ -186,7 +186,7 @@ static int32_t *VIB_TABLE;
>  
>  /* envelope output curve table */
>  /* attack + decay + OFF */
> -static int32_t ENV_CURVE[2*EG_ENT+1];
> +static int32_t *ENV_CURVE;
>  
>  /* multiple table */
>  #define ML 2
> @@ -1090,6 +1090,7 @@ FM_OPL *OPLCreate(int clock, int rate)
>  	OPL->clock = clock;
>  	OPL->rate  = rate;
>  	OPL->max_ch = max_ch;
> +    ENV_CURVE = g_new(int32_t, 2 * EG_ENT + 1);
>  	/* init grobal tables */
>  	OPL_initialize(OPL);
>  	/* reset chip */
> @@ -1127,6 +1128,7 @@ void OPLDestroy(FM_OPL *OPL)
>  #endif
>  	OPL_UnLockTable();
>  	free(OPL);
> +    g_free(ENV_CURVE);

Just for curiosity, here the entire fmopl.c is indented with tabs.

In this case, is it better to continue with the tabs or use spaces
for new changes?

Anyway the changes LGTM:
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Re: [PATCH v2 7/9] ui/curses: Make control_characters[] array const

[Stefano Garzarella]

On Thu, Mar 05, 2020 at 01:45:23PM +0100, Philippe Mathieu-Daudé wrote:
> As we only use this array as input, make it const.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  ui/curses.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> 
> diff --git a/ui/curses.c b/ui/curses.c
> index 3a1b71451c..3bafc10c1c 100644
> --- a/ui/curses.c
> +++ b/ui/curses.c
> @@ -529,7 +529,7 @@ static void font_setup(void)
>       * Control characters are normally non-printable, but VGA does have
>       * well-known glyphs for them.
>       */
> -    static uint16_t control_characters[0x20] = {
> +    static const uint16_t control_characters[0x20] = {
>        0x0020,
>        0x263a,
>        0x263b,
> -- 
> 2.21.1
> 

Re: [PATCH v2 2/9] hw/audio/fmopl: Move ENV_CURVE to .heap to save 32KiB of .bss

[Stefano Garzarella]

On Thu, Mar 05, 2020 at 02:44:03PM +0100, Stefano Garzarella wrote:
> On Thu, Mar 05, 2020 at 01:45:18PM +0100, Philippe Mathieu-Daudé wrote:
> > This buffer is only used by the adlib audio device. Move it to
> > the .heap to release 32KiB of .bss (size reported on x86_64 host).
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> >  hw/audio/fmopl.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
> > index 173a7521f2..356d4dfbca 100644
> > --- a/hw/audio/fmopl.c
> > +++ b/hw/audio/fmopl.c
> > @@ -186,7 +186,7 @@ static int32_t *VIB_TABLE;
> >  
> >  /* envelope output curve table */
> >  /* attack + decay + OFF */
> > -static int32_t ENV_CURVE[2*EG_ENT+1];
> > +static int32_t *ENV_CURVE;
> >  
> >  /* multiple table */
> >  #define ML 2
> > @@ -1090,6 +1090,7 @@ FM_OPL *OPLCreate(int clock, int rate)
> >  	OPL->clock = clock;
> >  	OPL->rate  = rate;
> >  	OPL->max_ch = max_ch;
> > +    ENV_CURVE = g_new(int32_t, 2 * EG_ENT + 1);

Should we use g_new0() ?

> >  	/* init grobal tables */
> >  	OPL_initialize(OPL);
> >  	/* reset chip */
> > @@ -1127,6 +1128,7 @@ void OPLDestroy(FM_OPL *OPL)
> >  #endif
> >  	OPL_UnLockTable();
> >  	free(OPL);
> > +    g_free(ENV_CURVE);
> 
> Just for curiosity, here the entire fmopl.c is indented with tabs.
> 
> In this case, is it better to continue with the tabs or use spaces
> for new changes?
> 
> Anyway the changes LGTM:
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Re: [PATCH v2 0/9] hw, ui, virtfs-proxy-helper: Reduce QEMU .data/.rodata/.bss footprint

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200305124525.14555-1-philmd@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
Cloning into 'dtc'...
remote: Counting objects: 5394, done.        
error: RPC failed; result=18, HTTP code = 200
fatal: The remote end hung up unexpectedly
fatal: protocol error: bad pack header
Clone of 'https://git.qemu.org/git/dtc.git' into submodule path 'dtc' failed
failed to update submodule dtc
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) unregistered for path 'dtc'
make[1]: *** [/var/tmp/patchew-tester-tmp-uyru360g/src/docker-src.2020-03-05-08.43.13.1580] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-uyru360g/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    5m33.393s
user    0m2.702s


The full log is available at
http://patchew.org/logs/20200305124525.14555-1-philmd@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2 2/9] hw/audio/fmopl: Move ENV_CURVE to .heap to save 32KiB of .bss

[Philippe Mathieu-Daudé]

On 3/5/20 2:44 PM, Stefano Garzarella wrote:
> On Thu, Mar 05, 2020 at 01:45:18PM +0100, Philippe Mathieu-Daudé wrote:
>> This buffer is only used by the adlib audio device. Move it to
>> the .heap to release 32KiB of .bss (size reported on x86_64 host).
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   hw/audio/fmopl.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
>> index 173a7521f2..356d4dfbca 100644
>> --- a/hw/audio/fmopl.c
>> +++ b/hw/audio/fmopl.c
>> @@ -186,7 +186,7 @@ static int32_t *VIB_TABLE;
>>   
>>   /* envelope output curve table */
>>   /* attack + decay + OFF */
>> -static int32_t ENV_CURVE[2*EG_ENT+1];
>> +static int32_t *ENV_CURVE;
>>   
>>   /* multiple table */
>>   #define ML 2
>> @@ -1090,6 +1090,7 @@ FM_OPL *OPLCreate(int clock, int rate)
>>   	OPL->clock = clock;
>>   	OPL->rate  = rate;
>>   	OPL->max_ch = max_ch;
>> +    ENV_CURVE = g_new(int32_t, 2 * EG_ENT + 1);
>>   	/* init grobal tables */
>>   	OPL_initialize(OPL);
>>   	/* reset chip */
>> @@ -1127,6 +1128,7 @@ void OPLDestroy(FM_OPL *OPL)
>>   #endif
>>   	OPL_UnLockTable();
>>   	free(OPL);
>> +    g_free(ENV_CURVE);
> 
> Just for curiosity, here the entire fmopl.c is indented with tabs.
> 
> In this case, is it better to continue with the tabs or use spaces
> for new changes?

checkpatch.pl doesn't allow us to use spaces.

> 
> Anyway the changes LGTM:
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> 

Thanks!

Re: [PATCH v2 2/9] hw/audio/fmopl: Move ENV_CURVE to .heap to save 32KiB of .bss

[Philippe Mathieu-Daudé]

On 3/5/20 2:48 PM, Stefano Garzarella wrote:
> On Thu, Mar 05, 2020 at 02:44:03PM +0100, Stefano Garzarella wrote:
>> On Thu, Mar 05, 2020 at 01:45:18PM +0100, Philippe Mathieu-Daudé wrote:
>>> This buffer is only used by the adlib audio device. Move it to
>>> the .heap to release 32KiB of .bss (size reported on x86_64 host).
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>   hw/audio/fmopl.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
>>> index 173a7521f2..356d4dfbca 100644
>>> --- a/hw/audio/fmopl.c
>>> +++ b/hw/audio/fmopl.c
>>> @@ -186,7 +186,7 @@ static int32_t *VIB_TABLE;
>>>   
>>>   /* envelope output curve table */
>>>   /* attack + decay + OFF */
>>> -static int32_t ENV_CURVE[2*EG_ENT+1];
>>> +static int32_t *ENV_CURVE;
>>>   
>>>   /* multiple table */
>>>   #define ML 2
>>> @@ -1090,6 +1090,7 @@ FM_OPL *OPLCreate(int clock, int rate)
>>>   	OPL->clock = clock;
>>>   	OPL->rate  = rate;
>>>   	OPL->max_ch = max_ch;
>>> +    ENV_CURVE = g_new(int32_t, 2 * EG_ENT + 1);
> 
> Should we use g_new0() ?

No because the array is filled before being used. I can add a note about 
this.

> 
>>>   	/* init grobal tables */
>>>   	OPL_initialize(OPL);
>>>   	/* reset chip */
>>> @@ -1127,6 +1128,7 @@ void OPLDestroy(FM_OPL *OPL)
>>>   #endif
>>>   	OPL_UnLockTable();
>>>   	free(OPL);
>>> +    g_free(ENV_CURVE);
>>
>> Just for curiosity, here the entire fmopl.c is indented with tabs.
>>
>> In this case, is it better to continue with the tabs or use spaces
>> for new changes?
>>
>> Anyway the changes LGTM:
>> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> 

Re: [PATCH v2 2/9] hw/audio/fmopl: Move ENV_CURVE to .heap to save 32KiB of .bss

[Daniel P. Berrangé]

On Thu, Mar 05, 2020 at 02:49:37PM +0100, Philippe Mathieu-Daudé wrote:
> On 3/5/20 2:44 PM, Stefano Garzarella wrote:
> > On Thu, Mar 05, 2020 at 01:45:18PM +0100, Philippe Mathieu-Daudé wrote:
> > > This buffer is only used by the adlib audio device. Move it to
> > > the .heap to release 32KiB of .bss (size reported on x86_64 host).
> > > 
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > ---
> > >   hw/audio/fmopl.c | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
> > > index 173a7521f2..356d4dfbca 100644
> > > --- a/hw/audio/fmopl.c
> > > +++ b/hw/audio/fmopl.c
> > > @@ -186,7 +186,7 @@ static int32_t *VIB_TABLE;
> > >   /* envelope output curve table */
> > >   /* attack + decay + OFF */
> > > -static int32_t ENV_CURVE[2*EG_ENT+1];
> > > +static int32_t *ENV_CURVE;
> > >   /* multiple table */
> > >   #define ML 2
> > > @@ -1090,6 +1090,7 @@ FM_OPL *OPLCreate(int clock, int rate)
> > >   	OPL->clock = clock;
> > >   	OPL->rate  = rate;
> > >   	OPL->max_ch = max_ch;
> > > +    ENV_CURVE = g_new(int32_t, 2 * EG_ENT + 1);
> > >   	/* init grobal tables */
> > >   	OPL_initialize(OPL);
> > >   	/* reset chip */
> > > @@ -1127,6 +1128,7 @@ void OPLDestroy(FM_OPL *OPL)
> > >   #endif
> > >   	OPL_UnLockTable();
> > >   	free(OPL);
> > > +    g_free(ENV_CURVE);
> > 
> > Just for curiosity, here the entire fmopl.c is indented with tabs.
> > 
> > In this case, is it better to continue with the tabs or use spaces
> > for new changes?
> 
> checkpatch.pl doesn't allow us to use spaces.

ITYM  'tabs' here.

IMHO this is a case where the cure is worse than the disease.
In priority order I think we generally rank:

 * Exclusive use of space indent (best)
 * Exclusive use of tab indent (bad)
 * Mixture of space & tab indent (terrible)

IOW, this is a case where I would suggest either:

 - Have a cleanup commit first that eliminates all tabs

or

 - Continue to use tabs consistently & ignore checkpatch

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 0/9] hw, ui, virtfs-proxy-helper: Reduce QEMU .data/.rodata/.bss footprint

[Philippe Mathieu-Daudé]

On 3/5/20 2:42 PM, Daniel P. Berrangé wrote:
> On Thu, Mar 05, 2020 at 01:45:16PM +0100, Philippe Mathieu-Daudé wrote:
>> Since v1:
>> - merged 2 series
>> - reworked hw/usb/quirks
>> - added R-b/A-b tags
>>
>> This series reduce the footprint of the QEMU binary:
>> .bss: 106KiB (moved to .heap)
> 
> Did this actually have an impact on the binary size, or just on the
> size the elf-dissector reports ?  I'm not very familiar with ELF,
> but Wikipedia's description of BSS makes me question it...
> 
>    "Typically only the length of the bss section, but no data,
>     is stored in the object file. The program loader allocates
>     memory for the bss section when it loads the program. On
>     some platforms, some or all of the bss section is initialized
>     to zeroes. Unix-like systems and Windows initialize the bss
>     section to zero"
> 
> This suggests .bss has no on-disk overhead, only runtime overhead,
> which is presumably going to be the same with heap allocations.

IIUC when stored in the .bss, the buffer are always allocated in memory, 
even if not used. By moving them to the .heap, we only allocate them 
when using either the adlib audio device or curses.

> 
>> .data: 1MiB
>> .rodata: 4.34MiB
> 
> These looks useful though in terms of disk footprint.

Memory footprint is more important than disk footprint, but harder to 
track/manage.

> 
>> (sizes on x86_64 building with -Os)
>>
>> The elf-dissector tool [1] [2] helped to notice the big array.
>>
>> [1] https://phabricator.kde.org/source/elf-dissector/
>> [2] https://www.volkerkrause.eu/2019/06/22/elf-dissector-aarch64-support.html
>> [heap equivalent tool working with QEMU: https://github.com/KDE/heaptrack]
>>
>> Supersedes: <20200304221807.25212-1-philmd@redhat.com>
>> Supersedes: <20200305010446.17029-1-philmd@redhat.com>
>>
>> Philippe Mathieu-Daudé (9):
>>    hw/audio/fmopl: Fix a typo twice
>>    hw/audio/fmopl: Move ENV_CURVE to .heap to save 32KiB of .bss
>>    hw/audio/intel-hda: Use memory region alias to reduce .rodata by
>>      4.34MB
>>    hw/net/e1000: Add readops/writeops typedefs
>>    hw/net/e1000: Move macreg[] arrays to .rodata to save 1MiB of .data
>>    hw/usb/quirks: Use smaller types to reduce .rodata by 10KiB
>>    ui/curses: Make control_characters[] array const
>>    ui/curses: Move arrays to .heap to save 74KiB of .bss
>>    virtfs-proxy-helper: Make the helper_opts[] array const
>>
>>   hw/usb/quirks.h             | 22 +++++++++++++---------
>>   fsdev/virtfs-proxy-helper.c |  2 +-
>>   hw/audio/fmopl.c            |  8 +++++---
>>   hw/audio/intel-hda.c        | 24 ++++++++++--------------
>>   hw/net/e1000.c              |  6 ++++--
>>   hw/net/e1000e_core.c        |  6 ++++--
>>   hw/usb/quirks.c             |  4 ++--
>>   ui/curses.c                 | 10 +++++++---
>>   8 files changed, 46 insertions(+), 36 deletions(-)
>>
>> -- 
>> 2.21.1
>>
>>
> 
> Regards,
> Daniel
> 

Re: [PATCH v2 2/9] hw/audio/fmopl: Move ENV_CURVE to .heap to save 32KiB of .bss

[Stefano Garzarella]

On Thu, Mar 05, 2020 at 02:50:47PM +0100, Philippe Mathieu-Daudé wrote:
> On 3/5/20 2:48 PM, Stefano Garzarella wrote:
> > On Thu, Mar 05, 2020 at 02:44:03PM +0100, Stefano Garzarella wrote:
> > > On Thu, Mar 05, 2020 at 01:45:18PM +0100, Philippe Mathieu-Daudé wrote:
> > > > This buffer is only used by the adlib audio device. Move it to
> > > > the .heap to release 32KiB of .bss (size reported on x86_64 host).
> > > > 
> > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > > ---
> > > >   hw/audio/fmopl.c | 4 +++-
> > > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
> > > > index 173a7521f2..356d4dfbca 100644
> > > > --- a/hw/audio/fmopl.c
> > > > +++ b/hw/audio/fmopl.c
> > > > @@ -186,7 +186,7 @@ static int32_t *VIB_TABLE;
> > > >   /* envelope output curve table */
> > > >   /* attack + decay + OFF */
> > > > -static int32_t ENV_CURVE[2*EG_ENT+1];
> > > > +static int32_t *ENV_CURVE;
> > > >   /* multiple table */
> > > >   #define ML 2
> > > > @@ -1090,6 +1090,7 @@ FM_OPL *OPLCreate(int clock, int rate)
> > > >   	OPL->clock = clock;
> > > >   	OPL->rate  = rate;
> > > >   	OPL->max_ch = max_ch;
> > > > +    ENV_CURVE = g_new(int32_t, 2 * EG_ENT + 1);
> > 
> > Should we use g_new0() ?
> 
> No because the array is filled before being used. I can add a note about
> this.
> 

Thanks for the clarification!
Yes, if you need to respin, maybe it's better.

Thanks,
Stefano

Re: [PATCH v2 0/9] hw, ui, virtfs-proxy-helper: Reduce QEMU .data/.rodata/.bss footprint

[Eric Blake]

On 3/5/20 6:45 AM, Philippe Mathieu-Daudé wrote:
> Since v1:
> - merged 2 series
> - reworked hw/usb/quirks
> - added R-b/A-b tags
> 
> This series reduce the footprint of the QEMU binary:
> .bss: 106KiB (moved to .heap)

Why is moving stuff from .bss to .heap beneficial?  With .bss, 
0-initialization is cheap (the OS gives it to us for free by mapping the 
entire .bss to a copy-on-write zero page); but with .heap, we generally 
have to zero it at runtime ourselves.

> .data: 1MiB
> .rodata: 4.34MiB

But reducing these sizes makes sense.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 0/9] hw, ui, virtfs-proxy-helper: Reduce QEMU .data/.rodata/.bss footprint

[Eric Blake]

On 3/5/20 7:42 AM, Daniel P. Berrangé wrote:
> On Thu, Mar 05, 2020 at 01:45:16PM +0100, Philippe Mathieu-Daudé wrote:
>> Since v1:
>> - merged 2 series
>> - reworked hw/usb/quirks
>> - added R-b/A-b tags
>>
>> This series reduce the footprint of the QEMU binary:
>> .bss: 106KiB (moved to .heap)
> 
> Did this actually have an impact on the binary size, or just on the
> size the elf-dissector reports ?  I'm not very familiar with ELF,
> but Wikipedia's description of BSS makes me question it...
> 
>    "Typically only the length of the bss section, but no data,
>     is stored in the object file. The program loader allocates
>     memory for the bss section when it loads the program. On
>     some platforms, some or all of the bss section is initialized
>     to zeroes. Unix-like systems and Windows initialize the bss
>     section to zero"
> 
> This suggests .bss has no on-disk overhead, only runtime overhead,
> which is presumably going to be the same with heap allocations.

Or even LESS overhead.  Heap allocationhave unspecified contents 
requiring runtime effort (true, some implementations of malloc() handle 
large allocations specially as anonymous mmap initially backed by 
/dev/zero, so that those allocations start life 0-allocated, but you 
can't rely on that optimization), while .bss is required by the C 
language to be 0 initialized (and you CAN rely on the OS implementing 
that as efficiently as possible, generally by starting with COW mapping 
initially backed by the zero page).

In fact, on nbdkit, we hit an interesting case where using .bss instead 
of .rodata is actually beneficial:
https://www.redhat.com/archives/libguestfs/2019-July/msg00074.html
Marking a large array that will always consist only of zero bytes as 
const actually pessimized the image size and load time, because the 
addition of const moved the array from .bss into .rodata.

> 
>> .data: 1MiB
>> .rodata: 4.34MiB
> 
> These looks useful though in terms of disk footprint.

Yes. Smaller data structures allow for smaller binaries and faster loading.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 0/9] hw, ui, virtfs-proxy-helper: Reduce QEMU .data/.rodata/.bss footprint

[Eric Blake]

On 3/5/20 7:56 AM, Philippe Mathieu-Daudé wrote:
> On 3/5/20 2:42 PM, Daniel P. Berrangé wrote:
>> On Thu, Mar 05, 2020 at 01:45:16PM +0100, Philippe Mathieu-Daudé wrote:
>>> Since v1:
>>> - merged 2 series
>>> - reworked hw/usb/quirks
>>> - added R-b/A-b tags
>>>
>>> This series reduce the footprint of the QEMU binary:
>>> .bss: 106KiB (moved to .heap)
>>
>> Did this actually have an impact on the binary size, or just on the
>> size the elf-dissector reports ?  I'm not very familiar with ELF,
>> but Wikipedia's description of BSS makes me question it...
>>
>>    "Typically only the length of the bss section, but no data,
>>     is stored in the object file. The program loader allocates
>>     memory for the bss section when it loads the program. On
>>     some platforms, some or all of the bss section is initialized
>>     to zeroes. Unix-like systems and Windows initialize the bss
>>     section to zero"
>>
>> This suggests .bss has no on-disk overhead, only runtime overhead,
>> which is presumably going to be the same with heap allocations.
> 
> IIUC when stored in the .bss, the buffer are always allocated in memory, 
> even if not used. By moving them to the .heap, we only allocate them 
> when using either the adlib audio device or curses.

Virtual memory is cheap on 64-bit platforms.  Just because the address 
range is reserved does not actually change the amount of memory in use 
by the machine, if the application does not touch those virtual 
addresses.  But you do have a potential point on a 32-bit platform, 
where a heap allocation only when needed may avoid address space 
exhaustion for other cases.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 1/9] hw/audio/fmopl: Fix a typo twice

[Laurent Vivier]

Le 05/03/2020 à 13:45, Philippe Mathieu-Daudé a écrit :
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/audio/fmopl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/audio/fmopl.c b/hw/audio/fmopl.c
> index 9f50a89b4a..173a7521f2 100644
> --- a/hw/audio/fmopl.c
> +++ b/hw/audio/fmopl.c
> @@ -1066,7 +1066,7 @@ static void OPLResetChip(FM_OPL *OPL)
>  	}
>  }
>  
> -/* ----------  Create one of vietual YM3812 ----------       */
> +/* ----------  Create one of virtual YM3812 ----------       */
>  /* 'rate'  is sampling rate and 'bufsiz' is the size of the  */
>  FM_OPL *OPLCreate(int clock, int rate)
>  {
> @@ -1115,7 +1115,7 @@ FM_OPL *OPLCreate(int clock, int rate)
>  	return OPL;
>  }
>  
> -/* ----------  Destroy one of vietual YM3812 ----------       */
> +/* ----------  Destroy one of virtual YM3812 ----------       */
>  void OPLDestroy(FM_OPL *OPL)
>  {
>  #ifdef OPL_OUTPUT_LOG
> 

Applied to my trivial-patches branch.

Thanks,
Laurent