[PATCH 1/19] bhyve: add basic basl implementation

[corvink]

Summary

Basl is the bhyve ASL compiler. At the moment, it's just a small wrapper
to call iasl, the Intel ASL compiler. As bhyve will gain support for qemu's
ACPI table loader in the future, it has to create ACPI tables on it's own. Therefore,
it makes sense to create a new file which keeps the code for basl.

This first implementation of basl supports creating an ACPI table by
appending raw bytes to it. It's also capable of loading all tables into guest memory.

This is the first patch required to merge D36983

[markj]

I don't have a strong sense for whether this series of patches is going in the right direction, but I'll go through them and try to point out any mistakes that I can see.

usr.sbin/bhyve/basl.c

10

This doesn't need to be added to new files.

37

This can be static?

40

Where does this get used? I can't find any other references to it in the final diff.

46

58

116

IMHO it's a bit strange to pass a nul-terminated string using a byte array like this. Why not just const char *.

123

The cast isn't needed.

133

You might check for truncation here.

usr.sbin/bhyve/basl.h

11

It looks like this header isn't required by basl.h itself. Can consumers (basl.c and acpi.c) just include it directly instead?

33

Use void for prototypes with empty parameter lists, i.e., this should be int basl_finish(void);.

[markj]

usr.sbin/bhyve/basl.c

43

"finish_alloc" is not very descriptive, maybe "install_guest_tables" or something like that?

[corvink]

usr.sbin/bhyve/basl.c

40

Good catch. Forgot to remove it. Qemu uses it's fwcfg interface to install ACPI tables. My first implementation of this rework does it too.

116

I like to use the qemu fwcfg interface in the future to pass ACPI tables to the guest. This interface uses a fixed size for names.

usr.sbin/bhyve/basl.h

11

E.g. it's used for ACPI_NAMESEG_SIZE by basl_table_append_pointer (D36991) and basl_table_append_header (D36992)

[corvink]

• address feedback from markj

[corvink]

@markj Any comments on my new version?

[corvink]

• use void for prototypes with empty parameter lists

[corvink]

• move table loop from basl_finish_isntall_guest_tables to basl_finish

[jhb]

usr.sbin/bhyve/basl.c

9

The #include <sys/cdefs.h> can also go away (assuming you removed __FBSDID that was here previously)

43

I think it's more that guests using bhyveload search for ACPI tables in guest memory and use them directly. The existing bhyve UEFI ROM has its own static ACPI table templates it fills out instead of using tables generated by bhyve.

68

style(9) still wants variables like this declared at the start of the function and not in the middle of the body.

102

Not sure the const here is adding much vs just using void *end. Also, style(9) would want the end variable declared at the top of the function (which means you can't use const I think).

128

This will raise a warning when built with -DNDEBUG. OTOH, I'm not sure this assert() is really needed as it's checking that snprintf() works as defined in the relevant standard.

usr.sbin/bhyve/basl.h

3

I think this specific license clause is now discouraged vs just using "BSD-2-Clause". It's been deprecated upstream at SPDX.org.

[corvink]

usr.sbin/bhyve/basl.c

43

I like to support the qemu OVMF in the future. It searches for ACPI tables. Additionally, I have a patch for bhyve OVMF to search for ACPI tables in guest memory https://github.com/Beckhoff/edk2/commit/5767f7bac7d163f62c6f5c897a895500684308a4.
I'm going to update the comment to be more accurately.

128

The intention of this assertion was to check for truncation.

[corvink]

• address reviewer feedback

[corvink]

• fix license header of basl.h

[corvink]

• fix uninitialized variable gva

[jhb]

usr.sbin/bhyve/basl.c

43

The new comment is still a bit confusing to me. Guest OS's do not install or update ACPI tables, they only read tables provided by the firmware. In this case bhyve's startup is either acting as the firmware (in the case of bhyveload) or providing tables to the firmware (qemu OVMF) that the firmware then stores in guest memory. Perhaps something like this might work:

/*
  * Install ACPI tables directly in guest memory for use by guests which do
  * not boot via EFI.  EFI ROMs provide a pointer to the firmware-generated
  * ACPI tables instead, but it doesn't hurt to install the tables always.
  */

[corvink]

• update comment why bhyve copies ACPI tables into guest memory

[corvink]

usr.sbin/bhyve/basl.c

43

I'm going to update all comments in my other commits when merging them.

[jhb]

usr.sbin/bhyve/basl.c

43

Thanks!