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

Tue May 10 16:24:40 CEST 2016

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

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160510/4d14f75c/attachment.sig>