| Message ID | 20220630113717.1893529-1-Jason@zx2c4.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/i386: pass RNG seed to e820 setup table | expand |
On Thu, Jun 30, 2022 at 01:37:17PM +0200, Jason A. Donenfeld wrote: > Tiny machines optimized for fast boot time generally don't use EFI, > which means a random seed has to be supplied some other way, in this > case by the e820 setup table, which supplies a place for one. This > commit adds passing this random seed via the table. It is confirmed to > be working with the Linux patch in the link. IIUC, this approach will only expose the random seed when QEMU is booted using -kernel + -initrd args. I agree with what you say about most VMs not using UEFI right now. I'd say the majority of general purpose VMs are using SeaBIOS still. The usage of -kernel + -initrd, is typically for more specialized use cases. IOW, exposing random seed via the setup table feels like it'll have a somewhat limited benefit. Can we get an approach that exposes a random seed regardless of whether using -kernel, or seabios, or uefi, or $whatever firmware ? Perhaps (ab)use 'fw_cfg', which is exposed for any x86 VM no matter what config it has for booting ? With regards, Daniel
Hi Daniel, On Fri, Jul 8, 2022 at 2:00 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Thu, Jun 30, 2022 at 01:37:17PM +0200, Jason A. Donenfeld wrote: > > Tiny machines optimized for fast boot time generally don't use EFI, > > which means a random seed has to be supplied some other way, in this > > case by the e820 setup table, which supplies a place for one. This > > commit adds passing this random seed via the table. It is confirmed to > > be working with the Linux patch in the link. > > IIUC, this approach will only expose the random seed when QEMU > is booted using -kernel + -initrd args. > > I agree with what you say about most VMs not using UEFI right now. > I'd say the majority of general purpose VMs are using SeaBIOS > still. The usage of -kernel + -initrd, is typically for more > specialized use cases. Highly disagree, based on seeing a lot of real world deployment. Furthermore, this is going to be used within Linux itself for kexec, so it makes sense to use it here too. > Can we get an approach that exposes a random seed regardless of > whether using -kernel, or seabios, or uefi, or $whatever firmware ? No. > Perhaps (ab)use 'fw_cfg', which is exposed for any x86 VM no matter > what config it has for booting ? That approach is super messy and doesn't work. I've already gone down that route. The entire point here is to include the seed on this part of the boot protocol. There might be other opportunities for doing it elsewhere. For example, EFI already has a thing. Please don't sink a good idea because it doesn't handle every possible use case. That type of mentality is just going to result in nothing ever getting done anywhere, making a decades old problem last for another decade. This patch here is simple and makes a tangible incremental advance toward something good, and fits the pattern of how it's done on all other platforms. Thanks, Jason
On Fri, Jul 08, 2022 at 02:04:40PM +0200, Jason A. Donenfeld wrote: > Hi Daniel, > > On Fri, Jul 8, 2022 at 2:00 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > On Thu, Jun 30, 2022 at 01:37:17PM +0200, Jason A. Donenfeld wrote: > > > Tiny machines optimized for fast boot time generally don't use EFI, > > > which means a random seed has to be supplied some other way, in this > > > case by the e820 setup table, which supplies a place for one. This > > > commit adds passing this random seed via the table. It is confirmed to > > > be working with the Linux patch in the link. > > > > IIUC, this approach will only expose the random seed when QEMU > > is booted using -kernel + -initrd args. > > > > I agree with what you say about most VMs not using UEFI right now. > > I'd say the majority of general purpose VMs are using SeaBIOS > > still. The usage of -kernel + -initrd, is typically for more > > specialized use cases. > > Highly disagree, based on seeing a lot of real world deployment. I guess we're looking at different places then, as all of the large scale virt mgmt apps I've experianced with KVM (OpenStack, oVirt, KubeVirt), along with the small scale ones (GNOME Boxes, virt-manager, virt-install, Cockpit), etc all primarily use SeaBIOS, and in more recently times a bit of UEFI. Direct kernel/initrd boot is usualy reserved for special cases, since users like to be able to manage their kernel/initrd inside the guest image. > Furthermore, this is going to be used within Linux itself for kexec, > so it makes sense to use it here too. Ok, useful info. > > Can we get an approach that exposes a random seed regardless of > > whether using -kernel, or seabios, or uefi, or $whatever firmware ? > > No. > > > Perhaps (ab)use 'fw_cfg', which is exposed for any x86 VM no matter > > what config it has for booting ? > > That approach is super messy and doesn't work. I've already gone down > that route. What's the problem with it ? fw_cfg is a pretty straightforward mechanism for injecting data into the guest OS, that we already use for alot of stuff. > The entire point here is to include the seed on this part of the boot > protocol. There might be other opportunities for doing it elsewhere. > For example, EFI already has a thing. > > Please don't sink a good idea because it doesn't handle every possible > use case. That type of mentality is just going to result in nothing > ever getting done anywhere, making a decades old problem last for > another decade. This patch here is simple and makes a tangible > incremental advance toward something good, and fits the pattern of how > it's done on all other platforms. I'm not trying to sink an idea. If this turns out to be the best idea, I've no problem with that. I merely asked some reasonable questions about whether there were alternative approaches that could solve more broadly useful scenarios, given the narrow usage of direct kernel boot, in context of the common VM deployments I've seen at large scale. You can't expect reviewers to blindly accept any proposal without considering it broader context. With regards, Daniel
diff --git a/hw/i386/x86.c b/hw/i386/x86.c index 6003b4b2df..0724759eec 100644 --- a/hw/i386/x86.c +++ b/hw/i386/x86.c @@ -26,6 +26,7 @@ #include "qemu/cutils.h" #include "qemu/units.h" #include "qemu/datadir.h" +#include "qemu/guest-random.h" #include "qapi/error.h" #include "qapi/qmp/qerror.h" #include "qapi/qapi-visit-common.h" @@ -1045,6 +1046,16 @@ void x86_load_linux(X86MachineState *x86ms, } fclose(f); + setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16); + kernel_size = setup_data_offset + sizeof(struct setup_data) + 32; + kernel = g_realloc(kernel, kernel_size); + stq_p(header + 0x250, prot_addr + setup_data_offset); + setup_data = (struct setup_data *)(kernel + setup_data_offset); + setup_data->next = 0; + setup_data->type = cpu_to_le32(SETUP_RNG_SEED); + setup_data->len = cpu_to_le32(32); + qemu_guest_getrandom_nofail(setup_data->data, 32); + /* append dtb to kernel */ if (dtb_filename) { if (protocol < 0x209) { @@ -1059,13 +1070,11 @@ void x86_load_linux(X86MachineState *x86ms, exit(1); } - setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16); - kernel_size = setup_data_offset + sizeof(struct setup_data) + dtb_size; + kernel_size += sizeof(struct setup_data) + dtb_size; kernel = g_realloc(kernel, kernel_size); - stq_p(header + 0x250, prot_addr + setup_data_offset); - - setup_data = (struct setup_data *)(kernel + setup_data_offset); + setup_data->next = prot_addr + setup_data_offset + sizeof(*setup_data) + setup_data->len; + ++setup_data; setup_data->next = 0; setup_data->type = cpu_to_le32(SETUP_DTB); setup_data->len = cpu_to_le32(dtb_size); diff --git a/include/standard-headers/asm-x86/bootparam.h b/include/standard-headers/asm-x86/bootparam.h index 072e2ed546..b8cb1fa313 100644 --- a/include/standard-headers/asm-x86/bootparam.h +++ b/include/standard-headers/asm-x86/bootparam.h @@ -10,6 +10,7 @@ #define SETUP_EFI 4 #define SETUP_APPLE_PROPERTIES 5 #define SETUP_JAILHOUSE 6 +#define SETUP_RNG_SEED 8 #define SETUP_INDIRECT (1<<31)
Tiny machines optimized for fast boot time generally don't use EFI, which means a random seed has to be supplied some other way, in this case by the e820 setup table, which supplies a place for one. This commit adds passing this random seed via the table. It is confirmed to be working with the Linux patch in the link. Link: https://lore.kernel.org/lkml/20220630113300.1892799-1-Jason@zx2c4.com/ Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- hw/i386/x86.c | 19 ++++++++++++++----- include/standard-headers/asm-x86/bootparam.h | 1 + 2 files changed, 15 insertions(+), 5 deletions(-)