[PATCH 0/6] hw,ui: Reduce QEMU .rodata/.bss footprint

[PATCH 0/6] hw,ui: Reduce QEMU .rodata/.bss footprint

[Philippe Mathieu-Daudé]

This series reduce the footprint of the QEMU binary:
.bss: 106KiB (moved to .heap)
.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

Philippe Mathieu-Daudé (6):
  hw/audio/fmopl: Fix a typo twice
  hw/audio/fmopl: Move ENV_CURVE to .heap to save 32KiB of .bss
  hw/usb/quirks: Use smaller types to reduce .rodata by 10KiB
  hw/audio/intel-hda: Use memory region alias to reduce .rodata by
    4.34MB
  ui/curses: Make control_characters[] array const
  ui/curses: Move arrays to .heap to save 74KiB of .bss

 hw/usb/quirks.h      | 10 +++++-----
 hw/audio/fmopl.c     |  8 +++++---
 hw/audio/intel-hda.c | 24 ++++++++++--------------
 ui/curses.c          | 10 +++++++---
 4 files changed, 27 insertions(+), 25 deletions(-)

-- 
2.21.1

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

[Philippe Mathieu-Daudé]

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

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

[Laurent Vivier]

Le 04/03/2020 à 23:18, Philippe Mathieu-Daudé a écrit :
> 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
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

[PATCH 2/6] 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 3/6] 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>
---
 hw/usb/quirks.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/usb/quirks.h b/hw/usb/quirks.h
index 89480befd7..794d89a356 100644
--- a/hw/usb/quirks.h
+++ b/hw/usb/quirks.h
@@ -21,11 +21,11 @@
 #include "quirks-pl2303-ids.h"
 
 struct usb_device_id {
-    int vendor_id;
-    int product_id;
-    int interface_class;
-    int interface_subclass;
-    int interface_protocol;
+    int16_t vendor_id;
+    int16_t product_id;
+    int8_t interface_class;
+    int8_t interface_subclass;
+    int8_t interface_protocol;
 };
 
 #define USB_DEVICE(vendor, product) \
-- 
2.21.1

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

[Gerd Hoffmann]

>  struct usb_device_id {
> -    int vendor_id;
> -    int product_id;
> -    int interface_class;
> -    int interface_subclass;
> -    int interface_protocol;
> +    int16_t vendor_id;
> +    int16_t product_id;
> +    int8_t interface_class;
> +    int8_t interface_subclass;
> +    int8_t interface_protocol;

I guess we should better use the uint variants here ...

cheers,
  Gerd

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

[Philippe Mathieu-Daudé]

On 3/5/20 9:02 AM, Gerd Hoffmann wrote:
>>   struct usb_device_id {
>> -    int vendor_id;
>> -    int product_id;
>> -    int interface_class;
>> -    int interface_subclass;
>> -    int interface_protocol;
>> +    int16_t vendor_id;
>> +    int16_t product_id;
>> +    int8_t interface_class;
>> +    int8_t interface_subclass;
>> +    int8_t interface_protocol;
> 
> I guess we should better use the uint variants here ...

I tried it first but I got:

   CC      hw/usb/quirks.o
hw/usb/quirks.c:25:34: error: result of comparison of constant -1 with 
expression of type 'const uint16_t' (aka 'const unsigned short') is 
always true [-Werror,-Wtautological-constant-out-of-range-compare]
     for (i = 0; ids[i].vendor_id != -1; i++) {
                 ~~~~~~~~~~~~~~~~ ^  ~~
hw/usb/quirks.c:28:37: error: result of comparison of constant -1 with 
expression of type 'const uint8_t' (aka 'const unsigned char') is always 
false [-Werror,-Wtautological-constant-out-of-range-compare]
             (ids[i].interface_class == -1 ||
              ~~~~~~~~~~~~~~~~~~~~~~ ^  ~~

And went this less intrusive way.

I'll respin with s/-1/UINT8_MAX/.

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

[Philippe Mathieu-Daudé]

On Thu, Mar 5, 2020 at 11:41 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
> On 3/5/20 9:02 AM, Gerd Hoffmann wrote:
> >>   struct usb_device_id {
> >> -    int vendor_id;
> >> -    int product_id;
> >> -    int interface_class;
> >> -    int interface_subclass;
> >> -    int interface_protocol;
> >> +    int16_t vendor_id;
> >> +    int16_t product_id;
> >> +    int8_t interface_class;
> >> +    int8_t interface_subclass;
> >> +    int8_t interface_protocol;
> >
> > I guess we should better use the uint variants here ...
>
> I tried it first but I got:
>
>    CC      hw/usb/quirks.o
> hw/usb/quirks.c:25:34: error: result of comparison of constant -1 with
> expression of type 'const uint16_t' (aka 'const unsigned short') is
> always true [-Werror,-Wtautological-constant-out-of-range-compare]
>      for (i = 0; ids[i].vendor_id != -1; i++) {
>                  ~~~~~~~~~~~~~~~~ ^  ~~
> hw/usb/quirks.c:28:37: error: result of comparison of constant -1 with
> expression of type 'const uint8_t' (aka 'const unsigned char') is always
> false [-Werror,-Wtautological-constant-out-of-range-compare]
>              (ids[i].interface_class == -1 ||
>               ~~~~~~~~~~~~~~~~~~~~~~ ^  ~~
>
> And went this less intrusive way.
>
> I'll respin with s/-1/UINT8_MAX/.

Problem, now this entry is ignored (interface_class==-1):

    { USB_DEVICE_AND_INTERFACE_INFO(MICROCHIP_VID, MICROCHIP_USB_BOARD_PID,
                                    0xff, 0xff, 0x00) },

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

[Gerd Hoffmann]

> > And went this less intrusive way.
> >
> > I'll respin with s/-1/UINT8_MAX/.
> 
> Problem, now this entry is ignored (interface_class==-1):

Yep, "-1" is used as "not used" or "end-of-list" indicator, and it is
outside the valid range to avoid that kind of clashes.  You need some
other way to express that if you want go with smaller types which don't
allow values outside the valid range any more.  Add a "flags" field for
that maybe?

cheers,
  Gerd

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

[Philippe Mathieu-Daudé]

On 3/5/20 12:59 PM, Gerd Hoffmann wrote:
>>> And went this less intrusive way.
>>>
>>> I'll respin with s/-1/UINT8_MAX/.
>>
>> Problem, now this entry is ignored (interface_class==-1):
> 
> Yep, "-1" is used as "not used" or "end-of-list" indicator, and it is
> outside the valid range to avoid that kind of clashes.  You need some
> other way to express that if you want go with smaller types which don't
> allow values outside the valid range any more.  Add a "flags" field for
> that maybe?

Oh I wasn't expecting 0xffff valid for idVendor /
idProduct, neither 0xff for bInterfaceClass / bInterfaceSubClass /
bInterfaceProtocol. Well maybe it doesn't worth the churn for 10KB of 
.rodata.

[PATCH 4/6] 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 5/6] 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 6/6] 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