[U-Boot,v4,1/8] x86: qemu: add fw_cfg support

The QEMU fw_cfg interface allows the guest to retrieve various
data information from QEMU. For example, APCI/SMBios tables, number
of online cpus, kernel data and command line, etc.

This patch adds support for QEMU fw_cfg interface.

Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
---
Changes in v4:
  - cleanups
  - change 'fw load' to take second parameter for initrd load address

 arch/x86/cpu/qemu/Makefile |   2 +-
 arch/x86/cpu/qemu/fw_cfg.c | 268 +++++++++++++++++++++++++++++++++++++++++++++
 arch/x86/cpu/qemu/fw_cfg.h |  97 ++++++++++++++++
 arch/x86/cpu/qemu/qemu.c   |   3 +
 4 files changed, 369 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/cpu/qemu/fw_cfg.c
 create mode 100644 arch/x86/cpu/qemu/fw_cfg.h

Hi Miao,

On 30 December 2015 at 19:55, Miao Yan <yanmiaobest@gmail.com> wrote:
> The QEMU fw_cfg interface allows the guest to retrieve various
> data information from QEMU. For example, APCI/SMBios tables, number
> of online cpus, kernel data and command line, etc.
>
> This patch adds support for QEMU fw_cfg interface.
>
> Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
> ---
> Changes in v4:
>   - cleanups
>   - change 'fw load' to take second parameter for initrd load address
>
>  arch/x86/cpu/qemu/Makefile |   2 +-
>  arch/x86/cpu/qemu/fw_cfg.c | 268 +++++++++++++++++++++++++++++++++++++++++++++
>  arch/x86/cpu/qemu/fw_cfg.h |  97 ++++++++++++++++
>  arch/x86/cpu/qemu/qemu.c   |   3 +
>  4 files changed, 369 insertions(+), 1 deletion(-)
>  create mode 100644 arch/x86/cpu/qemu/fw_cfg.c
>  create mode 100644 arch/x86/cpu/qemu/fw_cfg.h

Reviewed-by: Simon Glass <sjg@chromium.org>

But a few nits...

>
> diff --git a/arch/x86/cpu/qemu/Makefile b/arch/x86/cpu/qemu/Makefile
> index 3f3958a..ad424ec 100644
> --- a/arch/x86/cpu/qemu/Makefile
> +++ b/arch/x86/cpu/qemu/Makefile
> @@ -7,5 +7,5 @@
>  ifndef CONFIG_EFI_STUB
>  obj-y += car.o dram.o
>  endif
> -obj-y += qemu.o
> +obj-y += qemu.o fw_cfg.o

Can you put the new file first, so that the list is in alphabetical order?

>  obj-$(CONFIG_GENERATE_ACPI_TABLE) += acpi.o dsdt.o
> diff --git a/arch/x86/cpu/qemu/fw_cfg.c b/arch/x86/cpu/qemu/fw_cfg.c
> new file mode 100644
> index 0000000..9de8680
> --- /dev/null
> +++ b/arch/x86/cpu/qemu/fw_cfg.c
> @@ -0,0 +1,268 @@
> +/*
> + * (C) Copyright 2015 Miao Yan <yanmiaoebst@gmail.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <errno.h>
> +#include <malloc.h>
> +#include <asm/io.h>
> +#include "fw_cfg.h"
> +
> +static bool fwcfg_present;
> +static bool fwcfg_dma_present;
> +
> +static void qemu_fwcfg_read_entry_pio(uint16_t entry,
> +               uint32_t size, void *address)

Function comment - what does this do?

> +{
> +       uint32_t i = 0;
> +       uint8_t *data = address;
> +
> +       /*
> +        * writting FW_CFG_INVALID will cause
> +        * read operation to resume at
> +        * last offset, otherwise read will
> +        * start at offset 0

Please can you format comments to 75 columns or thereabout?

> +        */
> +
> +       if (entry != FW_CFG_INVALID)
> +               outw(entry, FW_CONTROL_PORT);
> +       while (size--)
> +               data[i++] = inb(FW_DATA_PORT);
> +}
> +
> +static void qemu_fwcfg_read_entry_dma(uint16_t entry,
> +               uint32_t size, void *address)
> +{
> +       struct fw_cfg_dma_access dma;
> +
> +       dma.length = cpu_to_be32(size);
> +       dma.address = cpu_to_be64((uintptr_t)address);
> +       dma.control = cpu_to_be32(FW_CFG_DMA_READ);
> +
> +       /*
> +        * writting FW_CFG_INVALID will cause
> +        * read operation to resume at
> +        * last offset, otherwise read will
> +        * start at offset 0
> +        */
> +
> +       if (entry != FW_CFG_INVALID)
> +               dma.control |= cpu_to_be32(FW_CFG_DMA_SELECT | (entry << 16));
> +
> +       barrier();
> +
> +       debug("qemu_fwcfg_dma_read_entry: addr %p, length %u control 0x%x\n",
> +             address, size, be32_to_cpu(dma.control));
> +
> +       outl(cpu_to_be32((uint32_t)&dma), FW_DMA_PORT_HIGH);
> +
> +       while (be32_to_cpu(dma.control) & ~FW_CFG_DMA_ERROR)
> +               __asm__ __volatile__ ("pause");
> +}
> +
> +static bool qemu_fwcfg_present(void)
> +{
> +       uint32_t qemu;
> +
> +       qemu_fwcfg_read_entry_pio(FW_CFG_SIGNATURE, 4, &qemu);
> +       return be32_to_cpu(qemu) == QEMU_FW_CFG_SIGNATURE;
> +}
> +
> +static bool qemu_fwcfg_dma_present(void)
> +{
> +       uint8_t dma_enabled;
> +
> +       qemu_fwcfg_read_entry_pio(FW_CFG_ID, 1, &dma_enabled);
> +       if (dma_enabled & FW_CFG_DMA_ENABLED)
> +               return true;
> +
> +       return false;
> +}
> +
> +static void qemu_fwcfg_read_entry(uint16_t entry,
> +               uint32_t length, void *address)
> +{
> +       if (fwcfg_dma_present)
> +               qemu_fwcfg_read_entry_dma(entry, length, address);
> +       else
> +               qemu_fwcfg_read_entry_pio(entry, length, address);
> +}
> +
> +int qemu_fwcfg_online_cpus(void)
> +{
> +       uint16_t nb_cpus;
> +
> +       if (!fwcfg_present)
> +               return 1;

-ENODEV

> +
> +       qemu_fwcfg_read_entry(FW_CFG_NB_CPUS, 2, &nb_cpus);
> +
> +       return le16_to_cpu(nb_cpus);
> +}
> +
> +static int qemu_fwcfg_setup_kernel(void *load_addr, void *initrd_addr)

Function comment

