Re: [Qemu-devel] [PATCH v2] Add optionrom compatible with fw_cfg DMA version

[Gerd Hoffmann <kraxel@redhat.com>]

  Hi,

> > > +    if (fw_cfg_dma_enabled(fw_cfg)) {
> > > +        option_rom[nb_option_roms].name = "linuxboot_dma.bin";
> > > +        option_rom[nb_option_roms].bootindex = 0;
> > > +    } else {
> > > +        option_rom[nb_option_roms].name = "linuxboot.bin";
> > > +        option_rom[nb_option_roms].bootindex = 0;
> > > +    }  
> > 
> > Live migration compatibility requires that guest-visible changes to
> > the machine are only introduced in a new -machine <machine-type>.

> > I've CCed Gerd and Juan, I think they know how changes to Option ROMs
> > affect live migration better than me.  What needs to be done to
> > preserve live migration compatibility?
> 
> They are CC'd now :)

I think we are fine here.  The dma interface is enabled for new machine
types only, thats why we have fw_cfg_dma_enabled() in the first place ;)

> > Was there a technical reason why linuxboot.S cannot be extended
> > (e.g.  a size limit)?
> 
> I don't think there's a technical reason. It is a lot simpler to write
> the fw_cfg DMA stuff in C. To extend linuxboot.S these things should be
> modified:
>  - Add fw_cfg DMA detection support
>  - Change read_fw from a macro to a function that checks for fw_cfg DMA
>    support and does the operation using IO or memory
>  - Extract bits and pieces from linuxboot.S into functions, that are
>    only necessary when there is no support for fw_cfg DMA (the most
>    important is jumping to 32 bits to read and copy the kernel).
> 
> This way, you check for support from the very beggining (when
> configuring the machine), and you don't have to branch the code
> anymore.
> 
> (I think I discussed this with somebody in the past. But I'm not sure
> with whom, or when. So I'll suppose it was a dream and it is not on the
> archives).

Could have been /me.

The fw_cfg macros in linuxboot.S are messy, looks like because they got
extended a few times.  Piling DMA support on top of that didn't look
very appealing to me.

Also DMA support simplifies things, there is no need to switch processor
modes to load the kernel above 1M.

> If you really think they should be merged, I'd even propose to
> merge the ASM version onto the C version (convert this patch into
> linuxboot.S). This slightly improves readability.

Fully agree.  I'm personally fine with having two roms, but when merging
them into one we surely should ditch the fw_cfg asm macros and go with
something more maintainable.

cheers,
  Gerd