Changeset View
Standalone View
usr.sbin/bhyve/bootrom.c
| Show All 33 Lines | ||||||||||||||||||
| #include <sys/stat.h> | #include <sys/stat.h> | |||||||||||||||||
| #include <machine/vmm.h> | #include <machine/vmm.h> | |||||||||||||||||
| #include <err.h> | #include <err.h> | |||||||||||||||||
| #include <errno.h> | #include <errno.h> | |||||||||||||||||
| #include <fcntl.h> | #include <fcntl.h> | |||||||||||||||||
| #include <stdio.h> | #include <stdio.h> | |||||||||||||||||
| #include <stdlib.h> | ||||||||||||||||||
| #include <string.h> | #include <string.h> | |||||||||||||||||
| #include <unistd.h> | #include <unistd.h> | |||||||||||||||||
| #include <stdbool.h> | #include <stdbool.h> | |||||||||||||||||
| #include <vmmapi.h> | #include <vmmapi.h> | |||||||||||||||||
| #include "bhyverun.h" | #include "bhyverun.h" | |||||||||||||||||
| #include "bootrom.h" | #include "bootrom.h" | |||||||||||||||||
| #include "debug.h" | #include "debug.h" | |||||||||||||||||
| #include "mem.h" | ||||||||||||||||||
| #define BOOTROM_SIZE (16 * 1024 * 1024) /* 16 MB */ | #define BOOTROM_SIZE (16 * 1024 * 1024) /* 16 MB */ | |||||||||||||||||
| /* | /* | |||||||||||||||||
| * ROM region is 16 MB at the top of 4GB ("low") memory. | * ROM region is 16 MB at the top of 4GB ("low") memory. | |||||||||||||||||
| * | * | |||||||||||||||||
| * The size is limited so it doesn't encroach into reserved MMIO space (e.g., | * The size is limited so it doesn't encroach into reserved MMIO space (e.g., | |||||||||||||||||
| * APIC, HPET, MSI). | * APIC, HPET, MSI). | |||||||||||||||||
| * | * | |||||||||||||||||
| * It is allocated in page-multiple blocks on a first-come first-serve basis, | * It is allocated in page-multiple blocks on a first-come first-serve basis, | |||||||||||||||||
| * from high to low, during initialization, and does not change at runtime. | * from high to low, during initialization, and does not change at runtime. | |||||||||||||||||
| */ | */ | |||||||||||||||||
| static char *romptr; /* Pointer to userspace-mapped bootrom region. */ | static char *romptr; /* Pointer to userspace-mapped bootrom region. */ | |||||||||||||||||
rgrimes: The next two are initialized in the code, which is the prefered form rather than doing it in… | ||||||||||||||||||
Done Inline ActionsThis was a static function-scoped variable in the last iteration. I've changed it to be part of the state struct now with the other state, because adding bounds checking caused the function to outgrow the two general purpose arguments that a handler function gets. scottph: This was a static function-scoped variable in the last iteration. I've changed it to be part of… | ||||||||||||||||||
| static vm_paddr_t gpa_base; /* GPA of low end of region. */ | static vm_paddr_t gpa_base; /* GPA of low end of region. */ | |||||||||||||||||
| static vm_paddr_t gpa_allocbot; /* Low GPA of free region. */ | static vm_paddr_t gpa_allocbot; /* Low GPA of free region. */ | |||||||||||||||||
| static vm_paddr_t gpa_alloctop; /* High GPA, minus 1, of free region. */ | static vm_paddr_t gpa_alloctop; /* High GPA, minus 1, of free region. */ | |||||||||||||||||
| #define CFI_BCS_WRITE_BYTE 0x10 | ||||||||||||||||||
| #define CFI_BCS_CLEAR_STATUS 0x50 | ||||||||||||||||||
| #define CFI_BCS_READ_STATUS 0x70 | ||||||||||||||||||
Done Inline ActionsWe know cmd == CFI_BCS_READ_ARRAY when we reach this line, why do we switch on it? rgrimes: We know cmd == CFI_BCS_READ_ARRAY when we reach this line, why do we switch on it? | ||||||||||||||||||
Done Inline ActionsThis will vary depending on which commands are sent. scottph: This will vary depending on which commands are sent. | ||||||||||||||||||
| #define CFI_BCS_READ_ARRAY 0xff | ||||||||||||||||||
| static struct bootrom_var_state { | ||||||||||||||||||
| uint8_t *mmap; | ||||||||||||||||||
| uint64_t gpa; | ||||||||||||||||||
| off_t size; | ||||||||||||||||||
| uint8_t cmd; | ||||||||||||||||||
| } var = { NULL, 0, 0, CFI_BCS_READ_ARRAY }; | ||||||||||||||||||
| /* | ||||||||||||||||||
| * Emulate just those CFI basic commands that will convince EDK II | ||||||||||||||||||
| * that the Firmware Volume area is writable and persistent. | ||||||||||||||||||
| */ | ||||||||||||||||||
| static int | ||||||||||||||||||
| bootrom_var_mem_handler(struct vmctx *ctx, int vcpu, int dir, uint64_t addr, | ||||||||||||||||||
| int size, uint64_t *val, void *arg1, long arg2) | ||||||||||||||||||
| { | ||||||||||||||||||
| off_t offset; | ||||||||||||||||||
Done Inline ActionsThe logic in this function seems to be confusing. dir seems to be the controlling parameter, when I would expect cmd to be, default: cases are used to override the cmd in one place, and to assume a read operation for all others? This seems that errors are not actually checked for and returned, ie the function always returns 0 rgrimes: The logic in this function seems to be confusing. dir seems to be the controlling parameter… | ||||||||||||||||||
Done Inline ActionsI looked at the restructuring you suggested but I think this is the more clear way to write the handling logic. As a first decision, (1) are we being given a new command, or are we servicing a read request under the existing command. Regarding error checking, addr is guaranteed to be in bounds by the logic in mem.c, but I think it is possible for size to go past the end of our range, so I've added a check for that. scottph: I looked at the restructuring you suggested but I think this is the more clear way to write the… | ||||||||||||||||||
| offset = addr - var.gpa; | ||||||||||||||||||
| if (offset + size > var.size || offset < 0 || offset + size <= offset) | ||||||||||||||||||
| return (EINVAL); | ||||||||||||||||||
| if (dir == MEM_F_WRITE) { | ||||||||||||||||||
| switch (var.cmd) { | ||||||||||||||||||
| case CFI_BCS_WRITE_BYTE: | ||||||||||||||||||
| memcpy(var.mmap + offset, val, size); | ||||||||||||||||||
| var.cmd = CFI_BCS_READ_ARRAY; | ||||||||||||||||||
| break; | ||||||||||||||||||
| default: | ||||||||||||||||||
| var.cmd = *(uint8_t *)val; | ||||||||||||||||||
| } | ||||||||||||||||||
| } else { | ||||||||||||||||||
| switch (var.cmd) { | ||||||||||||||||||
| case CFI_BCS_CLEAR_STATUS: | ||||||||||||||||||
| case CFI_BCS_READ_STATUS: | ||||||||||||||||||
| memset(val, 0, size); | ||||||||||||||||||
| var.cmd = CFI_BCS_READ_ARRAY; | ||||||||||||||||||
| break; | ||||||||||||||||||
| default: | ||||||||||||||||||
| memcpy(val, var.mmap + offset, size); | ||||||||||||||||||
| break; | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| return (0); | ||||||||||||||||||
| } | ||||||||||||||||||
| void | void | |||||||||||||||||
| init_bootrom(struct vmctx *ctx) | init_bootrom(struct vmctx *ctx) | |||||||||||||||||
| { | { | |||||||||||||||||
| romptr = vm_create_devmem(ctx, VM_BOOTROM, "bootrom", BOOTROM_SIZE); | romptr = vm_create_devmem(ctx, VM_BOOTROM, "bootrom", BOOTROM_SIZE); | |||||||||||||||||
| if (romptr == MAP_FAILED) | if (romptr == MAP_FAILED) | |||||||||||||||||
| err(4, "%s: vm_create_devmem", __func__); | err(4, "%s: vm_create_devmem", __func__); | |||||||||||||||||
| gpa_base = (1ULL << 32) - BOOTROM_SIZE; | gpa_base = (1ULL << 32) - BOOTROM_SIZE; | |||||||||||||||||
| gpa_allocbot = gpa_base; | gpa_allocbot = gpa_base; | |||||||||||||||||
| gpa_alloctop = (1ULL << 32) - 1; | gpa_alloctop = (1ULL << 32) - 1; | |||||||||||||||||
| } | } | |||||||||||||||||
| int | int | |||||||||||||||||
Done Inline ActionsThis function is long enough it should have a block comment that describes what it does, what the inputs are and what return values/error conditions exist. rgrimes: This function is long enough it should have a block comment that describes what it does, what… | ||||||||||||||||||
| bootrom_alloc(struct vmctx *ctx, size_t len, int prot, int flags, | bootrom_alloc(struct vmctx *ctx, size_t len, int prot, int flags, | |||||||||||||||||
| char **region_out, uint64_t *gpa_out) | char **region_out, uint64_t *gpa_out) | |||||||||||||||||
| { | { | |||||||||||||||||
| static const int bootrom_valid_flags = BOOTROM_ALLOC_TOP; | static const int bootrom_valid_flags = BOOTROM_ALLOC_TOP; | |||||||||||||||||
| vm_paddr_t gpa; | vm_paddr_t gpa; | |||||||||||||||||
| vm_ooffset_t segoff; | vm_ooffset_t segoff; | |||||||||||||||||
| ▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Lines | bootrom_alloc(struct vmctx *ctx, size_t len, int prot, int flags, | |||||||||||||||||
| return (0); | return (0); | |||||||||||||||||
| } | } | |||||||||||||||||
| int | int | |||||||||||||||||
| bootrom_loadrom(struct vmctx *ctx, const char *romfile) | bootrom_loadrom(struct vmctx *ctx, const char *romfile) | |||||||||||||||||
| { | { | |||||||||||||||||
| struct stat sbuf; | struct stat sbuf; | |||||||||||||||||
| ssize_t rlen; | ssize_t rlen; | |||||||||||||||||
| char *ptr; | off_t rom_size, var_size, total_size; | |||||||||||||||||
| int fd, i, rv; | char *ptr, *varfile; | |||||||||||||||||
| int fd, varfd, i, rv; | ||||||||||||||||||
Done Inline Actionsstyle(9) ordering, char/uchar should be after int. rgrimes: style(9) ordering, char/uchar should be after int. | ||||||||||||||||||
Done Inline ActionsI believe this ordering agrees with style(9). cf https://svnweb.freebsd.org/base/head/share/man/man9/style.9?revision=340033&view=markup#l481 scottph: I believe this ordering agrees with style(9). cf https://svnweb.freebsd. | ||||||||||||||||||
Done Inline ActionsAt line 660 of style: When declaring variables in functions declare them sorted by size, rgrimes: At line 660 of style: When declaring variables in functions declare them sorted by size,
then… | ||||||||||||||||||
Done Inline Actionsah, I believe you may have missed that those are char pointers in your first reading, where sizeof(char *) == 8, sizeof(int) == 4. cf line 483 of style(9). scottph: ah, I believe you may have missed that those are char pointers in your first reading, where… | ||||||||||||||||||
Done Inline ActionsIneeded I did, so this is fine, sorry for the noise. rgrimes: Ineeded I did, so this is fine, sorry for the noise. | ||||||||||||||||||
| rv = -1; | rv = -1; | |||||||||||||||||
| varfd = -1; | ||||||||||||||||||
| varfile = strdup(romfile); | ||||||||||||||||||
| romfile = strsep(&varfile, ","); | ||||||||||||||||||
| fd = open(romfile, O_RDONLY); | fd = open(romfile, O_RDONLY); | |||||||||||||||||
| if (fd < 0) { | if (fd < 0) { | |||||||||||||||||
Done Inline ActionsYou can put all 3 assignments on one line. It might help with what feels a bit repetitive about this clause. imp: You can put all 3 assignments on one line. It might help with what feels a bit repetitive about… | ||||||||||||||||||
Done Inline Actionsromfile_dup is unused. corvink: `romfile_dup` is unused. | ||||||||||||||||||
| EPRINTLN("Error opening bootrom \"%s\": %s", | EPRINTLN("Error opening bootrom \"%s\": %s", | |||||||||||||||||
| romfile, strerror(errno)); | romfile, strerror(errno)); | |||||||||||||||||
Done Inline Actions
I'd prefer to move parsing to pci_lpc where romfile is parsed. However, if you prefer to parse it at this place: Another possible solution: varfile = strdup(romfile); romfile = strsep(&varfile, ","); corvink: I'd prefer to move parsing to `pci_lpc` where `romfile` is parsed.
However, if you prefer to… | ||||||||||||||||||
Done Inline ActionsThanks, fixed. bcran: Thanks, fixed.
I'd prefer not to move the parsing at this time, since I didn't write this code… | ||||||||||||||||||
Not Done Inline ActionsFrom my side, it's ok if parsing isn't moved. Btw: You should be able to remove the if statement because varfile should be set to NULL by strsep if "," isn't found. corvink: From my side, it's ok if parsing isn't moved.
Btw: You should be able to remove the if… | ||||||||||||||||||
| goto done; | goto done; | |||||||||||||||||
| } | } | |||||||||||||||||
| if (varfile != NULL) { | ||||||||||||||||||
| varfd = open(varfile, O_RDWR); | ||||||||||||||||||
| if (varfd < 0) { | ||||||||||||||||||
Done Inline ActionsOpened varfd, but check status of fd? rgrimes: Opened varfd, but check status of fd?
| ||||||||||||||||||
Done Inline Actionsdefinitely wrong, thanks for catching that scottph: definitely wrong, thanks for catching that | ||||||||||||||||||
| fprintf(stderr, "Error opening bootrom variable file " | ||||||||||||||||||
| "\"%s\": %s\n", varfile, strerror(errno)); | ||||||||||||||||||
| goto done; | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| if (fstat(fd, &sbuf) < 0) { | if (fstat(fd, &sbuf) < 0) { | |||||||||||||||||
| EPRINTLN("Could not fstat bootrom file \"%s\": %s", | EPRINTLN("Could not fstat bootrom file \"%s\": %s", | |||||||||||||||||
| romfile, strerror(errno)); | romfile, strerror(errno)); | |||||||||||||||||
| goto done; | goto done; | |||||||||||||||||
| } | } | |||||||||||||||||
| rom_size = sbuf.st_size; | ||||||||||||||||||
| if (varfd < 0) { | ||||||||||||||||||
| var_size = 0; | ||||||||||||||||||
| } else { | ||||||||||||||||||
Done Inline ActionsSince the 'else' clause has {} around it, I'd be tempted to put {} here for consistency, but it's up to you (it's allowed, but not required by style(9)). imp: Since the 'else' clause has {} around it, I'd be tempted to put {} here for consistency, but… | ||||||||||||||||||
| if (fstat(varfd, &sbuf) < 0) { | ||||||||||||||||||
Done Inline Actionsi wonder how this can happen since we just opened the file.... imp: i wonder how this can happen since we just opened the file.... | ||||||||||||||||||
Done Inline ActionsSomeone just deleted the file? bcran: Someone just deleted the file? | ||||||||||||||||||
| fprintf(stderr, "Could not fstat bootrom variable file \"%s\": %s\n", | ||||||||||||||||||
| varfile, strerror(errno)); | ||||||||||||||||||
| goto done; | ||||||||||||||||||
| } | ||||||||||||||||||
| var_size = sbuf.st_size; | ||||||||||||||||||
| } | ||||||||||||||||||
| if (var_size > BOOTROM_SIZE || | ||||||||||||||||||
| (var_size != 0 && var_size < PAGE_SIZE)) { | ||||||||||||||||||
| fprintf(stderr, "Invalid bootrom variable size %ld\n", | ||||||||||||||||||
| var_size); | ||||||||||||||||||
| goto done; | ||||||||||||||||||
| } | ||||||||||||||||||
| total_size = rom_size + var_size; | ||||||||||||||||||
| if (total_size > BOOTROM_SIZE) { | ||||||||||||||||||
| fprintf(stderr, "Invalid bootrom and variable aggregate size " | ||||||||||||||||||
| "%ld\n", total_size); | ||||||||||||||||||
| goto done; | ||||||||||||||||||
| } | ||||||||||||||||||
| /* Map the bootrom into the guest address space */ | /* Map the bootrom into the guest address space */ | |||||||||||||||||
| if (bootrom_alloc(ctx, sbuf.st_size, PROT_READ | PROT_EXEC, | if (bootrom_alloc(ctx, rom_size, PROT_READ | PROT_EXEC, | |||||||||||||||||
Done Inline Actions"Bootrom or variable file is not a multiple of the page size." is a more correct error. I think I would split this out and leave the old code untouched as possible and add a new error for var_size & PAGE_MASK rgrimes: "Bootrom or variable file is not a multiple of the page size." is a more correct error. I… | ||||||||||||||||||
Done Inline ActionsI've split up the checks here so that there are individual checks and error messages for each case. scottph: I've split up the checks here so that there are individual checks and error messages for each… | ||||||||||||||||||
| BOOTROM_ALLOC_TOP, &ptr, NULL) != 0) | BOOTROM_ALLOC_TOP, &ptr, NULL) != 0) { | |||||||||||||||||
Done Inline ActionsIt may be more useful to print 2 or more different errors here, this single error covers a lot of conditions that can cause it. rgrimes: It may be more useful to print 2 or more different errors here, this single error covers a lot… | ||||||||||||||||||
| goto done; | goto done; | |||||||||||||||||
| } | ||||||||||||||||||
| /* Read 'romfile' into the guest address space */ | /* Read 'romfile' into the guest address space */ | |||||||||||||||||
| for (i = 0; i < sbuf.st_size / PAGE_SIZE; i++) { | for (i = 0; i < rom_size / PAGE_SIZE; i++) { | |||||||||||||||||
| rlen = read(fd, ptr + i * PAGE_SIZE, PAGE_SIZE); | rlen = read(fd, ptr + i * PAGE_SIZE, PAGE_SIZE); | |||||||||||||||||
| if (rlen != PAGE_SIZE) { | if (rlen != PAGE_SIZE) { | |||||||||||||||||
| EPRINTLN("Incomplete read of page %d of bootrom " | EPRINTLN("Incomplete read of page %d of bootrom " | |||||||||||||||||
| "file %s: %ld bytes", i, romfile, rlen); | "file %s: %ld bytes", i, romfile, rlen); | |||||||||||||||||
| goto done; | goto done; | |||||||||||||||||
| } | } | |||||||||||||||||
| } | } | |||||||||||||||||
| if (varfd >= 0) { | ||||||||||||||||||
| var.mmap = mmap(NULL, var_size, PROT_READ | PROT_WRITE, | ||||||||||||||||||
| MAP_SHARED, varfd, 0); | ||||||||||||||||||
| if (var.mmap == MAP_FAILED) | ||||||||||||||||||
| goto done; | ||||||||||||||||||
| var.size = var_size; | ||||||||||||||||||
| var.gpa = (gpa_alloctop - var_size) + 1; | ||||||||||||||||||
Done Inline Actionsgpa_alloctop should be decreased. corvink: `gpa_alloctop` should be decreased. | ||||||||||||||||||
Done Inline Actionswhat's the thinking there? imp: what's the thinking there? | ||||||||||||||||||
Done Inline Actionsbootrom_alloc is used to allocate space in the bootrom segment. It uses gpa_alloctop and gpa_allocbottom for allocation. Since varfile is allocated in the bootrom segment, it should keep those values consistent. Maybe it would be even better to reuse bootrom_alloc. corvink: `bootrom_alloc` is used to allocate space in the bootrom segment. It uses `gpa_alloctop` and… | ||||||||||||||||||
| gpa_alloctop = var.gpa - 1; | ||||||||||||||||||
Not Done Inline ActionsWould be a good idea to check if there's enough space left. corvink: Would be a good idea to check if there's enough space left. | ||||||||||||||||||
| rv = register_mem(&(struct mem_range){ | ||||||||||||||||||
| .name = "bootrom variable", | ||||||||||||||||||
| .flags = MEM_F_RW, | ||||||||||||||||||
| .handler = bootrom_var_mem_handler, | ||||||||||||||||||
| .base = var.gpa, | ||||||||||||||||||
| .size = var.size, | ||||||||||||||||||
| }); | ||||||||||||||||||
| if (rv != 0) | ||||||||||||||||||
| goto done; | ||||||||||||||||||
| } | ||||||||||||||||||
| rv = 0; | rv = 0; | |||||||||||||||||
| done: | done: | |||||||||||||||||
| if (fd >= 0) | if (fd >= 0) | |||||||||||||||||
| close(fd); | close(fd); | |||||||||||||||||
| return (rv); | return (rv); | |||||||||||||||||
| } | } | |||||||||||||||||
The next two are initialized in the code, which is the prefered form rather than doing it in the declares, for consistancy could you move this initialization to be with the others below.