[U-Boot] [PATCH 1/7] x86: qemu: add fw_cfg support
Miao Yan
yanmiaobest at gmail.com
Tue Dec 29 07:41:28 CET 2015
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 ?
>
>> + 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 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.
Miao
More information about the U-Boot
mailing list