Hi Simon, 2015-12-31 13:07 GMT+08:00 Simon Glass <s...@chromium.org>: > Hi Miao, > > On 30 December 2015 at 19:55, Miao Yan <yanmiaob...@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 <yanmiaob...@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 <s...@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 <yanmiaoe...@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 <yanmiaob...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot