[U-Boot] [PATCH 1/7] x86: qemu: add fw_cfg support
Bin Meng
bmeng.cn at gmail.com
Tue Dec 29 07:19:04 CET 2015
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).
> + 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 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.
> +};
> +
> +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
More information about the U-Boot
mailing list