Re: [PATCH v6 00/10] Introduce the microvm machine type

[Philippe Mathieu-Daudé]

On 10/7/19 3:44 PM, Sergio Lopez wrote:
> Michael S. Tsirkin <address@hidden> writes:
>
> > On Fri, Oct 04, 2019 at 11:37:42AM +0200, Sergio Lopez wrote:
> > > Microvm is a machine type inspired by Firecracker and constructed
> > > after the its machine model.
> > >
> > > It's a minimalist machine type without PCI nor ACPI support, designed
> > > for short-lived guests. Microvm also establishes a baseline for
> > > benchmarking and optimizing both QEMU and guest operating systems,
> > > since it is optimized for both boot time and footprint.
> >
> > Pls take a look at patchew warnings and errors.
> > Both coding style issues and test failures need to be
> > addressed somehow I think.
>
> I've fixed the issue with the test suite, but I'm not sure what to do
> about the coding style errors. Every one of them (except perhaps one at
> xen-hvm.c) comes from code I've moved from pc.c to x86.c. I'd say fixing
> those are outside the scope of the corresponding patches, but please
> correct me if I'm wrong.

What makes reviews easier for me is to split in 2 patches: 1 fixing the 
lines I'm going to move, then the patch that simply move.

You can add checkpatch as a pre-commit hook:
http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html

In a temporary branch, git mv/commit will call checkpatch and display 
warnings; discard your commit and fix the warnings in the original code.

> On the other hand, I haven't touched MAINTAINERS, because I'm not sure
> about the actual policies that apply while doing so. Should I add the
> new files to it?

The new "hw/virtio/virtio-mmio.h" is covered by the "virtio" section:

F: hw/*/virtio*
F: include/hw/virtio/

Now I think the MicroVM files deserve an entry, probably with Paolo/you 
listed:

- docs/microvm.rst
- hw/i386/microvm.c
- include/hw/i386/microvm.h
- pc-bios/bios-microvm.bin

> > > ---
> > >
> > > Changelog
> > > v6:
> > >   - Some style fixes (Philippe Mathieu-Daudé)
> > >   - Fix a documentation bug stating that LAPIC was in userspace (Paolo
> > >     Bonzini)
> > >   - Update Xen HVM code after X86MachineState introduction (Philippe
> > >     Mathieu-Daudé)
> > >   - Rename header guard from QEMU_VIRTIO_MMIO_H to HW_VIRTIO_MMIO_H
> > >     (Philippe Mathieu-Daudé)
> > >
> > > v5:
> > >   - Drop unneeded "[PATCH v4 2/8] hw/i386: Factorize e820 related
> > >     functions" (Philippe Mathieu-Daudé)
> > >   - Drop unneeded "[PATCH v4 1/8] hw/i386: Factorize PVH related
> > >     functions" (Stefano Garzarella)
> > >   - Split X86MachineState introduction into smaller patches (Philippe
> > >     Mathieu-Daudé)
> > >   - Change option-roms to x-option-roms and kernel-cmdline to
> > >     auto-kernel-cmdline (Paolo Bonzini)
> > >   - Make i8259 PIT and i8254 PIC optional (Paolo Bonzini)
> > >   - Some fixes to the documentation (Paolo Bonzini)
> > >   - Switch documentation format from txt to rst (Peter Maydell)
> > >   - Move NMI interface to X86_MACHINE (Philippe Mathieu-Daudé, Paolo
> > >     Bonzini)
> > >
> > > v4:
> > >   - This is a complete rewrite of the whole patchset, with a focus on
> > >     reusing as much existing code as possible to ease the maintenance burden
> > >     and making the machine type as compatible as possible by default. As
> > >     a result, the number of lines dedicated specifically to microvm is
> > >     383 (code lines measured by "cloc") and, with the default
> > >     configuration, it's now able to boot both PVH ELF images and
> > >     bzImages with either SeaBIOS or qboot.
> > >
> > > v3:
> > >    - Add initrd support (thanks Stefano).
> > >
> > > v2:
> > >    - Drop "[PATCH 1/4] hw/i386: Factorize CPU routine".
> > >    - Simplify machine definition (thanks Eduardo).
> > >    - Remove use of unneeded NUMA-related callbacks (thanks Eduardo).
> > >    - Add a patch to factorize PVH-related functions.
> > >    - Replace use of Linux's Zero Page with PVH (thanks Maran and Paolo).
> > >
> > > ---
> > > Sergio Lopez (10):
> > >    hw/virtio: Factorize virtio-mmio headers
> > >    hw/i386/pc: rename functions shared with non-PC machines
> > >    hw/i386/pc: move shared x86 functions to x86.c and export them
> > >    hw/i386: split PCMachineState deriving X86MachineState from it
> > >    hw/i386: make x86.c independent from PCMachineState
> > >    fw_cfg: add "modify" functions for all types
> > >    hw/intc/apic: reject pic ints if isa_pic == NULL
> > >    roms: add microvm-bios (qboot) as binary and git submodule
> > >    docs/microvm.rst: document the new microvm machine type
> > >    hw/i386: Introduce the microvm machine type
> > >
> > >   docs/microvm.rst                 |  98 ++++
> > >   default-configs/i386-softmmu.mak |   1 +
> > >   include/hw/i386/microvm.h        |  83 ++++
> > >   include/hw/i386/pc.h             |  28 +-
> > >   include/hw/i386/x86.h            |  94 ++++
> > >   include/hw/nvram/fw_cfg.h        |  42 ++
> > >   include/hw/virtio/virtio-mmio.h  |  73 +++
> > >   hw/acpi/cpu_hotplug.c            |  10 +-
> > >   hw/i386/acpi-build.c             |  29 +-
> > >   hw/i386/amd_iommu.c              |   3 +-
> > >   hw/i386/intel_iommu.c            |   3 +-
> > >   hw/i386/microvm.c                | 574 ++++++++++++++++++++++
> > >   hw/i386/pc.c                     | 780 +++---------------------------
> > >   hw/i386/pc_piix.c                |  46 +-
> > >   hw/i386/pc_q35.c                 |  38 +-
> > >   hw/i386/pc_sysfw.c               |  58 +--
> > >   hw/i386/x86.c                    | 790 +++++++++++++++++++++++++++++++
> > >   hw/i386/xen/xen-hvm.c            |  23 +-
> > >   hw/intc/apic.c                   |   2 +-
> > >   hw/intc/ioapic.c                 |   2 +-
> > >   hw/nvram/fw_cfg.c                |  29 ++
> > >   hw/virtio/virtio-mmio.c          |  48 +-
> > >   .gitmodules                      |   3 +
> > >   hw/i386/Kconfig                  |   4 +
> > >   hw/i386/Makefile.objs            |   2 +
> > >   pc-bios/bios-microvm.bin         | Bin 0 -> 65536 bytes
> > >   roms/Makefile                    |   6 +
> > >   roms/qboot                       |   1 +
> > >   28 files changed, 1963 insertions(+), 907 deletions(-)
> > >   create mode 100644 docs/microvm.rst
> > >   create mode 100644 include/hw/i386/microvm.h
> > >   create mode 100644 include/hw/i386/x86.h
> > >   create mode 100644 include/hw/virtio/virtio-mmio.h
> > >   create mode 100644 hw/i386/microvm.c
> > >   create mode 100644 hw/i386/x86.c
> > >   create mode 100755 pc-bios/bios-microvm.bin
> > >   create mode 160000 roms/qboot
> > >
> > > --
> > > 2.21.0