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

Tue May 10 09:17:04 CEST 2016

Hi Bin,

2016-05-10 13:12 GMT+08:00 Bin Meng <bmeng.cn at gmail.com>:
> 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

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.

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

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

>
> Maybe you can send out some patches to address these?

Looks like Tom has plans for this ?

Thanks,
Miao

>
> Regards,
> Bin