Changeset View
Standalone View
usr.sbin/bhyve/bootrom.c
Show All 32 Lines | ||||||||||||||||||
#include <sys/mman.h> | #include <sys/mman.h> | |||||||||||||||||
#include <sys/stat.h> | #include <sys/stat.h> | |||||||||||||||||
#include <machine/vmm.h> | #include <machine/vmm.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 "mem.h" | ||||||||||||||||||
#define MAX_BOOTROM_SIZE (16 * 1024 * 1024) /* 16 MB */ | #define MAX_BOOTROM_SIZE (16 * 1024 * 1024) /* 16 MB */ | |||||||||||||||||
#define CFI_BCS_WRITE_BYTE 0x10 | ||||||||||||||||||
#define CFI_BCS_CLEAR_STATUS 0x50 | ||||||||||||||||||
#define CFI_BCS_READ_STATUS 0x70 | ||||||||||||||||||
#define CFI_BCS_READ_ARRAY 0xff | ||||||||||||||||||
static struct bootrom_var_state { | ||||||||||||||||||
uint8_t *mmap; | ||||||||||||||||||
uint64_t gpa; | ||||||||||||||||||
off_t size; | ||||||||||||||||||
uint8_t cmd; | ||||||||||||||||||
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… | ||||||||||||||||||
} 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 | ||||||||||||||||||
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. | ||||||||||||||||||
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; | ||||||||||||||||||
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 { | ||||||||||||||||||
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… | ||||||||||||||||||
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); | ||||||||||||||||||
} | ||||||||||||||||||
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_init(struct vmctx *ctx, const char *romfile) | bootrom_init(struct vmctx *ctx, const char *romfile) | |||||||||||||||||
{ | { | |||||||||||||||||
struct stat sbuf; | struct stat sbuf; | |||||||||||||||||
vm_paddr_t gpa; | vm_paddr_t gpa; | |||||||||||||||||
off_t rom_size, var_size, total_size; | ||||||||||||||||||
ssize_t rlen; | ssize_t rlen; | |||||||||||||||||
char *ptr; | char *ptr, *romfile_dup, *varfile; | |||||||||||||||||
int fd, i, rv, prot; | int fd, varfd, i, rv, prot; | |||||||||||||||||
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… | ||||||||||||||||||
rgrimesUnsubmitted 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; | ||||||||||||||||||
if (strchr(romfile, ',') == NULL) { | ||||||||||||||||||
romfile_dup = NULL; | ||||||||||||||||||
varfile = NULL; | ||||||||||||||||||
} else { | ||||||||||||||||||
romfile_dup = strdup(romfile); | ||||||||||||||||||
romfile = romfile_dup; | ||||||||||||||||||
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. | ||||||||||||||||||
varfile = romfile_dup; | ||||||||||||||||||
strsep(&varfile, ","); | ||||||||||||||||||
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… | ||||||||||||||||||
} | ||||||||||||||||||
fd = open(romfile, O_RDONLY); | fd = open(romfile, O_RDONLY); | |||||||||||||||||
if (fd < 0) { | if (fd < 0) { | |||||||||||||||||
fprintf(stderr, "Error opening bootrom \"%s\": %s\n", | fprintf(stderr, "Error opening bootrom \"%s\": %s\n", | |||||||||||||||||
romfile, strerror(errno)); | romfile, strerror(errno)); | |||||||||||||||||
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) { | |||||||||||||||||
fprintf(stderr, "Could not fstat bootrom file \"%s\": %s\n", | fprintf(stderr, "Could not fstat bootrom file \"%s\": %s\n", | |||||||||||||||||
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; | ||||||||||||||||||
} | ||||||||||||||||||
/* | /* | |||||||||||||||||
* Limit bootrom size to 16MB so it doesn't encroach into reserved | * Limit bootrom size to 16MB so it doesn't encroach into reserved | |||||||||||||||||
* MMIO space (e.g. APIC, HPET, MSI). | * MMIO space (e.g. APIC, HPET, MSI). | |||||||||||||||||
*/ | */ | |||||||||||||||||
if (sbuf.st_size > MAX_BOOTROM_SIZE || sbuf.st_size < PAGE_SIZE) { | if (rom_size > MAX_BOOTROM_SIZE || rom_size < PAGE_SIZE) { | |||||||||||||||||
fprintf(stderr, "Invalid bootrom size %ld\n", sbuf.st_size); | fprintf(stderr, "Invalid bootrom size %ld\n", rom_size); | |||||||||||||||||
goto done; | goto done; | |||||||||||||||||
} | } | |||||||||||||||||
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… | ||||||||||||||||||
if (sbuf.st_size & PAGE_MASK) { | if (var_size > MAX_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 > MAX_BOOTROM_SIZE) { | ||||||||||||||||||
fprintf(stderr, "Invalid bootrom and variable aggregate size " | ||||||||||||||||||
"%ld\n", total_size); | ||||||||||||||||||
goto done; | ||||||||||||||||||
} | ||||||||||||||||||
if (rom_size & PAGE_MASK) { | ||||||||||||||||||
fprintf(stderr, "Bootrom size %ld is not a multiple of the " | fprintf(stderr, "Bootrom size %ld is not a multiple of the " | |||||||||||||||||
"page size\n", sbuf.st_size); | "page size\n", rom_size); | |||||||||||||||||
goto done; | goto done; | |||||||||||||||||
} | } | |||||||||||||||||
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… | ||||||||||||||||||
ptr = vm_create_devmem(ctx, VM_BOOTROM, "bootrom", sbuf.st_size); | if (var_size & PAGE_MASK) { | |||||||||||||||||
fprintf(stderr, "Bootrom variable size %ld is not a multiple " | ||||||||||||||||||
"of the page size\n", var_size); | ||||||||||||||||||
goto done; | ||||||||||||||||||
} | ||||||||||||||||||
ptr = vm_create_devmem(ctx, VM_BOOTROM, "bootrom", rom_size); | ||||||||||||||||||
if (ptr == MAP_FAILED) | if (ptr == MAP_FAILED) | |||||||||||||||||
goto done; | goto done; | |||||||||||||||||
/* Map the bootrom into the guest address space */ | /* Map the bootrom into the guest address space */ | |||||||||||||||||
prot = PROT_READ | PROT_EXEC; | prot = PROT_READ | PROT_EXEC; | |||||||||||||||||
gpa = (1ULL << 32) - sbuf.st_size; | gpa = (1ULL << 32) - rom_size; | |||||||||||||||||
if (vm_mmap_memseg(ctx, gpa, VM_BOOTROM, 0, sbuf.st_size, prot) != 0) | if (vm_mmap_memseg(ctx, gpa, VM_BOOTROM, 0, rom_size, prot) != 0) | |||||||||||||||||
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) { | |||||||||||||||||
fprintf(stderr, "Incomplete read of page %d of bootrom " | fprintf(stderr, "Incomplete read of page %d of bootrom " | |||||||||||||||||
"file %s: %ld bytes\n", i, romfile, rlen); | "file %s: %ld bytes\n", 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 - var_size; | ||||||||||||||||||
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… | ||||||||||||||||||
rv = register_mem(&(struct mem_range){ | ||||||||||||||||||
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. | ||||||||||||||||||
.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: | |||||||||||||||||
free(romfile_dup); | ||||||||||||||||||
if (fd >= 0) | if (fd >= 0) | |||||||||||||||||
close(fd); | close(fd); | |||||||||||||||||
if (varfd >= 0) | ||||||||||||||||||
close(varfd); | ||||||||||||||||||
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.