On Tue, May 10, 2016 at 03:17:04PM +0800, Miao Yan wrote:
Hi Bin,
2016-05-10 13:12 GMT+08:00 Bin Meng bmeng.cn@gmail.com:
Hi Miao,
On Tue, May 10, 2016 at 12:35 PM, Miao Yan yanmiaobest@gmail.com wrote:
2016-05-10 11:08 GMT+08:00 Bin Meng bmeng.cn@gmail.com:
On Tue, May 10, 2016 at 10:17 AM, Tom Rini trini@konsulko.com wrote:
On Tue, May 10, 2016 at 09:20:45AM +0800, Bin Meng wrote:
On Fri, May 6, 2016 at 10:40 PM, Tom Rini trini@konsulko.com wrote: > - Move the command portion of arch/x86/cpu/qemu/fw_cfg.c into > cmd/qemu_fw_cfg.c > - Move arch/x86/include/asm/fw_cfg.h to include/qemu_fw_cfg.h > - Rename ACPI table portion to arch/x86/cpu/qemu/acpi_table.c > > Signed-off-by: Tom Rini trini@konsulko.com > --- > Changes in v2: > - Depend on X86 (per Miao Yan) > --- > arch/x86/cpu/mp_init.c | 2 +- > arch/x86/cpu/qemu/Makefile | 3 +- > arch/x86/cpu/qemu/acpi_table.c | 243 ++++++++++++++++++ > arch/x86/cpu/qemu/cpu.c | 2 +- > arch/x86/cpu/qemu/fw_cfg.c | 570 ----------------------------------------- > arch/x86/cpu/qemu/qemu.c | 2 +- > arch/x86/include/asm/fw_cfg.h | 157 ------------ > arch/x86/lib/acpi_table.c | 2 +- > cmd/Kconfig | 7 + > cmd/Makefile | 1 + > cmd/qemu_fw_cfg.c | 343 +++++++++++++++++++++++++ > configs/qemu-x86_defconfig | 1 + > include/qemu_fw_cfg.h | 162 ++++++++++++ > 13 files changed, 763 insertions(+), 732 deletions(-) > create mode 100644 arch/x86/cpu/qemu/acpi_table.c > delete mode 100644 arch/x86/cpu/qemu/fw_cfg.c > delete mode 100644 arch/x86/include/asm/fw_cfg.h > create mode 100644 cmd/qemu_fw_cfg.c > create mode 100644 include/qemu_fw_cfg.h >
Looks good.
Reviewed-by: Bin Meng bmeng.cn@gmail.com
Tom, will you pick this for this release, or next release?
Miao has a patch [1] to remove CONFIG_QEMU_ACPI_TABLE. If your patch comes first, Miao needs to rebase his and submit v2.
For the next release, and I'll leave it to you to pull in. Thanks!
applied to u-boot-x86/next, thanks!
Wait, you applied this already ? Did you include the diff I mentioned
The patch is not in mainline. It's currently in x86 tree and we can always fix issues before a pull request is sent out.
? This patch has build issues. Before the patch, the qfw is built unconditionally for x86-qemu, after applying this patch, qfw depends on CONFIG_CMD_QEMU_FW_CFG. This is a change of behavior, so you need to test:
1) defconfig build 2) defconfig with CONFIG_CMD_QEMU_FW_CFG disabled because it'suser visible now
This patch breaks 2):
I doubt everything can be build error free if we randomly switch something on or off from the Kconfig GUI. I did buildman testing for
Well I don't think it's "random", we have mature tools to deal with dependencies, it's not like I was directly modifying random stuff in .config with a text editor. Kconfig options should be there for a reason, otherwise why bother. And if it doesn't work then it's a bug.
So yes, I suppose QEMU should select this new option since it is required there for other non-selectable options, rather than just being enabled in the defconfigs (which is why this is build clean from my point of view, qemu-x86_defconfig was updated).
this patch and nothing breaks by default. Having said that, I agree the build error you reported is something we should fix.
OK. So can you please apply the diff or ask for v3 if you like ?
arch/x86/cpu/built-in.o: In function `cpu_qemu_get_count': /home/myan/work/u-boot/arch/x86/cpu/qemu/cpu.c:28: undefined reference to `qemu_fwcfg_online_cpus' arch/x86/cpu/built-in.o: In function `qemu_chipset_init': /home/myan/work/u-boot/arch/x86/cpu/qemu/qemu.c:91: undefined reference to `qemu_fwcfg_init' arch/x86/cpu/built-in.o: In function `write_acpi_tables': /home/myan/work/u-boot/arch/x86/cpu/qemu/acpi_table.c:187: undefined reference to `qemu_fwcfg_read_firmware_list' /home/myan/work/u-boot/arch/x86/cpu/qemu/acpi_table.c:193: undefined reference to `qemu_fwcfg_find_file' /home/myan/work/u-boot/arch/x86/cpu/qemu/acpi_table.c:211: undefined reference to `qemu_fwcfg_read_entry' arch/x86/cpu/built-in.o: In function `bios_linker_allocate': /home/myan/work/u-boot/arch/x86/cpu/qemu/acpi_table.c:41: undefined reference to `qemu_fwcfg_find_file' /home/myan/work/u-boot/arch/x86/cpu/qemu/acpi_table.c:71: undefined reference to `qemu_fwcfg_read_entry' arch/x86/cpu/built-in.o: In function `bios_linker_add_pointer': /home/myan/work/u-boot/arch/x86/cpu/qemu/acpi_table.c:96: undefined reference to `qemu_fwcfg_find_file' /home/myan/work/u-boot/arch/x86/cpu/qemu/acpi_table.c:99: undefined reference to `qemu_fwcfg_find_file' arch/x86/cpu/built-in.o: In function `bios_linker_add_checksum': /home/myan/work/u-boot/arch/x86/cpu/qemu/acpi_table.c:129: undefined reference to `qemu_fwcfg_find_file' arch/x86/cpu/built-in.o: In function `write_acpi_tables': /home/myan/work/u-boot/arch/x86/cpu/qemu/acpi_table.c:239: undefined reference to `qemu_fwcfg_free_files' arch/x86/cpu/built-in.o: In function `qemu_cpu_fixup': /home/myan/work/u-boot/arch/x86/cpu/mp_init.c:454: undefined reference to `qemu_fwcfg_online_cpus' make: *** [u-boot] Error 1
And I still think for this patch, it should depend on x86 && qemu. It doesn't make sense to build qfw for other non-qemu boards.
Tom has the following statement regarding adding qemu dependency to the Kconfig option:
"In general, I want to see the least restrictive set of depends used. This should be "easy" to throw into sandbox for testing there and when we use normal boards under qemu this should also be easy to enable."
I believe Tom wanted to enable this command build testing for Sanbox, hence increase our build coverage. I think more refactoring work needs
Maybe some kind of random/allyes/allno config should be in that build coverage test.
Yes, my long term goal is that we can make use of random/allyes/allno for real, along with making the sandbox config extremely full. Which is partly why I want lose depends lines
to done to achieve that. Like you said, now QEMU's ACPI table generation depends on CONFIG_CMD_QEMU_FW_CFG. This looks to me a little bit odd. We may further split the qemu_fw_cfg.c to two parts: one only does the command line handling, and the other one does the fw_cfg stuff.
Agreed. I just thought these will be automatically sorted out when we extend qfw to other architectures.
Well, this patch was a first pass at trying to separate out the logic. My end goal is to be able to use -kernel / -initrd / -dtb to pass in files "directly" to say vexpress_ca9x4 rather than have to fiddle with fake networking. So we need to keep that in mind when splitting the code into cmd/ common/ and arch/x86/ (I'll set aside ACPI tables on ARM in QEMU for the moment, but lets also not forget that will be something someone will talk about in the future).
Maybe you can send out some patches to address these?
Looks like Tom has plans for this ?
Well, what you posted before addresses things well enough to start with, when I have time to cycle back and make use of this on either ARM or PowerPC or MIPS (and sandbox) I'll re-tweak the Kconfig logic again as needed. So lets fold that in for now, thanks!