Re: [Qemu-devel] [PATCH v2] register reset handler to write image into memory

[Alexander Graf]

On 26.08.2012, at 20:50, Yin Olivia-R63875 <address@hidden> wrote:

> Thanks to Dunrong and Andreas.
> 
> $ scripts/get_maintainer.pl -f hw/loader.c
> Alexander Graf <address@hidden> (commit_signer:3/6=50%)
> Anthony Liguori <address@hidden> (commit_signer:2/6=33%)
> Stefan Weil <address@hidden> (commit_signer:1/6=17%)
> Benjamin Herrenschmidt <address@hidden> (commit_signer:1/6=17%)
> Avi Kivity <address@hidden> (commit_signer:1/6=17%)
> 
> Dear maintainers,
> Could you please help review this patch?
> 
> So far I got feedback from Andreas and try to answer the question.
> 
>> This patch does not answer the question why you try to avoid the ROM blobs
>> and what ROM blobs are still being used for after your patch. I don't 
>> think it makes much sense to work around them for your use cases and to 
>> leave them behind - if there's something fundamentally wrong with them 
>> they should be ripped out completely or fixed. But maybe I'm 
>> misunderstanding 
>> in the absence of explanations?
> 
> It's a general problem. 
> 
> For example, in my case, there're 3 different files loaded from host rootfs.
> $ qemu-system-ppc -enable-kvm -m 256 -nographic -M mpc8544ds -kernel 
> uImage.8572.agraf -initrd /media/ram/guest-8572.rootfs.ext2.gz -append 
> "root=/dev/ram rw loglevel=7 console=ttyS0,115200" -serial tcp::4445,server 
> -net nic
> 
> (qemu) info roms
> addr=0000000000000000 size=0x782840 mem=ram name="uImage.8572.agraf"
> addr=0000000000c00000 size=0x010000 mem=ram name="mpc8544ds.dtb"
> addr=0000000002000000 size=0x3f922f mem=ram 
> name="/media/ram/guest-8572.rootfs.ext2.gz"
> 
> The problem is that rom_add_*() mallocs memory for the image, and then 
> rom_reset()
> copies those images into the guest's memory, but the QEMU memory does not get 
> freed.
> On a VM reset, the images get recopied from QEMU to guest.
> 
> Comparing the memory map of qemu process before and after starting up guest,
> we can find that QEMU consumes much memory for those images.
> 
> $ diff -urN pmap.pre.log pmap.post.log
> --- pmap.pre.log
> +++ pmap.post.log
> @@ -33,7 +33,14 @@
> 0ffee000      8K rwx--  /lib/ld-2.13.so
> 10000000   3472K r-x--  qemu-system-ppc
> 10374000    112K rwx--  qemu-system-ppc
> -10390000   6524K rwx--    [ anon ]
> +10390000   7100K rwx--    [ anon ]
> 48002000     16K rw---    [ anon ]
> +48006000      4K -----    [ anon ]
> +48007000   8188K rw---    [ anon ]
> +48806000      8K rw-s-    [ anon ]
> +48808000      4K rw---    [ anon ]
> +48809000 262144K rw---    [ anon ]
> +58809000   5280K rw---    [ anon ]
> +5cb98000   7692K rw---    [ anon ]
> bf93e000    132K rw---    [ stack ]
> - total    14456K
> + total   298352K
> 
> Exactly we can re-load them from disk on a reset instead of holding onto the 
> images in QEMU's memory.
> 
> With this patch, the two big images (uImage and especially initrd) will not 
> be loaded into QEMU's memory
> (qemu) info roms
> addr=0000000000c00000 size=0x010000 mem=ram name="mpc8544ds.dtb"
> 
> It will save much memory space according to memory map of QEMU process.
> # diff -urN pmap.pre.log pmap.post.log
> --- pmap.pre.log
> +++ pmap.post.log
> @@ -33,7 +33,14 @@
> 0ffee000      8K rwx--  /lib/ld-2.13.so
> 10000000   3472K r-x--  qemu-system-ppc
> 10374000    112K rwx--  qemu-system-ppc
> -10390000   6524K rwx--    [ anon ]
> +10390000   7036K rwx--    [ anon ]
> 48002000     16K rw---    [ anon ]
> +48006000      4K -----    [ anon ]
> +48007000   8188K rw---    [ anon ]
> +48806000      8K rw-s-    [ anon ]
> +48808000      4K rw---    [ anon ]
> +48809000 262144K rw---    [ anon ]
> +58809000      4K rw---    [ anon ]
> +58c04000   1204K rw---    [ anon ]
> bfb2a000    132K rw---    [ stack ]
> - total    14456K
> + total   286524K
> 
> This patch changes all the image load process called by load_uimage() and 
> load_image_targphys() in platform initialization.

This doesn't explain why you leave the old in-RAM code alive though. The only 
reason I can imagine would be to allow for reset to not reload new roms after 
an update.

Anthony, any opinion here? Do we need the keep-in-RAM rom code? Or could we 
just always load rom blobs on demand for everything?


Alex

> 
> Best Regards,
> Olivia
> 
>> -----Original Message-----
>> From: Dunrong Huang [mailto:address@hidden
>> Sent: Thursday, August 23, 2012 6:44 PM
>> To: Yin Olivia-R63875
>> Cc: address@hidden; address@hidden
>> Subject: Re: [Qemu-devel] [PATCH v2] register reset handler to write
>> image into memory
>> 
>> 2012/8/23 Yin Olivia-R63875 <address@hidden>:
>>> Dear All,
>>> 
>>> I can't find MAINTAINER of hw/loader.c.
>>> Who can help review and apply this patch?
>>> 
>> Please use the script scripts/get_maintainer.pl, like:
>> $ scripts/get_maintainer.pl your_patch_file.patch or
>> $ scripts/get_maintainer.pl -f hw/loader.c
>>> Best Regards,
>>> Olivia Yin
>>> 
>> 
>> 
>> --
>> Best Regards,
>> 
>> Dunrong Huang
> 
>