Hi Bin,
2015-12-29 14:19 GMT+08:00 Bin Meng bmeng.cn@gmail.com:
Hi Miao,
On Mon, Dec 28, 2015 at 5:18 PM, Miao Yan yanmiaobest@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@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@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);elseqemu_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);elseprintf("\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;elseload_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@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