[U-Boot] [PATCH 1/7] x86: qemu: add fw_cfg support
Bin Meng
bmeng.cn at gmail.com
Tue Dec 29 07:57:39 CET 2015
Hi Miao,
On Tue, Dec 29, 2015 at 2:41 PM, Miao Yan <yanmiaobest at gmail.com> wrote:
> 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 ?
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 <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.
>
Regards,
Bin
More information about the U-Boot
mailing list