Hi Miao, On Tue, Dec 29, 2015 at 2:41 PM, Miao Yan <yanmiaob...@gmail.com> wrote: > Hi Bin, > > 2015-12-29 14:19 GMT+08:00 Bin Meng <bmeng...@gmail.com>: >> Hi Miao, >> >> On Mon, Dec 28, 2015 at 5:18 PM, 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> >>> --- >>> 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 <yanmiaoe...@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 <yanmiaob...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot