[U-Boot] [PATCH 1/7] x86: qemu: add fw_cfg support

Bin Meng bmeng.cn at gmail.com
Tue Dec 29 07:57:39 CET 2015


Hi Miao,

On Tue, Dec 29, 2015 at 2:41 PM, Miao Yan <yanmiaobest at gmail.com> wrote:
> Hi Bin,
>
> 2015-12-29 14:19 GMT+08:00 Bin Meng <bmeng.cn at gmail.com>:
>> Hi Miao,
>>
>> On Mon, Dec 28, 2015 at 5:18 PM, Miao Yan <yanmiaobest at 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 at 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 at 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 at 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


More information about the U-Boot mailing list