hw/i386/pc.c | 1 + hw/i386/x86.c | 303 +++++++++++-------- include/hw/i386/x86.h | 2 + include/standard-headers/asm-x86/bootparam.h | 1 + 4 files changed, 185 insertions(+), 122 deletions(-)
As mentioned in the reviews of Jason's patches, the fw_cfg data, or at least its structure including the size, is part of the guest ABI and must match across two sides of migration. It would be possible to handle this with some duplicated code between the rng seed and DTB handling, but the conditionals to handle the linked list would be ugly. Unfortunately the code of x86_load_linux has no data structures available, it's all of a jumble of local variables. Hence the first two and largest patches in this series, which remove all non-Linux code from the function and move the local variables to a struct as necessary. The function was long overdue for some cleanup anyway. With this in place, adding the seed setup_data entry is just a couple lines of code, plus the scaffolding for a new machine property "linuxboot-seed". The property supports on/off/auto values, where "auto" disables/enables depending on the kernel support for setup data (which was added in 2.6.26); "on" currently fails when starting with an old kernel, and probably it should also fail when starting a PVH or multiboot kernel. Paolo Jason A. Donenfeld (1): hw/i386: pass RNG seed via setup_data entry Paolo Bonzini (3): hw/i386: extract PVH load to a separate function hw/i386: define a struct for Linux boot protocol data hw/i386: extract handling of setup data linked list hw/i386/pc.c | 1 + hw/i386/x86.c | 303 +++++++++++-------- include/hw/i386/x86.h | 2 + include/standard-headers/asm-x86/bootparam.h | 1 + 4 files changed, 185 insertions(+), 122 deletions(-) -- 2.36.1
On Thu, Jul 21, 2022 at 02:29:33PM +0200, Paolo Bonzini wrote: > As mentioned in the reviews of Jason's patches, the fw_cfg data, or at > least its structure including the size, is part of the guest ABI and > must match across two sides of migration. > > It would be possible to handle this with some duplicated code between > the rng seed and DTB handling, but the conditionals to handle the linked > list would be ugly. Unfortunately the code of x86_load_linux has no > data structures available, it's all of a jumble of local variables. > Hence the first two and largest patches in this series, which remove all > non-Linux code from the function and move the local variables to a struct > as necessary. The function was long overdue for some cleanup anyway. > > With this in place, adding the seed setup_data entry is just a > couple lines of code, plus the scaffolding for a new machine property > "linuxboot-seed". The property supports on/off/auto values, where "auto" > disables/enables depending on the kernel support for setup data (which was > added in 2.6.26); "on" currently fails when starting with an old kernel, > and probably it should also fail when starting a PVH or multiboot kernel. > > Paolo I like the refactoring Reviewed-by: Michael S. Tsirkin <mst@redhat.com> To avoid creating extra work for Jason and confusing attribution, maybe apply Jason's patch then your refactoring on top? > Jason A. Donenfeld (1): > hw/i386: pass RNG seed via setup_data entry > > Paolo Bonzini (3): > hw/i386: extract PVH load to a separate function > hw/i386: define a struct for Linux boot protocol data > hw/i386: extract handling of setup data linked list > > hw/i386/pc.c | 1 + > hw/i386/x86.c | 303 +++++++++++-------- > include/hw/i386/x86.h | 2 + > include/standard-headers/asm-x86/bootparam.h | 1 + > 4 files changed, 185 insertions(+), 122 deletions(-) > > -- > 2.36.1
Hi Michael, On Thu, Jul 21, 2022 at 10:52:32AM -0400, Michael S. Tsirkin wrote: > On Thu, Jul 21, 2022 at 02:29:33PM +0200, Paolo Bonzini wrote: > > As mentioned in the reviews of Jason's patches, the fw_cfg data, or at > > least its structure including the size, is part of the guest ABI and > > must match across two sides of migration. > > > > It would be possible to handle this with some duplicated code between > > the rng seed and DTB handling, but the conditionals to handle the linked > > list would be ugly. Unfortunately the code of x86_load_linux has no > > data structures available, it's all of a jumble of local variables. > > Hence the first two and largest patches in this series, which remove all > > non-Linux code from the function and move the local variables to a struct > > as necessary. The function was long overdue for some cleanup anyway. > > > > With this in place, adding the seed setup_data entry is just a > > couple lines of code, plus the scaffolding for a new machine property > > "linuxboot-seed". The property supports on/off/auto values, where "auto" > > disables/enables depending on the kernel support for setup data (which was > > added in 2.6.26); "on" currently fails when starting with an old kernel, > > and probably it should also fail when starting a PVH or multiboot kernel. > > > > Paolo > > I like the refactoring > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > To avoid creating extra work for Jason and confusing > attribution, maybe apply Jason's patch then your refactoring > on top? Yes, I think it'd make sense to apply: https://lore.kernel.org/qemu-devel/20220721125636.446842-1-Jason@zx2c4.com/ as-is, without any changes, since that handles your migration concerns. And then after, if you want to refactor things in general, apply that on top. As I mentioned before, we really don't need nor want a user-facing option for this. What I do in that v7 there is sufficient and fine. Michael - do you want to take that v7 into your tree? Jason
On Thu, Jul 21, 2022 at 05:11:31PM +0200, Jason A. Donenfeld wrote: > Hi Michael, > > On Thu, Jul 21, 2022 at 10:52:32AM -0400, Michael S. Tsirkin wrote: > > On Thu, Jul 21, 2022 at 02:29:33PM +0200, Paolo Bonzini wrote: > > > As mentioned in the reviews of Jason's patches, the fw_cfg data, or at > > > least its structure including the size, is part of the guest ABI and > > > must match across two sides of migration. > > > > > > It would be possible to handle this with some duplicated code between > > > the rng seed and DTB handling, but the conditionals to handle the linked > > > list would be ugly. Unfortunately the code of x86_load_linux has no > > > data structures available, it's all of a jumble of local variables. > > > Hence the first two and largest patches in this series, which remove all > > > non-Linux code from the function and move the local variables to a struct > > > as necessary. The function was long overdue for some cleanup anyway. > > > > > > With this in place, adding the seed setup_data entry is just a > > > couple lines of code, plus the scaffolding for a new machine property > > > "linuxboot-seed". The property supports on/off/auto values, where "auto" > > > disables/enables depending on the kernel support for setup data (which was > > > added in 2.6.26); "on" currently fails when starting with an old kernel, > > > and probably it should also fail when starting a PVH or multiboot kernel. > > > > > > Paolo > > > > I like the refactoring > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > > > To avoid creating extra work for Jason and confusing > > attribution, maybe apply Jason's patch then your refactoring > > on top? > > Yes, I think it'd make sense to apply: > https://lore.kernel.org/qemu-devel/20220721125636.446842-1-Jason@zx2c4.com/ > as-is, without any changes, since that handles your migration concerns. > > And then after, if you want to refactor things in general, apply that on > top. > > As I mentioned before, we really don't need nor want a user-facing > option for this. Yes I think we don't want to support such an option. We have a general rule of prefixing properties with "x-" for this these are considered unstable and we are often adding them for internal purposes. > What I do in that v7 there is sufficient and fine. > > Michael - do you want to take that v7 into your tree? > > Jason Can be my tree or Paolo's but I'll wait for him to respond, I like consensus. -- MST
© 2016 - 2024 Red Hat, Inc.