[U-Boot,1/7] 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>
---
 arch/x86/cpu/qemu/Makefile |   2 +-
 arch/x86/cpu/qemu/fw_cfg.c | 215 +++++++++++++++++++++++++++++++++++++++++++++
 arch/x86/cpu/qemu/fw_cfg.h |  84 ++++++++++++++++++
 arch/x86/cpu/qemu/qemu.c   |   3 +
 4 files changed, 303 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 Mon, Dec 28, 2015 at 5:18 PM, 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>
> ---
>  arch/x86/cpu/qemu/Makefile |   2 +-
>  arch/x86/cpu/qemu/fw_cfg.c | 215 +++++++++++++++++++++++++++++++++++++++++++++
>  arch/x86/cpu/qemu/fw_cfg.h |  84 ++++++++++++++++++
>  arch/x86/cpu/qemu/qemu.c   |   3 +
>  4 files changed, 303 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..e7615d1
> --- /dev/null
> +++ b/arch/x86/cpu/qemu/fw_cfg.c
> @@ -0,0 +1,215 @@
> +/*
> + * (C) Copyright 2015 Miao Yan <yanmiaoebst@gmail.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <environment.h>
> +#include <stdlib.h>

environment.h and stdlib.h are not needed.

> +#include <malloc.h>
> +#include <errno.h>

nits: move <error.h> to before <malloc.h>

> +#include <fs.h>

fs.h is not needed.

> +#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,

nits: please leave only one space between 'static' and 'void'

> +               uint32_t size, void *address)
> +{
> +       uint32_t i = 0;
> +       uint8_t *data = address;
> +
> +       if (entry != FW_CFG_INVALID)
> +               outw(entry, FW_CONTROL_PORT);

Missing branch to handle (entry == FW_CFG_INVALID).

> +       while (size--)
> +               data[i++] = inb(FW_DATA_PORT);
> +}
> +
> +static  void qemu_fwcfg_read_entry_dma(uint16_t entry,

nits: please leave only one space between 'static' and 'void'

> +               uint32_t length, void *address)

To keep consistency with qemu_fwcfg_read_entry_pio(), either change
'length' to 'size', or change 'size' parameter of
qemu_fwcfg_read_entry_pio() to 'length'.

> +{
> +       struct fw_cfg_dma_access dma;
> +
> +       debug("qemu_fwcfg_dma_read_entry: addr %p, length %u control 0x%x\n",
> +             address, length, dma.control);

dma.control is not initialized yet. We need move this debug before
"barrier()" below.

> +
> +       dma.length = cpu_to_be32(length);
> +       dma.address = cpu_to_be64((uintptr_t)address);
> +       dma.control = cpu_to_be32(FW_CFG_DMA_READ);
> +       if (entry != FW_CFG_INVALID)
> +               dma.control |= cpu_to_be32(FW_CFG_DMA_SELECT | (entry << 16));

Missing branch to handle (entry == FW_CFG_INVALID).

> +
> +       barrier();
> +
> +       outl(cpu_to_be32((uint32_t)&dma), FW_DMA_PORT + 4);

#define this "FW_DMA_PORT + 4" to a port macro?

> +
> +       while (dma.control & ~FW_CFG_DMA_ERROR)
> +               __asm__ __volatile__ ("rep;nop");

Just use "pause" instruction.

> +}
> +
> +static int qemu_fwcfg_present(void)

int -> bool

> +{
> +       uint32_t qemu;
> +
> +       qemu_fwcfg_read_entry_pio(FW_CFG_SIGNATURE, 4, &qemu);
> +       return be32_to_cpu(qemu) == QEMU_FW_CFG_SIGNATURE;
> +}
> +
> +static int qemu_fwcfg_dma_present(void)

int -> bool

> +{
> +       uint8_t dma_enabled;
> +       uint32_t qemu;
> +
> +       qemu_fwcfg_read_entry_pio(FW_CFG_ID, 1, &dma_enabled);
> +       if (dma_enabled & 0x1) {

#define 0x1 to something meaningful?

> +               qemu = inl(FW_DMA_PORT);
> +               if (be32_to_cpu(qemu) == QEMU_FW_CFG_SIGNATURE)
> +                       return 1;
> +       }
> +       return 0;
> +}
> +
> +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);
> +}
> +
> +uint16_t qemu_fwcfg_online_cpus(void)

Can we change the return value to int?

> +{
> +       uint16_t nb_cpus;

nits: needs a blank line here.

> +       if (!fwcfg_present)
> +               return 1;
> +
> +       qemu_fwcfg_read_entry(FW_CFG_NB_CPUS, 2, &nb_cpus);

No need to be16_to_cpu()? How do we know which has endian problem? Is
this documented somewhere in the QEMU doc?

nits: needs a blank line here.

> +       return nb_cpus;
> +}
> +
> +static int qemu_fwcfg_setup_kernel(void *load_addr)
> +{
> +       char *cmd_addr;

We can rename this to something like: char *data_addr = load_addr;
then (see below)

> +       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);
> +

No need to endian conversion for setup_size and kernel_size?

> +       if (setup_size == 0 || kernel_size == 0) {
> +               printf("warning: no kernel available\n");
> +               return -1;
> +       }
> +       qemu_fwcfg_read_entry(FW_CFG_SETUP_DATA, setup_size, load_addr);

load_addr -> data_addr

and do: data_addr += setup_size;

> +       qemu_fwcfg_read_entry(FW_CFG_KERNEL_DATA, kernel_size,
> +                             (char *)load_addr + setup_size);

load_addr + setup_size -> data_addr;

> +
> +       qemu_fwcfg_read_entry(FW_CFG_INITRD_SIZE, 4, &initrd_size);
> +       if (initrd_size == 0) {
> +               printf("warning: no initrd available\n");
> +       } else {

data_addr += kernel_size;

> +               qemu_fwcfg_read_entry(FW_CFG_INITRD_DATA, initrd_size,
> +                                     (char *)load_addr +
> +                                     setup_size + kernel_size);

load_addr + setup_size + kernel_size -> data_addr;

> +       }
> +
> +       qemu_fwcfg_read_entry(FW_CFG_CMDLINE_SIZE, 4, &cmdline_size);
> +       cmd_addr = (char *)load_addr + setup_size + kernel_size + initrd_size;

data_addr += initrd_size;

> +       qemu_fwcfg_read_entry(FW_CFG_CMDLINE_DATA, cmdline_size, cmd_addr);

cmd_addr -> data_addr;

> +
> +       printf("loading kernel to address %p", load_addr);
> +       if (initrd_size)
> +               printf(" initrd %p\n",
> +                      (char *)load_addr + setup_size + kernel_size);
> +       else
> +               printf("\n");
> +
> +       return setenv("bootargs", cmd_addr);
> +}
> +
> +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);

nits: needs a blank line here.

> +       return 0;
> +}
> +
> +void qemu_fwcfg_init(void)
> +{
> +       fwcfg_present = false;
> +       fwcfg_dma_present = false;
> +
> +       if (qemu_fwcfg_present()) {
> +               fwcfg_present = true;
> +               if (qemu_fwcfg_dma_present())
> +                       fwcfg_dma_present = true;
> +       }

This function can be simplified to:

    fwcfg_present = qemu_fwcfg_present();
    if (fwcfg_present)
        fwcfg_dma_present = qemu_fwcfg_dma_present();

> +}
> +
> +int do_qemu_fw(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +       void *load_addr;
> +
> +       if (!fwcfg_present) {
> +               printf("no qemu fw_cfg found\n");
> +               return 0;
> +       }
> +
> +       if (!strncmp(argv[1], "list", 4)) {
> +               qemu_fwcfg_list_firmware();
> +               return 0;
> +       } else if (!strncmp(argv[1], "cpus", 4)) {
> +               printf("%u\n", qemu_fwcfg_online_cpus());

Can we put more words here instead of just printing the cpu numbers?

> +               return 0;
> +       } else if (!strncmp(argv[1], "load", 4)) {
> +               if (argc == 3) {
> +                       load_addr = (void *)simple_strtoul(argv[2], NULL, 16);
> +               } else {
> +                       load_addr = getenv("loadaddr");
> +                       if (!load_addr)
> +                               load_addr = (void *)CONFIG_SYS_LOAD_ADDR;
> +                       else
> +                               load_addr = (void *)simple_strtoul(load_addr,
> +                                                                  NULL, 16);
> +               }
> +
> +               return qemu_fwcfg_setup_kernel(load_addr);
> +
> +       } else {
> +               return -1;
> +       }
> +

Can you rewrite the command logic utilizing find_cmd_tbl() and
cmd_process_error()? See arch/x86/lib/fsp/cmd_fsp.c for example.

> +       return 0;
> +}
> +
> +U_BOOT_CMD(
> +       fw,     3,      1,      do_qemu_fw,
> +       "qemu firmware interface",

nits: qemu -> QEMU

> +       "<command>\n"
> +       "    - list        : print firmware(s) currently loaded\n"
> +       "    - cpus        : print online cpu number\n"
> +       "    - load <addr> : load kernel (if any) to address <addr>, 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..66e0c8a
> --- /dev/null
> +++ b/arch/x86/cpu/qemu/fw_cfg.h
> @@ -0,0 +1,84 @@
> +/*
> + * (C) Copyright 2015 Miao Yan <yanmiaobest@gmail.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#ifndef __FW_CFG__
> +#define __FW_CFG__
> +
> +#include <common.h>
> +#include <asm/io.h>

Not needed. Please remove these two includes.

> +
> +#define FW_CONTROL_PORT         0x510
> +#define FW_DATA_PORT            0x511
> +#define FW_DMA_PORT             0x514
> +
> +#define FW_CFG_SIGNATURE        0x00
> +#define FW_CFG_ID               0x01
> +#define FW_CFG_UUID             0x02
> +#define FW_CFG_RAM_SIZE         0x03
> +#define FW_CFG_NOGRAPHIC        0x04
> +#define FW_CFG_NB_CPUS          0x05
> +#define FW_CFG_MACHINE_ID       0x06
> +#define FW_CFG_KERNEL_ADDR      0x07
> +#define FW_CFG_KERNEL_SIZE      0x08
> +#define FW_CFG_KERNEL_CMDLINE   0x09
> +#define FW_CFG_INITRD_ADDR      0x0a
> +#define FW_CFG_INITRD_SIZE      0x0b
> +#define FW_CFG_BOOT_DEVICE      0x0c
> +#define FW_CFG_NUMA             0x0d
> +#define FW_CFG_BOOT_MENU        0x0e
> +#define FW_CFG_MAX_CPUS         0x0f
> +#define FW_CFG_KERNEL_ENTRY     0x10
> +#define FW_CFG_KERNEL_DATA      0x11
> +#define FW_CFG_INITRD_DATA      0x12
> +#define FW_CFG_CMDLINE_ADDR     0x13
> +#define FW_CFG_CMDLINE_SIZE     0x14
> +#define FW_CFG_CMDLINE_DATA     0x15
> +#define FW_CFG_SETUP_ADDR       0x16
> +#define FW_CFG_SETUP_SIZE       0x17
> +#define FW_CFG_SETUP_DATA       0x18
> +#define FW_CFG_FILE_DIR         0x19

Can we use enum for the above entries?

> +
> +#define FW_CFG_FILE_FIRST       0x20
> +#define FW_CFG_FILE_SLOTS       0x10
> +#define FW_CFG_MAX_ENTRY        (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
> +
> +#define FW_CFG_WRITE_CHANNEL    0x4000
> +#define FW_CFG_ARCH_LOCAL       0x8000
> +#define FW_CFG_ENTRY_MASK       ~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL)
> +
> +#define FW_CFG_INVALID          0xffff
> +
> +#define FW_CFG_MAX_FILE_PATH    56
> +
> +#define QEMU_FW_CFG_SIGNATURE (('Q' << 24) | ('E' << 16) | ('M' << 8) | 'U')
> +
> +#define FW_CFG_DMA_ERROR  (0x1)
> +#define FW_CFG_DMA_READ   (0x2)
> +#define FW_CFG_DMA_SKIP   (0x4)
> +#define FW_CFG_DMA_SELECT (0x8)

For above 4, we can use (1 << 0) / (1 << 1) / (1 << 2) / (1 << 3) to
indicate they are bit definitions.

> +
> +struct fw_cfg_file {
> +       uint32_t size;
> +       uint16_t select;
> +       uint16_t reserved;
> +       char name[FW_CFG_MAX_FILE_PATH];
> +};
> +
> +struct fw_cfg_files {
> +       __be32 count;
> +       struct fw_cfg_file files[];

Ah, this fw_cfg interface is weird. count is big endian, but
fw_cfg_files is not.

> +};
> +
> +struct fw_cfg_dma_access {
> +       __be32 control;
> +       __be32 length;
> +       __be64 address;

These are big endians again. Weird.

> +};
> +
> +void qemu_fwcfg_init(void);
> +uint16_t qemu_fwcfg_online_cpus(void);

Can you add the comment header block for these two functions?

> +
> +#endif
> diff --git a/arch/x86/cpu/qemu/qemu.c b/arch/x86/cpu/qemu/qemu.c
> index 84fb082..c0a79d2 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

Hi Bin,

2015-12-29 14:19 GMT+08:00 Bin Meng <bmeng.cn@gmail.com>:
> Hi Miao,
>
> On Mon, Dec 28, 2015 at 5:18 PM, 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>
>> ---
>>  arch/x86/cpu/qemu/Makefile |   2 +-
>>  arch/x86/cpu/qemu/fw_cfg.c | 215 +++++++++++++++++++++++++++++++++++++++++++++
>>  arch/x86/cpu/qemu/fw_cfg.h |  84 ++++++++++++++++++
>>  arch/x86/cpu/qemu/qemu.c   |   3 +
>>  4 files changed, 303 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..e7615d1
>> --- /dev/null
>> +++ b/arch/x86/cpu/qemu/fw_cfg.c
>> @@ -0,0 +1,215 @@
>> +/*
>> + * (C) Copyright 2015 Miao Yan <yanmiaoebst@gmail.com>
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <command.h>
>> +#include <environment.h>
>> +#include <stdlib.h>
>
> environment.h and stdlib.h are not needed.
>
>> +#include <malloc.h>
>> +#include <errno.h>
>
> nits: move <error.h> to before <malloc.h>
>
>> +#include <fs.h>
>
> fs.h is not needed.
>
>> +#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,
>
> nits: please leave only one space between 'static' and 'void'
>
>> +               uint32_t size, void *address)
>> +{
>> +       uint32_t i = 0;
>> +       uint8_t *data = address;
>> +
>> +       if (entry != FW_CFG_INVALID)
>> +               outw(entry, FW_CONTROL_PORT);
>
> Missing branch to handle (entry == FW_CFG_INVALID).

This is on purpose. QEMU has a 'offset' associated with
the selected data items, writting 'entry' will reset
'offset' to zero. If you want to do more than one read
on a data item, then subsequent reads should pass 'FW_CFG_INVALID'
so it won't start reading from the beginning again.

But maybe it deserves a better interface. Any idea ?



>
>> +       while (size--)
>> +               data[i++] = inb(FW_DATA_PORT);
>> +}
>> +
>> +static  void qemu_fwcfg_read_entry_dma(uint16_t entry,
>
> nits: please leave only one space between 'static' and 'void'
>
>> +               uint32_t length, void *address)
>
> To keep consistency with qemu_fwcfg_read_entry_pio(), either change
> 'length' to 'size', or change 'size' parameter of
> qemu_fwcfg_read_entry_pio() to 'length'.
>
>> +{
>> +       struct fw_cfg_dma_access dma;
>> +
>> +       debug("qemu_fwcfg_dma_read_entry: addr %p, length %u control 0x%x\n",
>> +             address, length, dma.control);
>
> dma.control is not initialized yet. We need move this debug before
> "barrier()" below.
>
>> +
>> +       dma.length = cpu_to_be32(length);
>> +       dma.address = cpu_to_be64((uintptr_t)address);
>> +       dma.control = cpu_to_be32(FW_CFG_DMA_READ);
>> +       if (entry != FW_CFG_INVALID)
>> +               dma.control |= cpu_to_be32(FW_CFG_DMA_SELECT | (entry << 16));
>
> Missing branch to handle (entry == FW_CFG_INVALID).

See above.

>
>> +
>> +       barrier();
>> +
>> +       outl(cpu_to_be32((uint32_t)&dma), FW_DMA_PORT + 4);
>
> #define this "FW_DMA_PORT + 4" to a port macro?
>
>> +
>> +       while (dma.control & ~FW_CFG_DMA_ERROR)
>> +               __asm__ __volatile__ ("rep;nop");
>
> Just use "pause" instruction.
>
>> +}
>> +
>> +static int qemu_fwcfg_present(void)
>
> int -> bool
>
>> +{
>> +       uint32_t qemu;
>> +
>> +       qemu_fwcfg_read_entry_pio(FW_CFG_SIGNATURE, 4, &qemu);
>> +       return be32_to_cpu(qemu) == QEMU_FW_CFG_SIGNATURE;
>> +}
>> +
>> +static int qemu_fwcfg_dma_present(void)
>
> int -> bool
>
>> +{
>> +       uint8_t dma_enabled;
>> +       uint32_t qemu;
>> +
>> +       qemu_fwcfg_read_entry_pio(FW_CFG_ID, 1, &dma_enabled);
>> +       if (dma_enabled & 0x1) {
>
> #define 0x1 to something meaningful?
>
>> +               qemu = inl(FW_DMA_PORT);
>> +               if (be32_to_cpu(qemu) == QEMU_FW_CFG_SIGNATURE)
>> +                       return 1;
>> +       }
>> +       return 0;
>> +}
>> +
>> +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);
>> +}
>> +
>> +uint16_t qemu_fwcfg_online_cpus(void)
>
> Can we change the return value to int?
>
>> +{
>> +       uint16_t nb_cpus;
>
> nits: needs a blank line here.
>
>> +       if (!fwcfg_present)
>> +               return 1;
>> +
>> +       qemu_fwcfg_read_entry(FW_CFG_NB_CPUS, 2, &nb_cpus);
>
> No need to be16_to_cpu()? How do we know which has endian problem? Is
> this documented somewhere in the QEMU doc?
>
> nits: needs a blank line here.
>
>> +       return nb_cpus;
>> +}
>> +
>> +static int qemu_fwcfg_setup_kernel(void *load_addr)
>> +{
>> +       char *cmd_addr;
>
> We can rename this to something like: char *data_addr = load_addr;
> then (see below)
>
>> +       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);
>> +
>
> No need to endian conversion for setup_size and kernel_size?
>
>> +       if (setup_size == 0 || kernel_size == 0) {
>> +               printf("warning: no kernel available\n");
>> +               return -1;
>> +       }
>> +       qemu_fwcfg_read_entry(FW_CFG_SETUP_DATA, setup_size, load_addr);
>
> load_addr -> data_addr
>
> and do: data_addr += setup_size;
>
>> +       qemu_fwcfg_read_entry(FW_CFG_KERNEL_DATA, kernel_size,
>> +                             (char *)load_addr + setup_size);
>
> load_addr + setup_size -> data_addr;
>
>> +
>> +       qemu_fwcfg_read_entry(FW_CFG_INITRD_SIZE, 4, &initrd_size);
>> +       if (initrd_size == 0) {
>> +               printf("warning: no initrd available\n");
>> +       } else {
>
> data_addr += kernel_size;
>
>> +               qemu_fwcfg_read_entry(FW_CFG_INITRD_DATA, initrd_size,
>> +                                     (char *)load_addr +
>> +                                     setup_size + kernel_size);
>
> load_addr + setup_size + kernel_size -> data_addr;
>
>> +       }
>> +
>> +       qemu_fwcfg_read_entry(FW_CFG_CMDLINE_SIZE, 4, &cmdline_size);
>> +       cmd_addr = (char *)load_addr + setup_size + kernel_size + initrd_size;
>
> data_addr += initrd_size;
>
>> +       qemu_fwcfg_read_entry(FW_CFG_CMDLINE_DATA, cmdline_size, cmd_addr);
>
> cmd_addr -> data_addr;
>
>> +
>> +       printf("loading kernel to address %p", load_addr);
>> +       if (initrd_size)
>> +               printf(" initrd %p\n",
>> +                      (char *)load_addr + setup_size + kernel_size);
>> +       else
>> +               printf("\n");
>> +
>> +       return setenv("bootargs", cmd_addr);
>> +}
>> +
>> +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);
>
> nits: needs a blank line here.
>
>> +       return 0;
>> +}
>> +
>> +void qemu_fwcfg_init(void)
>> +{
>> +       fwcfg_present = false;
>> +       fwcfg_dma_present = false;
>> +
>> +       if (qemu_fwcfg_present()) {
>> +               fwcfg_present = true;
>> +               if (qemu_fwcfg_dma_present())
>> +                       fwcfg_dma_present = true;
>> +       }
>
> This function can be simplified to:
>
>     fwcfg_present = qemu_fwcfg_present();
>     if (fwcfg_present)
>         fwcfg_dma_present = qemu_fwcfg_dma_present();
>
>> +}
>> +
>> +int do_qemu_fw(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> +       void *load_addr;
>> +
>> +       if (!fwcfg_present) {
>> +               printf("no qemu fw_cfg found\n");
>> +               return 0;
>> +       }
>> +
>> +       if (!strncmp(argv[1], "list", 4)) {
>> +               qemu_fwcfg_list_firmware();
>> +               return 0;
>> +       } else if (!strncmp(argv[1], "cpus", 4)) {
>> +               printf("%u\n", qemu_fwcfg_online_cpus());
>
> Can we put more words here instead of just printing the cpu numbers?

What words do you have in mind ? "cpus number" ?



>
>> +               return 0;
>> +       } else if (!strncmp(argv[1], "load", 4)) {
>> +               if (argc == 3) {
>> +                       load_addr = (void *)simple_strtoul(argv[2], NULL, 16);
>> +               } else {
>> +                       load_addr = getenv("loadaddr");
>> +                       if (!load_addr)
>> +                               load_addr = (void *)CONFIG_SYS_LOAD_ADDR;
>> +                       else
>> +                               load_addr = (void *)simple_strtoul(load_addr,
>> +                                                                  NULL, 16);
>> +               }
>> +
>> +               return qemu_fwcfg_setup_kernel(load_addr);
>> +
>> +       } else {
>> +               return -1;
>> +       }
>> +
>
> Can you rewrite the command logic utilizing find_cmd_tbl() and
> cmd_process_error()? See arch/x86/lib/fsp/cmd_fsp.c for example.
>
>> +       return 0;
>> +}
>> +
>> +U_BOOT_CMD(
>> +       fw,     3,      1,      do_qemu_fw,
>> +       "qemu firmware interface",
>
> nits: qemu -> QEMU
>
>> +       "<command>\n"
>> +       "    - list        : print firmware(s) currently loaded\n"
>> +       "    - cpus        : print online cpu number\n"
>> +       "    - load <addr> : load kernel (if any) to address <addr>, 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..66e0c8a
>> --- /dev/null
>> +++ b/arch/x86/cpu/qemu/fw_cfg.h
>> @@ -0,0 +1,84 @@
>> +/*
>> + * (C) Copyright 2015 Miao Yan <yanmiaobest@gmail.com>
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#ifndef __FW_CFG__
>> +#define __FW_CFG__
>> +
>> +#include <common.h>
>> +#include <asm/io.h>
>
> Not needed. Please remove these two includes.
>
>> +
>> +#define FW_CONTROL_PORT         0x510
>> +#define FW_DATA_PORT            0x511
>> +#define FW_DMA_PORT             0x514
>> +
>> +#define FW_CFG_SIGNATURE        0x00
>> +#define FW_CFG_ID               0x01
>> +#define FW_CFG_UUID             0x02
>> +#define FW_CFG_RAM_SIZE         0x03
>> +#define FW_CFG_NOGRAPHIC        0x04
>> +#define FW_CFG_NB_CPUS          0x05
>> +#define FW_CFG_MACHINE_ID       0x06
>> +#define FW_CFG_KERNEL_ADDR      0x07
>> +#define FW_CFG_KERNEL_SIZE      0x08
>> +#define FW_CFG_KERNEL_CMDLINE   0x09
>> +#define FW_CFG_INITRD_ADDR      0x0a
>> +#define FW_CFG_INITRD_SIZE      0x0b
>> +#define FW_CFG_BOOT_DEVICE      0x0c
>> +#define FW_CFG_NUMA             0x0d
>> +#define FW_CFG_BOOT_MENU        0x0e
>> +#define FW_CFG_MAX_CPUS         0x0f
>> +#define FW_CFG_KERNEL_ENTRY     0x10
>> +#define FW_CFG_KERNEL_DATA      0x11
>> +#define FW_CFG_INITRD_DATA      0x12
>> +#define FW_CFG_CMDLINE_ADDR     0x13
>> +#define FW_CFG_CMDLINE_SIZE     0x14
>> +#define FW_CFG_CMDLINE_DATA     0x15
>> +#define FW_CFG_SETUP_ADDR       0x16
>> +#define FW_CFG_SETUP_SIZE       0x17
>> +#define FW_CFG_SETUP_DATA       0x18
>> +#define FW_CFG_FILE_DIR         0x19
>
> Can we use enum for the above entries?
>
>> +
>> +#define FW_CFG_FILE_FIRST       0x20
>> +#define FW_CFG_FILE_SLOTS       0x10
>> +#define FW_CFG_MAX_ENTRY        (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
>> +
>> +#define FW_CFG_WRITE_CHANNEL    0x4000
>> +#define FW_CFG_ARCH_LOCAL       0x8000
>> +#define FW_CFG_ENTRY_MASK       ~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL)
>> +
>> +#define FW_CFG_INVALID          0xffff
>> +
>> +#define FW_CFG_MAX_FILE_PATH    56
>> +
>> +#define QEMU_FW_CFG_SIGNATURE (('Q' << 24) | ('E' << 16) | ('M' << 8) | 'U')
>> +
>> +#define FW_CFG_DMA_ERROR  (0x1)
>> +#define FW_CFG_DMA_READ   (0x2)
>> +#define FW_CFG_DMA_SKIP   (0x4)
>> +#define FW_CFG_DMA_SELECT (0x8)
>
> For above 4, we can use (1 << 0) / (1 << 1) / (1 << 2) / (1 << 3) to
> indicate they are bit definitions.
>
>> +
>> +struct fw_cfg_file {
>> +       uint32_t size;
>> +       uint16_t select;
>> +       uint16_t reserved;
>> +       char name[FW_CFG_MAX_FILE_PATH];
>> +};
>> +
>> +struct fw_cfg_files {
>> +       __be32 count;
>> +       struct fw_cfg_file files[];
>
> Ah, this fw_cfg interface is weird. count is big endian, but
> fw_cfg_files is not.


fw_cfg_file is big endian. I forgot to change them. Will fix.


>
>> +};
>> +
>> +struct fw_cfg_dma_access {
>> +       __be32 control;
>> +       __be32 length;
>> +       __be64 address;
>
> These are big endians again. Weird.
>
>> +};
>> +
>> +void qemu_fwcfg_init(void);
>> +uint16_t qemu_fwcfg_online_cpus(void);
>
> Can you add the comment header block for these two functions?
>
>> +
>> +#endif
>> diff --git a/arch/x86/cpu/qemu/qemu.c b/arch/x86/cpu/qemu/qemu.c
>> index 84fb082..c0a79d2 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

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

Miao

Hi Miao,

On Tue, Dec 29, 2015 at 2:41 PM, Miao Yan <yanmiaobest@gmail.com> wrote:
> Hi Bin,
>
> 2015-12-29 14:19 GMT+08:00 Bin Meng <bmeng.cn@gmail.com>:
>> Hi Miao,
>>
>> On Mon, Dec 28, 2015 at 5:18 PM, 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>
>>> ---
>>>  arch/x86/cpu/qemu/Makefile |   2 +-
>>>  arch/x86/cpu/qemu/fw_cfg.c | 215 +++++++++++++++++++++++++++++++++++++++++++++
>>>  arch/x86/cpu/qemu/fw_cfg.h |  84 ++++++++++++++++++
>>>  arch/x86/cpu/qemu/qemu.c   |   3 +
>>>  4 files changed, 303 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..e7615d1
>>> --- /dev/null
>>> +++ b/arch/x86/cpu/qemu/fw_cfg.c
>>> @@ -0,0 +1,215 @@
>>> +/*
>>> + * (C) Copyright 2015 Miao Yan <yanmiaoebst@gmail.com>
>>> + *
>>> + * SPDX-License-Identifier:    GPL-2.0+
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <command.h>
>>> +#include <environment.h>
>>> +#include <stdlib.h>
>>
>> environment.h and stdlib.h are not needed.
>>
>>> +#include <malloc.h>
>>> +#include <errno.h>
>>
>> nits: move <error.h> to before <malloc.h>
>>
>>> +#include <fs.h>
>>
>> fs.h is not needed.
>>
>>> +#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,
>>
>> nits: please leave only one space between 'static' and 'void'
>>
>>> +               uint32_t size, void *address)
>>> +{
>>> +       uint32_t i = 0;
>>> +       uint8_t *data = address;
>>> +
>>> +       if (entry != FW_CFG_INVALID)
>>> +               outw(entry, FW_CONTROL_PORT);
>>
>> Missing branch to handle (entry == FW_CFG_INVALID).
>
> This is on purpose. QEMU has a 'offset' associated with
> the selected data items, writting 'entry' will reset
> 'offset' to zero. If you want to do more than one read
> on a data item, then subsequent reads should pass 'FW_CFG_INVALID'
> so it won't start reading from the beginning again.
>
> But maybe it deserves a better interface. Any idea ?

OK, this makes sense. Can you please add a comment block here?

>
>
>
>>
>>> +       while (size--)
>>> +               data[i++] = inb(FW_DATA_PORT);
>>> +}
>>> +
>>> +static  void qemu_fwcfg_read_entry_dma(uint16_t entry,
>>
>> nits: please leave only one space between 'static' and 'void'
>>
>>> +               uint32_t length, void *address)
>>
>> To keep consistency with qemu_fwcfg_read_entry_pio(), either change
>> 'length' to 'size', or change 'size' parameter of
>> qemu_fwcfg_read_entry_pio() to 'length'.
>>
>>> +{
>>> +       struct fw_cfg_dma_access dma;
>>> +
>>> +       debug("qemu_fwcfg_dma_read_entry: addr %p, length %u control 0x%x\n",
>>> +             address, length, dma.control);
>>
>> dma.control is not initialized yet. We need move this debug before
>> "barrier()" below.
>>
>>> +
>>> +       dma.length = cpu_to_be32(length);
>>> +       dma.address = cpu_to_be64((uintptr_t)address);
>>> +       dma.control = cpu_to_be32(FW_CFG_DMA_READ);
>>> +       if (entry != FW_CFG_INVALID)
>>> +               dma.control |= cpu_to_be32(FW_CFG_DMA_SELECT | (entry << 16));
>>
>> Missing branch to handle (entry == FW_CFG_INVALID).
>
> See above.
>
>>
>>> +
>>> +       barrier();
>>> +
>>> +       outl(cpu_to_be32((uint32_t)&dma), FW_DMA_PORT + 4);
>>
>> #define this "FW_DMA_PORT + 4" to a port macro?
>>
>>> +
>>> +       while (dma.control & ~FW_CFG_DMA_ERROR)
>>> +               __asm__ __volatile__ ("rep;nop");
>>
>> Just use "pause" instruction.
>>
>>> +}
>>> +
>>> +static int qemu_fwcfg_present(void)
>>
>> int -> bool
>>
>>> +{
>>> +       uint32_t qemu;
>>> +
>>> +       qemu_fwcfg_read_entry_pio(FW_CFG_SIGNATURE, 4, &qemu);
>>> +       return be32_to_cpu(qemu) == QEMU_FW_CFG_SIGNATURE;
>>> +}
>>> +
>>> +static int qemu_fwcfg_dma_present(void)
>>
>> int -> bool
>>
>>> +{
>>> +       uint8_t dma_enabled;
>>> +       uint32_t qemu;
>>> +
>>> +       qemu_fwcfg_read_entry_pio(FW_CFG_ID, 1, &dma_enabled);
>>> +       if (dma_enabled & 0x1) {
>>
>> #define 0x1 to something meaningful?
>>
>>> +               qemu = inl(FW_DMA_PORT);
>>> +               if (be32_to_cpu(qemu) == QEMU_FW_CFG_SIGNATURE)
>>> +                       return 1;
>>> +       }
>>> +       return 0;
>>> +}
>>> +
>>> +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);
>>> +}
>>> +
>>> +uint16_t qemu_fwcfg_online_cpus(void)
>>
>> Can we change the return value to int?
>>
>>> +{
>>> +       uint16_t nb_cpus;
>>
>> nits: needs a blank line here.
>>
>>> +       if (!fwcfg_present)
>>> +               return 1;
>>> +
>>> +       qemu_fwcfg_read_entry(FW_CFG_NB_CPUS, 2, &nb_cpus);
>>
>> No need to be16_to_cpu()? How do we know which has endian problem? Is
>> this documented somewhere in the QEMU doc?
>>
>> nits: needs a blank line here.
>>
>>> +       return nb_cpus;
>>> +}
>>> +
>>> +static int qemu_fwcfg_setup_kernel(void *load_addr)
>>> +{
>>> +       char *cmd_addr;
>>
>> We can rename this to something like: char *data_addr = load_addr;
>> then (see below)
>>
>>> +       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);
>>> +
>>
>> No need to endian conversion for setup_size and kernel_size?
>>
>>> +       if (setup_size == 0 || kernel_size == 0) {
>>> +               printf("warning: no kernel available\n");
>>> +               return -1;
>>> +       }
>>> +       qemu_fwcfg_read_entry(FW_CFG_SETUP_DATA, setup_size, load_addr);
>>
>> load_addr -> data_addr
>>
>> and do: data_addr += setup_size;
>>
>>> +       qemu_fwcfg_read_entry(FW_CFG_KERNEL_DATA, kernel_size,
>>> +                             (char *)load_addr + setup_size);
>>
>> load_addr + setup_size -> data_addr;
>>
>>> +
>>> +       qemu_fwcfg_read_entry(FW_CFG_INITRD_SIZE, 4, &initrd_size);
>>> +       if (initrd_size == 0) {
>>> +               printf("warning: no initrd available\n");
>>> +       } else {
>>
>> data_addr += kernel_size;
>>
>>> +               qemu_fwcfg_read_entry(FW_CFG_INITRD_DATA, initrd_size,
>>> +                                     (char *)load_addr +
>>> +                                     setup_size + kernel_size);
>>
>> load_addr + setup_size + kernel_size -> data_addr;
>>
>>> +       }
>>> +
>>> +       qemu_fwcfg_read_entry(FW_CFG_CMDLINE_SIZE, 4, &cmdline_size);
>>> +       cmd_addr = (char *)load_addr + setup_size + kernel_size + initrd_size;
>>
>> data_addr += initrd_size;
>>
>>> +       qemu_fwcfg_read_entry(FW_CFG_CMDLINE_DATA, cmdline_size, cmd_addr);
>>
>> cmd_addr -> data_addr;
>>
>>> +
>>> +       printf("loading kernel to address %p", load_addr);
>>> +       if (initrd_size)
>>> +               printf(" initrd %p\n",
>>> +                      (char *)load_addr + setup_size + kernel_size);
>>> +       else
>>> +               printf("\n");
>>> +
>>> +       return setenv("bootargs", cmd_addr);
>>> +}
>>> +
>>> +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);
>>
>> nits: needs a blank line here.
>>
>>> +       return 0;
>>> +}
>>> +
>>> +void qemu_fwcfg_init(void)
>>> +{
>>> +       fwcfg_present = false;
>>> +       fwcfg_dma_present = false;
>>> +
>>> +       if (qemu_fwcfg_present()) {
>>> +               fwcfg_present = true;
>>> +               if (qemu_fwcfg_dma_present())
>>> +                       fwcfg_dma_present = true;
>>> +       }
>>
>> This function can be simplified to:
>>
>>     fwcfg_present = qemu_fwcfg_present();
>>     if (fwcfg_present)
>>         fwcfg_dma_present = qemu_fwcfg_dma_present();
>>
>>> +}
>>> +
>>> +int do_qemu_fw(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>> +{
>>> +       void *load_addr;
>>> +
>>> +       if (!fwcfg_present) {
>>> +               printf("no qemu fw_cfg found\n");
>>> +               return 0;
>>> +       }
>>> +
>>> +       if (!strncmp(argv[1], "list", 4)) {
>>> +               qemu_fwcfg_list_firmware();
>>> +               return 0;
>>> +       } else if (!strncmp(argv[1], "cpus", 4)) {
>>> +               printf("%u\n", qemu_fwcfg_online_cpus());
>>
>> Can we put more words here instead of just printing the cpu numbers?
>
> What words do you have in mind ? "cpus number" ?
>
>

Maybe: %u cpus online?

>
>>
>>> +               return 0;
>>> +       } else if (!strncmp(argv[1], "load", 4)) {
>>> +               if (argc == 3) {
>>> +                       load_addr = (void *)simple_strtoul(argv[2], NULL, 16);
>>> +               } else {
>>> +                       load_addr = getenv("loadaddr");
>>> +                       if (!load_addr)
>>> +                               load_addr = (void *)CONFIG_SYS_LOAD_ADDR;
>>> +                       else
>>> +                               load_addr = (void *)simple_strtoul(load_addr,
>>> +                                                                  NULL, 16);
>>> +               }
>>> +
>>> +               return qemu_fwcfg_setup_kernel(load_addr);
>>> +
>>> +       } else {
>>> +               return -1;
>>> +       }
>>> +
>>
>> Can you rewrite the command logic utilizing find_cmd_tbl() and
>> cmd_process_error()? See arch/x86/lib/fsp/cmd_fsp.c for example.
>>
>>> +       return 0;
>>> +}
>>> +
>>> +U_BOOT_CMD(
>>> +       fw,     3,      1,      do_qemu_fw,
>>> +       "qemu firmware interface",
>>
>> nits: qemu -> QEMU
>>
>>> +       "<command>\n"
>>> +       "    - list        : print firmware(s) currently loaded\n"
>>> +       "    - cpus        : print online cpu number\n"
>>> +       "    - load <addr> : load kernel (if any) to address <addr>, 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..66e0c8a
>>> --- /dev/null
>>> +++ b/arch/x86/cpu/qemu/fw_cfg.h
>>> @@ -0,0 +1,84 @@
>>> +/*
>>> + * (C) Copyright 2015 Miao Yan <yanmiaobest@gmail.com>
>>> + *
>>> + * SPDX-License-Identifier:    GPL-2.0+
>>> + */
>>> +
>>> +#ifndef __FW_CFG__
>>> +#define __FW_CFG__
>>> +
>>> +#include <common.h>
>>> +#include <asm/io.h>
>>
>> Not needed. Please remove these two includes.
>>
>>> +
>>> +#define FW_CONTROL_PORT         0x510
>>> +#define FW_DATA_PORT            0x511
>>> +#define FW_DMA_PORT             0x514
>>> +
>>> +#define FW_CFG_SIGNATURE        0x00
>>> +#define FW_CFG_ID               0x01
>>> +#define FW_CFG_UUID             0x02
>>> +#define FW_CFG_RAM_SIZE         0x03
>>> +#define FW_CFG_NOGRAPHIC        0x04
>>> +#define FW_CFG_NB_CPUS          0x05
>>> +#define FW_CFG_MACHINE_ID       0x06
>>> +#define FW_CFG_KERNEL_ADDR      0x07
>>> +#define FW_CFG_KERNEL_SIZE      0x08
>>> +#define FW_CFG_KERNEL_CMDLINE   0x09
>>> +#define FW_CFG_INITRD_ADDR      0x0a
>>> +#define FW_CFG_INITRD_SIZE      0x0b
>>> +#define FW_CFG_BOOT_DEVICE      0x0c
>>> +#define FW_CFG_NUMA             0x0d
>>> +#define FW_CFG_BOOT_MENU        0x0e
>>> +#define FW_CFG_MAX_CPUS         0x0f
>>> +#define FW_CFG_KERNEL_ENTRY     0x10
>>> +#define FW_CFG_KERNEL_DATA      0x11
>>> +#define FW_CFG_INITRD_DATA      0x12
>>> +#define FW_CFG_CMDLINE_ADDR     0x13
>>> +#define FW_CFG_CMDLINE_SIZE     0x14
>>> +#define FW_CFG_CMDLINE_DATA     0x15
>>> +#define FW_CFG_SETUP_ADDR       0x16
>>> +#define FW_CFG_SETUP_SIZE       0x17
>>> +#define FW_CFG_SETUP_DATA       0x18
>>> +#define FW_CFG_FILE_DIR         0x19
>>
>> Can we use enum for the above entries?
>>
>>> +
>>> +#define FW_CFG_FILE_FIRST       0x20
>>> +#define FW_CFG_FILE_SLOTS       0x10
>>> +#define FW_CFG_MAX_ENTRY        (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
>>> +
>>> +#define FW_CFG_WRITE_CHANNEL    0x4000
>>> +#define FW_CFG_ARCH_LOCAL       0x8000
>>> +#define FW_CFG_ENTRY_MASK       ~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL)
>>> +
>>> +#define FW_CFG_INVALID          0xffff
>>> +
>>> +#define FW_CFG_MAX_FILE_PATH    56
>>> +
>>> +#define QEMU_FW_CFG_SIGNATURE (('Q' << 24) | ('E' << 16) | ('M' << 8) | 'U')
>>> +
>>> +#define FW_CFG_DMA_ERROR  (0x1)
>>> +#define FW_CFG_DMA_READ   (0x2)
>>> +#define FW_CFG_DMA_SKIP   (0x4)
>>> +#define FW_CFG_DMA_SELECT (0x8)
>>
>> For above 4, we can use (1 << 0) / (1 << 1) / (1 << 2) / (1 << 3) to
>> indicate they are bit definitions.
>>
>>> +
>>> +struct fw_cfg_file {
>>> +       uint32_t size;
>>> +       uint16_t select;
>>> +       uint16_t reserved;
>>> +       char name[FW_CFG_MAX_FILE_PATH];
>>> +};
>>> +
>>> +struct fw_cfg_files {
>>> +       __be32 count;
>>> +       struct fw_cfg_file files[];
>>
>> Ah, this fw_cfg interface is weird. count is big endian, but
>> fw_cfg_files is not.
>
>
> fw_cfg_file is big endian. I forgot to change them. Will fix.
>
>
>>
>>> +};
>>> +
>>> +struct fw_cfg_dma_access {
>>> +       __be32 control;
>>> +       __be32 length;
>>> +       __be64 address;
>>
>> These are big endians again. Weird.
>>
>>> +};
>>> +
>>> +void qemu_fwcfg_init(void);
>>> +uint16_t qemu_fwcfg_online_cpus(void);
>>
>> Can you add the comment header block for these two functions?
>>
>>> +
>>> +#endif
>>> diff --git a/arch/x86/cpu/qemu/qemu.c b/arch/x86/cpu/qemu/qemu.c
>>> index 84fb082..c0a79d2 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
>
> I'll fix the rest of your comments. Thanks for the review.
>

Regards,
Bin