> +{
> +       char *data_addr;
> +       uint32_t setup_size, kernel_size, cmdline_size, initrd_size;
> +
> +       qemu_fwcfg_read_entry(FW_CFG_SETUP_SIZE, 4, &setup_size);
> +       qemu_fwcfg_read_entry(FW_CFG_KERNEL_SIZE, 4, &kernel_size);
> +
> +       if (setup_size == 0 || kernel_size == 0) {
> +               printf("warning: no kernel available\n");
> +               return -1;
> +       }
> +
> +       data_addr = load_addr;
> +       qemu_fwcfg_read_entry(FW_CFG_SETUP_DATA,
> +                             le32_to_cpu(setup_size), data_addr);
> +       data_addr += le32_to_cpu(setup_size);
> +
> +       qemu_fwcfg_read_entry(FW_CFG_KERNEL_DATA,
> +                             le32_to_cpu(kernel_size), data_addr);
> +       data_addr += le32_to_cpu(kernel_size);
> +
> +       data_addr = initrd_addr;
> +       qemu_fwcfg_read_entry(FW_CFG_INITRD_SIZE, 4, &initrd_size);
> +       if (initrd_size == 0) {
> +               printf("warning: no initrd available\n");
> +       } else {
> +               qemu_fwcfg_read_entry(FW_CFG_INITRD_DATA,
> +                                     le32_to_cpu(initrd_size), data_addr);
> +               data_addr += le32_to_cpu(initrd_size);
> +       }
> +
> +       qemu_fwcfg_read_entry(FW_CFG_CMDLINE_SIZE, 4, &cmdline_size);
> +       if (cmdline_size) {
> +               qemu_fwcfg_read_entry(FW_CFG_CMDLINE_DATA,
> +                                     le32_to_cpu(cmdline_size), data_addr);
> +               if (setenv("bootargs", data_addr) < 0)
> +                       printf("warning: unable to change bootargs\n");
> +       }
> +
> +       printf("loading kernel to address %p", load_addr);
> +       if (initrd_size)
> +               printf(" initrd %p, size 0x%x\n",
> +                      initrd_addr,
> +                      le32_to_cpu(initrd_size));
> +       else
> +               printf("\n");
> +
> +       return 0;
> +}
> +
> +static int qemu_fwcfg_list_firmware(void)
> +{
> +       int i;
> +       uint32_t count;
> +       struct fw_cfg_files *files;
> +
> +       qemu_fwcfg_read_entry(FW_CFG_FILE_DIR, 4, &count);
> +       if (!count)
> +               return 0;
> +
> +       count = be32_to_cpu(count);
> +       files = malloc(count * sizeof(struct fw_cfg_file));
> +       if (!files)
> +               return -ENOMEM;
> +
> +       files->count = count;
> +       qemu_fwcfg_read_entry(FW_CFG_INVALID,
> +                             count * sizeof(struct fw_cfg_file),
> +                             files->files);
> +
> +       for (i = 0; i < files->count; i++)
> +               printf("%-56s\n", files->files[i].name);
> +       free(files);
> +       return 0;
> +}
> +
> +void qemu_fwcfg_init(void)
> +{
> +       fwcfg_present = qemu_fwcfg_present();
> +       if (fwcfg_present)
> +               fwcfg_dma_present = qemu_fwcfg_dma_present();
> +}
> +
> +static int qemu_fwcfg_do_list(cmd_tbl_t *cmdtp, int flag,
> +               int argc, char * const argv[])
> +{
> +       qemu_fwcfg_list_firmware();

Check return value and return CMD_RET_FAILURE if non-zero.

> +
> +       return 0;
> +}
> +
> +static int qemu_fwcfg_do_cpus(cmd_tbl_t *cmdtp, int flag,
> +               int argc, char * const argv[])
> +{
> +       printf("%d cpu(s) online\n", qemu_fwcfg_online_cpus());

But qemu_fwcfg_online_cpus() can fail.

> +
> +       return 0;
> +}
> +
> +static int qemu_fwcfg_do_load(cmd_tbl_t *cmdtp, int flag,
> +               int argc, char * const argv[])
> +{
> +       char *env;
> +       void *load_addr;
> +       void *initrd_addr;
> +
> +       env = getenv("loadaddr");
> +       load_addr = env ?
> +               (void *)simple_strtoul(env, NULL, 16) :
> +               (void *)CONFIG_LOADADDR;
> +
> +       env = getenv("ramdiskaddr");
> +       initrd_addr = env ?
> +               (void *)simple_strtoul(env, NULL, 16) :
> +               (void *)CONFIG_RAMDISK_ADDR;
> +
> +       if (argc == 2) {
> +               load_addr = (void *)simple_strtoul(argv[0], NULL, 16);
> +               initrd_addr = (void *)simple_strtoul(argv[1], NULL, 16);
> +       } else if (argc == 1) {
> +               load_addr = (void *)simple_strtoul(argv[0], NULL, 16);
> +       }
> +
> +       return qemu_fwcfg_setup_kernel(load_addr, initrd_addr);
> +}
> +
> +static cmd_tbl_t fwcfg_commands[] = {
> +       U_BOOT_CMD_MKENT(list, 0, 1, qemu_fwcfg_do_list, "", ""),
> +       U_BOOT_CMD_MKENT(cpus, 0, 1, qemu_fwcfg_do_cpus, "", ""),
> +       U_BOOT_CMD_MKENT(load, 2, 1, qemu_fwcfg_do_load, "", ""),
> +};
> +
> +static int do_qemu_fw(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +       int ret;
> +       cmd_tbl_t *fwcfg_cmd;
> +
> +       if (!fwcfg_present) {
> +               printf("QEMU fw_cfg interface not found\n");
> +               return CMD_RET_USAGE;
> +       }
> +
> +       fwcfg_cmd = find_cmd_tbl(argv[1], fwcfg_commands,
> +                                ARRAY_SIZE(fwcfg_commands));
> +       argc -= 2;
> +       argv += 2;
> +       if (!fwcfg_cmd || argc > fwcfg_cmd->maxargs)
> +               return CMD_RET_USAGE;
> +
> +       ret = fwcfg_cmd->cmd(fwcfg_cmd, flag, argc, argv);
> +
> +       return cmd_process_error(fwcfg_cmd, ret);
> +}
> +
> +U_BOOT_CMD(
> +       fw,     4,      1,      do_qemu_fw,

I worry that 'fw' is too generic. Perhaps qfw would be better?

> +       "QEMU firmware interface",
> +       "<command>\n"
> +       "    - list                             : print firmware(s) currently loaded\n"
> +       "    - cpus                             : print online cpu number\n"

print number of online CPUs

> +       "    - load <kernel addr> <initrd addr> : load kernel and initrd (if any), and setup for zboot\n"
> +)
> diff --git a/arch/x86/cpu/qemu/fw_cfg.h b/arch/x86/cpu/qemu/fw_cfg.h
> new file mode 100644
> index 0000000..03ac27d
> --- /dev/null
> +++ b/arch/x86/cpu/qemu/fw_cfg.h
> @@ -0,0 +1,97 @@
> +/*
> + * (C) Copyright 2015 Miao Yan <yanmiaobest@gmail.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#ifndef __FW_CFG__
> +#define __FW_CFG__
> +
> +#define FW_CONTROL_PORT        0x510
> +#define FW_DATA_PORT           0x511
> +#define FW_DMA_PORT_LOW        0x514
> +#define FW_DMA_PORT_HIGH       0x518
> +
> +enum qemu_fwcfg_items {
> +       FW_CFG_SIGNATURE        = 0x00,
> +       FW_CFG_ID               = 0x01,
> +       FW_CFG_UUID             = 0x02,
> +       FW_CFG_RAM_SIZE         = 0x03,
> +       FW_CFG_NOGRAPHIC        = 0x04,
> +       FW_CFG_NB_CPUS          = 0x05,
> +       FW_CFG_MACHINE_ID       = 0x06,
> +       FW_CFG_KERNEL_ADDR      = 0x07,
> +       FW_CFG_KERNEL_SIZE      = 0x08,
> +       FW_CFG_KERNEL_CMDLINE   = 0x09,
> +       FW_CFG_INITRD_ADDR      = 0x0a,
> +       FW_CFG_INITRD_SIZE      = 0x0b,
> +       FW_CFG_BOOT_DEVICE      = 0x0c,
> +       FW_CFG_NUMA             = 0x0d,
> +       FW_CFG_BOOT_MENU        = 0x0e,
> +       FW_CFG_MAX_CPUS         = 0x0f,
> +       FW_CFG_KERNEL_ENTRY     = 0x10,
> +       FW_CFG_KERNEL_DATA      = 0x11,
> +       FW_CFG_INITRD_DATA      = 0x12,
> +       FW_CFG_CMDLINE_ADDR     = 0x13,
> +       FW_CFG_CMDLINE_SIZE     = 0x14,
> +       FW_CFG_CMDLINE_DATA     = 0x15,
> +       FW_CFG_SETUP_ADDR       = 0x16,
> +       FW_CFG_SETUP_SIZE       = 0x17,
> +       FW_CFG_SETUP_DATA       = 0x18,
> +       FW_CFG_FILE_DIR         = 0x19,
> +       FW_CFG_FILE_FIRST       = 0x20,
> +       FW_CFG_WRITE_CHANNEL    = 0x4000,
> +       FW_CFG_ARCH_LOCAL       = 0x8000,
> +       FW_CFG_INVALID          = 0xffff,
> +};
> +
> +#define FW_CFG_FILE_SLOTS      0x10
> +#define FW_CFG_MAX_ENTRY       (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
> +#define FW_CFG_ENTRY_MASK       ~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL)
> +
> +#define FW_CFG_MAX_FILE_PATH   56
> +
> +#define QEMU_FW_CFG_SIGNATURE  (('Q' << 24) | ('E' << 16) | ('M' << 8) | 'U')
> +
> +#define FW_CFG_DMA_ERROR       (1 << 0)
> +#define FW_CFG_DMA_READ        (1 << 1)
> +#define FW_CFG_DMA_SKIP        (1 << 2)
> +#define FW_CFG_DMA_SELECT      (1 << 3)
> +
> +#define FW_CFG_DMA_ENABLED     (1 << 1)
> +
> +struct fw_cfg_file {
> +       __be32 size;
> +       __be16 select;
> +       __be16 reserved;
> +       char name[FW_CFG_MAX_FILE_PATH];
> +};
> +
> +struct fw_cfg_files {
> +       __be32 count;
> +       struct fw_cfg_file files[];
> +};
> +
> +struct fw_cfg_dma_access {
> +       __be32 control;
> +       __be32 length;
> +       __be64 address;
> +};
> +
> +/**
> + * Initialize QEMU fw_cfg interface
> + *
> + * @return:   None

You can drop this line

> + */
> +
> +void qemu_fwcfg_init(void);
> +
> +/**
> + * Get system cpu number
> + *
> + * @return:   cpu number in system
> + */
> +
> +int qemu_fwcfg_online_cpus(void);
> +
> +#endif
> diff --git a/arch/x86/cpu/qemu/qemu.c b/arch/x86/cpu/qemu/qemu.c
> index 1f93f72..d9ae066 100644
> --- a/arch/x86/cpu/qemu/qemu.c
> +++ b/arch/x86/cpu/qemu/qemu.c
> @@ -11,6 +11,7 @@
>  #include <asm/processor.h>
>  #include <asm/arch/device.h>
>  #include <asm/arch/qemu.h>
> +#include "fw_cfg.h"
>
>  static bool i440fx;
>
> @@ -57,6 +58,8 @@ static void qemu_chipset_init(void)
>                 x86_pci_write_config32(PCI_BDF(0, 0, 0), PCIEX_BAR,
>                                        CONFIG_PCIE_ECAM_BASE | BAR_EN);
>         }
> +
> +       qemu_fwcfg_init();
>  }
>
>  int arch_cpu_init(void)
> --
> 1.9.1
>

