[U-Boot] [PATCH v2] x86: qemu: Move qfw command over to cmd and add Kconfig entry

[Bin Meng]

Hi Miao,

On Tue, May 10, 2016 at 12:35 PM, Miao Yan <yanmiaobest at gmail.com> wrote:
> 2016-05-10 11:08 GMT+08:00 Bin Meng <bmeng.cn at gmail.com>:
>> On Tue, May 10, 2016 at 10:17 AM, Tom Rini <trini at 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 at 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 at 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 at 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's
> user 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
this patch and nothing breaks by default. Having said that, I agree
the build error you reported is something we should fix.

>
> 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
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.

Maybe you can send out some patches to address these?

Regards,
Bin