Re: [PATCH v4] hw/i386: pass RNG seed via setup_data entry

["Jason A. Donenfeld" <Jason@zx2c4.com>]

Hi Michael,

Thanks for the feedback.

On Thu, Jul 21, 2022 at 06:35:41AM -0400, Michael S. Tsirkin wrote:
> > -        setup_data->next = 0;
> > +        setup_data->next = last_setup_data_offset;
> 
> does this make any difference? if the idea is that we'll add more stuff
> down the road, then see below ...

It doesn't; it's just for completeness, in case somebody decides to add
something prior, and then less code has to change and there's less
chance of an error. The compiler generates the same code either way.

> 
> >          setup_data->type = cpu_to_le32(SETUP_DTB);
> >          setup_data->len = cpu_to_le32(dtb_size);
> >  
> >          load_image_size(dtb_filename, setup_data->data, dtb_size);
> > +
> > +        last_setup_data_offset = prot_addr + setup_data_offset;
> 
> 
> if the idea is that we'll add more stuff down the road, then
> it should be += here.

It's just poorly named actually. It should be called
"prev_setup_data_prot_addr" or something. I'll find a better name for
v+1.

> 
> >      }
> >  
> > +    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);
> > +    setup_data = (struct setup_data *)(kernel + setup_data_offset);
> > +    setup_data->next = last_setup_data_offset;
> 
> Likely broken on LE.

Nice catch, thanks.

> 
> > +    setup_data->type = cpu_to_le32(SETUP_RNG_SEED);
> > +    setup_data->len = cpu_to_le32(32);
> > +    qemu_guest_getrandom_nofail(setup_data->data, 32);
> > +
> > +    last_setup_data_offset = prot_addr + setup_data_offset;
> 
> 
> where does this 32 come from? maybe make it a macro.

Will do.

> 
> > +
> > +    stq_p(header + 0x250, last_setup_data_offset);
> 
> add a comment while we are at it?

Ack.
 
Jason