Regards,
Simon

Hi Simon,

2015-12-31 13:07 GMT+08:00 Simon Glass <sjg@chromium.org>:
> Hi Miao,
>
> On 30 December 2015 at 19:55, Miao Yan <yanmiaobest@gmail.com> wrote:
>> The QEMU fw_cfg interface allows the guest to retrieve various
>> data information from QEMU. For example, APCI/SMBios tables, number
>> of online cpus, kernel data and command line, etc.
>>
>> This patch adds support for QEMU fw_cfg interface.
>>
>> Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
>> ---
>> Changes in v4:
>>   - cleanups
>>   - change 'fw load' to take second parameter for initrd load address
>>
>>  arch/x86/cpu/qemu/Makefile |   2 +-
>>  arch/x86/cpu/qemu/fw_cfg.c | 268 +++++++++++++++++++++++++++++++++++++++++++++
>>  arch/x86/cpu/qemu/fw_cfg.h |  97 ++++++++++++++++
>>  arch/x86/cpu/qemu/qemu.c   |   3 +
>>  4 files changed, 369 insertions(+), 1 deletion(-)
>>  create mode 100644 arch/x86/cpu/qemu/fw_cfg.c
>>  create mode 100644 arch/x86/cpu/qemu/fw_cfg.h
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> But a few nits...
>
>>
>> diff --git a/arch/x86/cpu/qemu/Makefile b/arch/x86/cpu/qemu/Makefile
>> index 3f3958a..ad424ec 100644
>> --- a/arch/x86/cpu/qemu/Makefile
>> +++ b/arch/x86/cpu/qemu/Makefile
>> @@ -7,5 +7,5 @@
>>  ifndef CONFIG_EFI_STUB
>>  obj-y += car.o dram.o
>>  endif
>> -obj-y += qemu.o
>> +obj-y += qemu.o fw_cfg.o
>
> Can you put the new file first, so that the list is in alphabetical order?
>
>>  obj-$(CONFIG_GENERATE_ACPI_TABLE) += acpi.o dsdt.o
>> diff --git a/arch/x86/cpu/qemu/fw_cfg.c b/arch/x86/cpu/qemu/fw_cfg.c
>> new file mode 100644
>> index 0000000..9de8680
>> --- /dev/null
>> +++ b/arch/x86/cpu/qemu/fw_cfg.c
>> @@ -0,0 +1,268 @@
>> +/*
>> + * (C) Copyright 2015 Miao Yan <yanmiaoebst@gmail.com>
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <command.h>
>> +#include <errno.h>
>> +#include <malloc.h>
>> +#include <asm/io.h>
>> +#include "fw_cfg.h"
>> +
>> +static bool fwcfg_present;
>> +static bool fwcfg_dma_present;
>> +
>> +static void qemu_fwcfg_read_entry_pio(uint16_t entry,
>> +               uint32_t size, void *address)
>
> Function comment - what does this do?
>
>> +{
>> +       uint32_t i = 0;
>> +       uint8_t *data = address;
>> +
>> +       /*
>> +        * writting FW_CFG_INVALID will cause
>> +        * read operation to resume at
>> +        * last offset, otherwise read will
>> +        * start at offset 0
>
> Please can you format comments to 75 columns or thereabout?
>
>> +        */
>> +
>> +       if (entry != FW_CFG_INVALID)
>> +               outw(entry, FW_CONTROL_PORT);
>> +       while (size--)
>> +               data[i++] = inb(FW_DATA_PORT);
>> +}
>> +
>> +static void qemu_fwcfg_read_entry_dma(uint16_t entry,
>> +               uint32_t size, void *address)
>> +{
>> +       struct fw_cfg_dma_access dma;
>> +
>> +       dma.length = cpu_to_be32(size);
>> +       dma.address = cpu_to_be64((uintptr_t)address);
>> +       dma.control = cpu_to_be32(FW_CFG_DMA_READ);
>> +
>> +       /*
>> +        * writting FW_CFG_INVALID will cause
>> +        * read operation to resume at
>> +        * last offset, otherwise read will
>> +        * start at offset 0
>> +        */
>> +
>> +       if (entry != FW_CFG_INVALID)
>> +               dma.control |= cpu_to_be32(FW_CFG_DMA_SELECT | (entry << 16));
>> +
>> +       barrier();
>> +
>> +       debug("qemu_fwcfg_dma_read_entry: addr %p, length %u control 0x%x\n",
>> +             address, size, be32_to_cpu(dma.control));
>> +
>> +       outl(cpu_to_be32((uint32_t)&dma), FW_DMA_PORT_HIGH);
>> +
>> +       while (be32_to_cpu(dma.control) & ~FW_CFG_DMA_ERROR)
>> +               __asm__ __volatile__ ("pause");
>> +}
>> +
>> +static bool qemu_fwcfg_present(void)
>> +{
>> +       uint32_t qemu;
>> +
>> +       qemu_fwcfg_read_entry_pio(FW_CFG_SIGNATURE, 4, &qemu);
>> +       return be32_to_cpu(qemu) == QEMU_FW_CFG_SIGNATURE;
>> +}
>> +
>> +static bool qemu_fwcfg_dma_present(void)
>> +{
>> +       uint8_t dma_enabled;
>> +
>> +       qemu_fwcfg_read_entry_pio(FW_CFG_ID, 1, &dma_enabled);
>> +       if (dma_enabled & FW_CFG_DMA_ENABLED)
>> +               return true;
>> +
>> +       return false;
>> +}
>> +
>> +static void qemu_fwcfg_read_entry(uint16_t entry,
>> +               uint32_t length, void *address)
>> +{
>> +       if (fwcfg_dma_present)
>> +               qemu_fwcfg_read_entry_dma(entry, length, address);
>> +       else
>> +               qemu_fwcfg_read_entry_pio(entry, length, address);
>> +}
>> +
>> +int qemu_fwcfg_online_cpus(void)
>> +{
>> +       uint16_t nb_cpus;
>> +
>> +       if (!fwcfg_present)
>> +               return 1;
>
> -ENODEV


Can we return 1 cpu if fw_cfg interface is not avaliable (which is
quite unlikey),
and print a warning maybe ? Because there has to be one cpu at least
and returning
-ENODEV wouldn't make much difference.

I'll fix rest of your comments and thanks for the review.

Miao


>
>> +
>> +       qemu_fwcfg_read_entry(FW_CFG_NB_CPUS, 2, &nb_cpus);
>> +
>> +       return le16_to_cpu(nb_cpus);
>> +}
>> +
>> +static int qemu_fwcfg_setup_kernel(void *load_addr, void *initrd_addr)
>
> Function comment
>
>> +{
>> +       char *data_addr;
>> +       uint32_t setup_size, kernel_size, cmdline_size, initrd_size;
>> +
>> +       qemu_fwcfg_read_entry(FW_CFG_SETUP_SIZE, 4, &setup_size);
>> +       qemu_fwcfg_read_entry(FW_CFG_KERNEL_SIZE, 4, &kernel_size);
>> +
>> +       if (setup_size == 0 || kernel_size == 0) {
>> +               printf("warning: no kernel available\n");
>> +               return -1;
>> +       }
>> +
>> +       data_addr = load_addr;
>> +       qemu_fwcfg_read_entry(FW_CFG_SETUP_DATA,
>> +                             le32_to_cpu(setup_size), data_addr);
>> +       data_addr += le32_to_cpu(setup_size);
>> +
>> +       qemu_fwcfg_read_entry(FW_CFG_KERNEL_DATA,
>> +                             le32_to_cpu(kernel_size), data_addr);
>> +       data_addr += le32_to_cpu(kernel_size);
>> +
>> +       data_addr = initrd_addr;
>> +       qemu_fwcfg_read_entry(FW_CFG_INITRD_SIZE, 4, &initrd_size);
>> +       if (initrd_size == 0) {
>> +               printf("warning: no initrd available\n");
>> +       } else {
>> +               qemu_fwcfg_read_entry(FW_CFG_INITRD_DATA,
>> +                                     le32_to_cpu(initrd_size), data_addr);
>> +               data_addr += le32_to_cpu(initrd_size);
>> +       }
>> +
>> +       qemu_fwcfg_read_entry(FW_CFG_CMDLINE_SIZE, 4, &cmdline_size);
>> +       if (cmdline_size) {
>> +               qemu_fwcfg_read_entry(FW_CFG_CMDLINE_DATA,
>> +                                     le32_to_cpu(cmdline_size), data_addr);
>> +               if (setenv("bootargs", data_addr) < 0)
>> +                       printf("warning: unable to change bootargs\n");
>> +       }
>> +
>> +       printf("loading kernel to address %p", load_addr);
>> +       if (initrd_size)
>> +               printf(" initrd %p, size 0x%x\n",
>> +                      initrd_addr,
>> +                      le32_to_cpu(initrd_size));
>> +       else
>> +               printf("\n");
>> +
>> +       return 0;
>> +}
>> +
>> +static int qemu_fwcfg_list_firmware(void)
>> +{
>> +       int i;
>> +       uint32_t count;
>> +       struct fw_cfg_files *files;
>> +
>> +       qemu_fwcfg_read_entry(FW_CFG_FILE_DIR, 4, &count);
>> +       if (!count)
>> +               return 0;
>> +
>> +       count = be32_to_cpu(count);
>> +       files = malloc(count * sizeof(struct fw_cfg_file));
>> +       if (!files)
>> +               return -ENOMEM;
>> +
>> +       files->count = count;
>> +       qemu_fwcfg_read_entry(FW_CFG_INVALID,
>> +                             count * sizeof(struct fw_cfg_file),
>> +                             files->files);
>> +
>> +       for (i = 0; i < files->count; i++)
>> +               printf("%-56s\n", files->files[i].name);
>> +       free(files);
>> +       return 0;
>> +}
>> +
>> +void qemu_fwcfg_init(void)
>> +{
>> +       fwcfg_present = qemu_fwcfg_present();
>> +       if (fwcfg_present)
>> +               fwcfg_dma_present = qemu_fwcfg_dma_present();
>> +}
>> +
>> +static int qemu_fwcfg_do_list(cmd_tbl_t *cmdtp, int flag,
>> +               int argc, char * const argv[])
>> +{
>> +       qemu_fwcfg_list_firmware();
>
> Check return value and return CMD_RET_FAILURE if non-zero.
>
>> +
>> +       return 0;
>> +}
>> +
>> +static int qemu_fwcfg_do_cpus(cmd_tbl_t *cmdtp, int flag,
>> +               int argc, char * const argv[])
>> +{
>> +       printf("%d cpu(s) online\n", qemu_fwcfg_online_cpus());
>
> But qemu_fwcfg_online_cpus() can fail.
>
>> +
>> +       return 0;
>> +}
>> +
>> +static int qemu_fwcfg_do_load(cmd_tbl_t *cmdtp, int flag,
>> +               int argc, char * const argv[])
>> +{
>> +       char *env;
>> +       void *load_addr;
>> +       void *initrd_addr;
>> +
>> +       env = getenv("loadaddr");
>> +       load_addr = env ?
>> +               (void *)simple_strtoul(env, NULL, 16) :
>> +               (void *)CONFIG_LOADADDR;
>> +
>> +       env = getenv("ramdiskaddr");
>> +       initrd_addr = env ?
>> +               (void *)simple_strtoul(env, NULL, 16) :
>> +               (void *)CONFIG_RAMDISK_ADDR;
>> +
>> +       if (argc == 2) {
>> +               load_addr = (void *)simple_strtoul(argv[0], NULL, 16);
>> +               initrd_addr = (void *)simple_strtoul(argv[1], NULL, 16);
>> +       } else if (argc == 1) {
>> +               load_addr = (void *)simple_strtoul(argv[0], NULL, 16);
>> +       }
>> +
>> +       return qemu_fwcfg_setup_kernel(load_addr, initrd_addr);
>> +}
>> +
>> +static cmd_tbl_t fwcfg_commands[] = {
>> +       U_BOOT_CMD_MKENT(list, 0, 1, qemu_fwcfg_do_list, "", ""),
>> +       U_BOOT_CMD_MKENT(cpus, 0, 1, qemu_fwcfg_do_cpus, "", ""),
>> +       U_BOOT_CMD_MKENT(load, 2, 1, qemu_fwcfg_do_load, "", ""),
>> +};
>> +
>> +static int do_qemu_fw(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> +       int ret;
>> +       cmd_tbl_t *fwcfg_cmd;
>> +
>> +       if (!fwcfg_present) {
>> +               printf("QEMU fw_cfg interface not found\n");
>> +               return CMD_RET_USAGE;
>> +       }
>> +
>> +       fwcfg_cmd = find_cmd_tbl(argv[1], fwcfg_commands,
>> +                                ARRAY_SIZE(fwcfg_commands));
>> +       argc -= 2;
>> +       argv += 2;
>> +       if (!fwcfg_cmd || argc > fwcfg_cmd->maxargs)
>> +               return CMD_RET_USAGE;
>> +
>> +       ret = fwcfg_cmd->cmd(fwcfg_cmd, flag, argc, argv);
>> +
>> +       return cmd_process_error(fwcfg_cmd, ret);
>> +}
>> +
>> +U_BOOT_CMD(
>> +       fw,     4,      1,      do_qemu_fw,
>
> I worry that 'fw' is too generic. Perhaps qfw would be better?
>
>> +       "QEMU firmware interface",
>> +       "<command>\n"
>> +       "    - list                             : print firmware(s) currently loaded\n"
>> +       "    - cpus                             : print online cpu number\n"
>
> print number of online CPUs
>
>> +       "    - load <kernel addr> <initrd addr> : load kernel and initrd (if any), and setup for zboot\n"
>> +)
>> diff --git a/arch/x86/cpu/qemu/fw_cfg.h b/arch/x86/cpu/qemu/fw_cfg.h
>> new file mode 100644
>> index 0000000..03ac27d
>> --- /dev/null
>> +++ b/arch/x86/cpu/qemu/fw_cfg.h
>> @@ -0,0 +1,97 @@
>> +/*
>> + * (C) Copyright 2015 Miao Yan <yanmiaobest@gmail.com>
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#ifndef __FW_CFG__
>> +#define __FW_CFG__
>> +
>> +#define FW_CONTROL_PORT        0x510
>> +#define FW_DATA_PORT           0x511
>> +#define FW_DMA_PORT_LOW        0x514
>> +#define FW_DMA_PORT_HIGH       0x518
>> +
>> +enum qemu_fwcfg_items {
>> +       FW_CFG_SIGNATURE        = 0x00,
>> +       FW_CFG_ID               = 0x01,
>> +       FW_CFG_UUID             = 0x02,
>> +       FW_CFG_RAM_SIZE         = 0x03,
>> +       FW_CFG_NOGRAPHIC        = 0x04,
>> +       FW_CFG_NB_CPUS          = 0x05,
>> +       FW_CFG_MACHINE_ID       = 0x06,
>> +       FW_CFG_KERNEL_ADDR      = 0x07,
>> +       FW_CFG_KERNEL_SIZE      = 0x08,
>> +       FW_CFG_KERNEL_CMDLINE   = 0x09,
>> +       FW_CFG_INITRD_ADDR      = 0x0a,
>> +       FW_CFG_INITRD_SIZE      = 0x0b,
>> +       FW_CFG_BOOT_DEVICE      = 0x0c,
>> +       FW_CFG_NUMA             = 0x0d,
>> +       FW_CFG_BOOT_MENU        = 0x0e,
>> +       FW_CFG_MAX_CPUS         = 0x0f,
>> +       FW_CFG_KERNEL_ENTRY     = 0x10,
>> +       FW_CFG_KERNEL_DATA      = 0x11,
>> +       FW_CFG_INITRD_DATA      = 0x12,
>> +       FW_CFG_CMDLINE_ADDR     = 0x13,
>> +       FW_CFG_CMDLINE_SIZE     = 0x14,
>> +       FW_CFG_CMDLINE_DATA     = 0x15,
>> +       FW_CFG_SETUP_ADDR       = 0x16,
>> +       FW_CFG_SETUP_SIZE       = 0x17,
>> +       FW_CFG_SETUP_DATA       = 0x18,
>> +       FW_CFG_FILE_DIR         = 0x19,
>> +       FW_CFG_FILE_FIRST       = 0x20,
>> +       FW_CFG_WRITE_CHANNEL    = 0x4000,
>> +       FW_CFG_ARCH_LOCAL       = 0x8000,
>> +       FW_CFG_INVALID          = 0xffff,
>> +};
>> +
>> +#define FW_CFG_FILE_SLOTS      0x10
>> +#define FW_CFG_MAX_ENTRY       (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
>> +#define FW_CFG_ENTRY_MASK       ~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL)
>> +
>> +#define FW_CFG_MAX_FILE_PATH   56
>> +
>> +#define QEMU_FW_CFG_SIGNATURE  (('Q' << 24) | ('E' << 16) | ('M' << 8) | 'U')
>> +
>> +#define FW_CFG_DMA_ERROR       (1 << 0)
>> +#define FW_CFG_DMA_READ        (1 << 1)
>> +#define FW_CFG_DMA_SKIP        (1 << 2)
>> +#define FW_CFG_DMA_SELECT      (1 << 3)
>> +
>> +#define FW_CFG_DMA_ENABLED     (1 << 1)
>> +
>> +struct fw_cfg_file {
>> +       __be32 size;
>> +       __be16 select;
>> +       __be16 reserved;
>> +       char name[FW_CFG_MAX_FILE_PATH];
>> +};
>> +
>> +struct fw_cfg_files {
>> +       __be32 count;
>> +       struct fw_cfg_file files[];
>> +};
>> +
>> +struct fw_cfg_dma_access {
>> +       __be32 control;
>> +       __be32 length;
>> +       __be64 address;
>> +};
>> +
>> +/**
>> + * Initialize QEMU fw_cfg interface
>> + *
>> + * @return:   None
>
> You can drop this line
>
>> + */
>> +
>> +void qemu_fwcfg_init(void);
>> +
>> +/**
>> + * Get system cpu number
>> + *
>> + * @return:   cpu number in system
>> + */
>> +
>> +int qemu_fwcfg_online_cpus(void);
>> +
>> +#endif
>> diff --git a/arch/x86/cpu/qemu/qemu.c b/arch/x86/cpu/qemu/qemu.c
>> index 1f93f72..d9ae066 100644
>> --- a/arch/x86/cpu/qemu/qemu.c
>> +++ b/arch/x86/cpu/qemu/qemu.c
>> @@ -11,6 +11,7 @@
>>  #include <asm/processor.h>
>>  #include <asm/arch/device.h>
>>  #include <asm/arch/qemu.h>
>> +#include "fw_cfg.h"
>>
>>  static bool i440fx;
>>
>> @@ -57,6 +58,8 @@ static void qemu_chipset_init(void)
>>                 x86_pci_write_config32(PCI_BDF(0, 0, 0), PCIEX_BAR,
>>                                        CONFIG_PCIE_ECAM_BASE | BAR_EN);
>>         }
>> +
>> +       qemu_fwcfg_init();
>>  }
>>
>>  int arch_cpu_init(void)
>> --
>> 1.9.1
>>
>
> Regards,
> Simon

