Message ID | 20220721122937.729959-1-pbonzini@redhat.com |
---|---|
Headers | show |
Series | Refactor x86_load_linux and pass RNG seed via setup_data entry | expand |
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.