Re: [PATCH 0/4] Refactor x86_load_linux and pass RNG seed via setup_data entry

["Michael S. Tsirkin" <mst@redhat.com>]

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