Hi Miao,

On 31 December 2015 at 01:42, Miao Yan <yanmiaobest@gmail.com> wrote:
> Hi Simon,
>
> 2015-12-31 13:07 GMT+08:00 Simon Glass <sjg@chromium.org>:
>> Hi Miao,
>>
>> On 30 December 2015 at 19:55, Miao Yan <yanmiaobest@gmail.com> wrote:
>>> The QEMU fw_cfg interface allows the guest to retrieve various
>>> data information from QEMU. For example, APCI/SMBios tables, number
>>> of online cpus, kernel data and command line, etc.
>>>
>>> This patch adds support for QEMU fw_cfg interface.
>>>
>>> Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
>>> ---
>>> Changes in v4:
>>>   - cleanups
>>>   - change 'fw load' to take second parameter for initrd load address
>>>
>>>  arch/x86/cpu/qemu/Makefile |   2 +-
>>>  arch/x86/cpu/qemu/fw_cfg.c | 268 +++++++++++++++++++++++++++++++++++++++++++++
>>>  arch/x86/cpu/qemu/fw_cfg.h |  97 ++++++++++++++++
>>>  arch/x86/cpu/qemu/qemu.c   |   3 +
>>>  4 files changed, 369 insertions(+), 1 deletion(-)
>>>  create mode 100644 arch/x86/cpu/qemu/fw_cfg.c
>>>  create mode 100644 arch/x86/cpu/qemu/fw_cfg.h
>>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> But a few nits...
[snip]
>>> +int qemu_fwcfg_online_cpus(void)
>>> +{
>>> +       uint16_t nb_cpus;
>>> +
>>> +       if (!fwcfg_present)
>>> +               return 1;
>>
>> -ENODEV
>
>
> Can we return 1 cpu if fw_cfg interface is not avaliable (which is
> quite unlikey),
> and print a warning maybe ? Because there has to be one cpu at least
> and returning
> -ENODEV wouldn't make much difference.

I see. In that case I think it is better if qemu_fwcfg_online_cpus()
returns an error and its caller can detect -ENODEV and use '1' as the
value. Then the decision as to how to deal with the error is handled
at the layer that is taking action.

>
> I'll fix rest of your comments and thanks for the review.
>
> Miao
>
[snip]

Hi Miao,

On Thu, Dec 31, 2015 at 10:55 AM, Miao Yan <yanmiaobest@gmail.com> wrote:
> The QEMU fw_cfg interface allows the guest to retrieve various
> data information from QEMU. For example, APCI/SMBios tables, number
> of online cpus, kernel data and command line, etc.
>
> This patch adds support for QEMU fw_cfg interface.
>
> Signed-off-by: Miao Yan <yanmiaobest@gmail.com>
> ---
> Changes in v4:
>   - cleanups
>   - change 'fw load' to take second parameter for initrd load address
>
>  arch/x86/cpu/qemu/Makefile |   2 +-
>  arch/x86/cpu/qemu/fw_cfg.c | 268 +++++++++++++++++++++++++++++++++++++++++++++
>  arch/x86/cpu/qemu/fw_cfg.h |  97 ++++++++++++++++
>  arch/x86/cpu/qemu/qemu.c   |   3 +
>  4 files changed, 369 insertions(+), 1 deletion(-)
>  create mode 100644 arch/x86/cpu/qemu/fw_cfg.c
>  create mode 100644 arch/x86/cpu/qemu/fw_cfg.h
>
> diff --git a/arch/x86/cpu/qemu/Makefile b/arch/x86/cpu/qemu/Makefile
> index 3f3958a..ad424ec 100644
> --- a/arch/x86/cpu/qemu/Makefile
> +++ b/arch/x86/cpu/qemu/Makefile
> @@ -7,5 +7,5 @@
>  ifndef CONFIG_EFI_STUB
>  obj-y += car.o dram.o
>  endif
> -obj-y += qemu.o
> +obj-y += qemu.o fw_cfg.o
>  obj-$(CONFIG_GENERATE_ACPI_TABLE) += acpi.o dsdt.o
> diff --git a/arch/x86/cpu/qemu/fw_cfg.c b/arch/x86/cpu/qemu/fw_cfg.c
> new file mode 100644
> index 0000000..9de8680
> --- /dev/null
> +++ b/arch/x86/cpu/qemu/fw_cfg.c
> @@ -0,0 +1,268 @@
> +/*
> + * (C) Copyright 2015 Miao Yan <yanmiaoebst@gmail.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <errno.h>
> +#include <malloc.h>
> +#include <asm/io.h>
> +#include "fw_cfg.h"
> +
> +static bool fwcfg_present;
> +static bool fwcfg_dma_present;
> +
> +static void qemu_fwcfg_read_entry_pio(uint16_t entry,
> +               uint32_t size, void *address)
> +{
> +       uint32_t i = 0;
> +       uint8_t *data = address;
> +
> +       /*
> +        * writting FW_CFG_INVALID will cause
> +        * read operation to resume at
> +        * last offset, otherwise read will
> +        * start at offset 0
> +        */
> +
> +       if (entry != FW_CFG_INVALID)
> +               outw(entry, FW_CONTROL_PORT);
> +       while (size--)
> +               data[i++] = inb(FW_DATA_PORT);
> +}
> +
> +static void qemu_fwcfg_read_entry_dma(uint16_t entry,
> +               uint32_t size, void *address)
> +{
> +       struct fw_cfg_dma_access dma;
> +
> +       dma.length = cpu_to_be32(size);
> +       dma.address = cpu_to_be64((uintptr_t)address);
> +       dma.control = cpu_to_be32(FW_CFG_DMA_READ);
> +
> +       /*
> +        * writting FW_CFG_INVALID will cause
> +        * read operation to resume at
> +        * last offset, otherwise read will
> +        * start at offset 0
> +        */
> +
> +       if (entry != FW_CFG_INVALID)
> +               dma.control |= cpu_to_be32(FW_CFG_DMA_SELECT | (entry << 16));
> +
> +       barrier();
> +
> +       debug("qemu_fwcfg_dma_read_entry: addr %p, length %u control 0x%x\n",
> +             address, size, be32_to_cpu(dma.control));
> +
> +       outl(cpu_to_be32((uint32_t)&dma), FW_DMA_PORT_HIGH);
> +
> +       while (be32_to_cpu(dma.control) & ~FW_CFG_DMA_ERROR)
> +               __asm__ __volatile__ ("pause");
> +}
> +
> +static bool qemu_fwcfg_present(void)
> +{
> +       uint32_t qemu;
> +
> +       qemu_fwcfg_read_entry_pio(FW_CFG_SIGNATURE, 4, &qemu);
> +       return be32_to_cpu(qemu) == QEMU_FW_CFG_SIGNATURE;
> +}
> +
> +static bool qemu_fwcfg_dma_present(void)
> +{
> +       uint8_t dma_enabled;
> +
> +       qemu_fwcfg_read_entry_pio(FW_CFG_ID, 1, &dma_enabled);
> +       if (dma_enabled & FW_CFG_DMA_ENABLED)
> +               return true;
> +
> +       return false;
> +}
> +
> +static void qemu_fwcfg_read_entry(uint16_t entry,
> +               uint32_t length, void *address)
> +{
> +       if (fwcfg_dma_present)
> +               qemu_fwcfg_read_entry_dma(entry, length, address);
> +       else
> +               qemu_fwcfg_read_entry_pio(entry, length, address);
> +}
> +
> +int qemu_fwcfg_online_cpus(void)
> +{
> +       uint16_t nb_cpus;
> +
> +       if (!fwcfg_present)
> +               return 1;
> +
> +       qemu_fwcfg_read_entry(FW_CFG_NB_CPUS, 2, &nb_cpus);
> +
> +       return le16_to_cpu(nb_cpus);
> +}
> +
> +static int qemu_fwcfg_setup_kernel(void *load_addr, void *initrd_addr)
> +{
> +       char *data_addr;
> +       uint32_t setup_size, kernel_size, cmdline_size, initrd_size;
> +
> +       qemu_fwcfg_read_entry(FW_CFG_SETUP_SIZE, 4, &setup_size);
> +       qemu_fwcfg_read_entry(FW_CFG_KERNEL_SIZE, 4, &kernel_size);
> +
> +       if (setup_size == 0 || kernel_size == 0) {
> +               printf("warning: no kernel available\n");
> +               return -1;
> +       }
> +
> +       data_addr = load_addr;
> +       qemu_fwcfg_read_entry(FW_CFG_SETUP_DATA,
> +                             le32_to_cpu(setup_size), data_addr);
> +       data_addr += le32_to_cpu(setup_size);
> +
> +       qemu_fwcfg_read_entry(FW_CFG_KERNEL_DATA,
> +                             le32_to_cpu(kernel_size), data_addr);
> +       data_addr += le32_to_cpu(kernel_size);
> +
> +       data_addr = initrd_addr;
> +       qemu_fwcfg_read_entry(FW_CFG_INITRD_SIZE, 4, &initrd_size);
> +       if (initrd_size == 0) {
> +               printf("warning: no initrd available\n");
> +       } else {
> +               qemu_fwcfg_read_entry(FW_CFG_INITRD_DATA,
> +                                     le32_to_cpu(initrd_size), data_addr);
> +               data_addr += le32_to_cpu(initrd_size);
> +       }
> +
> +       qemu_fwcfg_read_entry(FW_CFG_CMDLINE_SIZE, 4, &cmdline_size);
> +       if (cmdline_size) {
> +               qemu_fwcfg_read_entry(FW_CFG_CMDLINE_DATA,
> +                                     le32_to_cpu(cmdline_size), data_addr);
> +               if (setenv("bootargs", data_addr) < 0)
> +                       printf("warning: unable to change bootargs\n");
> +       }
> +
> +       printf("loading kernel to address %p", load_addr);
> +       if (initrd_size)
> +               printf(" initrd %p, size 0x%x\n",
> +                      initrd_addr,
> +                      le32_to_cpu(initrd_size));
> +       else
> +               printf("\n");
> +
> +       return 0;
> +}
> +
> +static int qemu_fwcfg_list_firmware(void)
> +{
> +       int i;
> +       uint32_t count;
> +       struct fw_cfg_files *files;
> +
> +       qemu_fwcfg_read_entry(FW_CFG_FILE_DIR, 4, &count);
> +       if (!count)
> +               return 0;
> +
> +       count = be32_to_cpu(count);
> +       files = malloc(count * sizeof(struct fw_cfg_file));
> +       if (!files)
> +               return -ENOMEM;
> +
> +       files->count = count;
> +       qemu_fwcfg_read_entry(FW_CFG_INVALID,
> +                             count * sizeof(struct fw_cfg_file),
> +                             files->files);
> +
> +       for (i = 0; i < files->count; i++)
> +               printf("%-56s\n", files->files[i].name);
> +       free(files);
> +       return 0;
> +}
> +
> +void qemu_fwcfg_init(void)
> +{
> +       fwcfg_present = qemu_fwcfg_present();
> +       if (fwcfg_present)
> +               fwcfg_dma_present = qemu_fwcfg_dma_present();
> +}
> +
> +static int qemu_fwcfg_do_list(cmd_tbl_t *cmdtp, int flag,
> +               int argc, char * const argv[])
> +{
> +       qemu_fwcfg_list_firmware();
> +
> +       return 0;
> +}
> +
> +static int qemu_fwcfg_do_cpus(cmd_tbl_t *cmdtp, int flag,
> +               int argc, char * const argv[])
> +{
> +       printf("%d cpu(s) online\n", qemu_fwcfg_online_cpus());
> +
> +       return 0;
> +}
> +
> +static int qemu_fwcfg_do_load(cmd_tbl_t *cmdtp, int flag,
> +               int argc, char * const argv[])
> +{
> +       char *env;
> +       void *load_addr;
> +       void *initrd_addr;
> +
> +       env = getenv("loadaddr");
> +       load_addr = env ?
> +               (void *)simple_strtoul(env, NULL, 16) :
> +               (void *)CONFIG_LOADADDR;
> +
> +       env = getenv("ramdiskaddr");
> +       initrd_addr = env ?
> +               (void *)simple_strtoul(env, NULL, 16) :
> +               (void *)CONFIG_RAMDISK_ADDR;

This does not build as CONFIG_RAMDISK_ADDR is introduced in a later
patch. Please reorder the patches.

> +
> +       if (argc == 2) {
> +               load_addr = (void *)simple_strtoul(argv[0], NULL, 16);
> +               initrd_addr = (void *)simple_strtoul(argv[1], NULL, 16);
> +       } else if (argc == 1) {
> +               load_addr = (void *)simple_strtoul(argv[0], NULL, 16);
> +       }
> +
> +       return qemu_fwcfg_setup_kernel(load_addr, initrd_addr);
> +}
> +
> +static cmd_tbl_t fwcfg_commands[] = {
> +       U_BOOT_CMD_MKENT(list, 0, 1, qemu_fwcfg_do_list, "", ""),
> +       U_BOOT_CMD_MKENT(cpus, 0, 1, qemu_fwcfg_do_cpus, "", ""),
> +       U_BOOT_CMD_MKENT(load, 2, 1, qemu_fwcfg_do_load, "", ""),
> +};
> +
> +static int do_qemu_fw(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +       int ret;
> +       cmd_tbl_t *fwcfg_cmd;
> +
> +       if (!fwcfg_present) {
> +               printf("QEMU fw_cfg interface not found\n");
> +               return CMD_RET_USAGE;
> +       }
> +
> +       fwcfg_cmd = find_cmd_tbl(argv[1], fwcfg_commands,
> +                                ARRAY_SIZE(fwcfg_commands));
> +       argc -= 2;
> +       argv += 2;
> +       if (!fwcfg_cmd || argc > fwcfg_cmd->maxargs)
> +               return CMD_RET_USAGE;
> +
> +       ret = fwcfg_cmd->cmd(fwcfg_cmd, flag, argc, argv);
> +
> +       return cmd_process_error(fwcfg_cmd, ret);
> +}
> +
> +U_BOOT_CMD(
> +       fw,     4,      1,      do_qemu_fw,
> +       "QEMU firmware interface",
> +       "<command>\n"
> +       "    - list                             : print firmware(s) currently loaded\n"
> +       "    - cpus                             : print online cpu number\n"
> +       "    - load <kernel addr> <initrd addr> : load kernel and initrd (if any), and setup for zboot\n"
> +)
> diff --git a/arch/x86/cpu/qemu/fw_cfg.h b/arch/x86/cpu/qemu/fw_cfg.h
> new file mode 100644
> index 0000000..03ac27d
> --- /dev/null
> +++ b/arch/x86/cpu/qemu/fw_cfg.h
> @@ -0,0 +1,97 @@
> +/*
> + * (C) Copyright 2015 Miao Yan <yanmiaobest@gmail.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#ifndef __FW_CFG__
> +#define __FW_CFG__
> +
> +#define FW_CONTROL_PORT        0x510
> +#define FW_DATA_PORT           0x511
> +#define FW_DMA_PORT_LOW        0x514
> +#define FW_DMA_PORT_HIGH       0x518
> +
> +enum qemu_fwcfg_items {
> +       FW_CFG_SIGNATURE        = 0x00,
> +       FW_CFG_ID               = 0x01,
> +       FW_CFG_UUID             = 0x02,
> +       FW_CFG_RAM_SIZE         = 0x03,
> +       FW_CFG_NOGRAPHIC        = 0x04,
> +       FW_CFG_NB_CPUS          = 0x05,
> +       FW_CFG_MACHINE_ID       = 0x06,
> +       FW_CFG_KERNEL_ADDR      = 0x07,
> +       FW_CFG_KERNEL_SIZE      = 0x08,
> +       FW_CFG_KERNEL_CMDLINE   = 0x09,
> +       FW_CFG_INITRD_ADDR      = 0x0a,
> +       FW_CFG_INITRD_SIZE      = 0x0b,
> +       FW_CFG_BOOT_DEVICE      = 0x0c,
> +       FW_CFG_NUMA             = 0x0d,
> +       FW_CFG_BOOT_MENU        = 0x0e,
> +       FW_CFG_MAX_CPUS         = 0x0f,
> +       FW_CFG_KERNEL_ENTRY     = 0x10,
> +       FW_CFG_KERNEL_DATA      = 0x11,
> +       FW_CFG_INITRD_DATA      = 0x12,
> +       FW_CFG_CMDLINE_ADDR     = 0x13,
> +       FW_CFG_CMDLINE_SIZE     = 0x14,
> +       FW_CFG_CMDLINE_DATA     = 0x15,
> +       FW_CFG_SETUP_ADDR       = 0x16,
> +       FW_CFG_SETUP_SIZE       = 0x17,
> +       FW_CFG_SETUP_DATA       = 0x18,
> +       FW_CFG_FILE_DIR         = 0x19,
> +       FW_CFG_FILE_FIRST       = 0x20,
> +       FW_CFG_WRITE_CHANNEL    = 0x4000,
> +       FW_CFG_ARCH_LOCAL       = 0x8000,
> +       FW_CFG_INVALID          = 0xffff,
> +};
> +
> +#define FW_CFG_FILE_SLOTS      0x10
> +#define FW_CFG_MAX_ENTRY       (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)

Sorry, one more nits: please add space around '+'

> +#define FW_CFG_ENTRY_MASK       ~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL)
> +
> +#define FW_CFG_MAX_FILE_PATH   56
> +
> +#define QEMU_FW_CFG_SIGNATURE  (('Q' << 24) | ('E' << 16) | ('M' << 8) | 'U')
> +
> +#define FW_CFG_DMA_ERROR       (1 << 0)
> +#define FW_CFG_DMA_READ        (1 << 1)
> +#define FW_CFG_DMA_SKIP        (1 << 2)
> +#define FW_CFG_DMA_SELECT      (1 << 3)
> +
> +#define FW_CFG_DMA_ENABLED     (1 << 1)
> +
> +struct fw_cfg_file {
> +       __be32 size;
> +       __be16 select;
> +       __be16 reserved;
> +       char name[FW_CFG_MAX_FILE_PATH];
> +};
> +
> +struct fw_cfg_files {
> +       __be32 count;
> +       struct fw_cfg_file files[];
> +};
> +
> +struct fw_cfg_dma_access {
> +       __be32 control;
> +       __be32 length;
> +       __be64 address;
> +};
> +
> +/**
> + * Initialize QEMU fw_cfg interface
> + *
> + * @return:   None
> + */
> +
> +void qemu_fwcfg_init(void);
> +
> +/**
> + * Get system cpu number
> + *
> + * @return:   cpu number in system
> + */
> +
> +int qemu_fwcfg_online_cpus(void);
> +
> +#endif
> diff --git a/arch/x86/cpu/qemu/qemu.c b/arch/x86/cpu/qemu/qemu.c
> index 1f93f72..d9ae066 100644
> --- a/arch/x86/cpu/qemu/qemu.c
> +++ b/arch/x86/cpu/qemu/qemu.c
> @@ -11,6 +11,7 @@
>  #include <asm/processor.h>
>  #include <asm/arch/device.h>
>  #include <asm/arch/qemu.h>
> +#include "fw_cfg.h"
>
>  static bool i440fx;
>
> @@ -57,6 +58,8 @@ static void qemu_chipset_init(void)
>                 x86_pci_write_config32(PCI_BDF(0, 0, 0), PCIEX_BAR,
>                                        CONFIG_PCIE_ECAM_BASE | BAR_EN);
>         }
> +
> +       qemu_fwcfg_init();
>  }
>
>  int arch_cpu_init(void)
> --

Regards,
Bin