Re: [Qemu-devel] [PATCH v2 2/2] hw/arm: Add Arm Enterprise machine type

[Andrew Jones]

On Thu, Jul 26, 2018 at 05:55:54PM +0800, Hongbo Zhang wrote:
> On 26 July 2018 at 00:15, Igor Mammedov <address@hidden> wrote:
> > On Wed, 25 Jul 2018 13:36:45 +0200
> > Andrew Jones <address@hidden> wrote:
> >
> >> On Wed, Jul 25, 2018 at 11:50:41AM +0100, Dr. David Alan Gilbert wrote:
> >> > * Andrew Jones (address@hidden) wrote:
> >> > > On Wed, Jul 25, 2018 at 01:30:52PM +0800, Hongbo Zhang wrote:
> >> > > > For the Aarch64, there is one machine 'virt', it is primarily meant 
> >> > > > to
> >> > > > run on KVM and execute virtualization workloads, but we need an
> >> > > > environment as faithful as possible to physical hardware, for 
> >> > > > supporting
> >> > > > firmware and OS development for pysical Aarch64 machines.
> >> > > >
> >> > > > This patch introduces new machine type 'Enterprise' with main 
> >> > > > features:
> >> > > >  - Based on 'virt' machine type.
> >> > > >  - Re-designed memory map.
> >> > > >  - EL2 and EL3 are enabled by default.
> >> > > >  - GIC version 3 by default.
> >> > > >  - AHCI controller attached to system bus, and then CDROM and hard 
> >> > > > disc
> >> > > >    can be added to it.
> >> > > >  - EHCI controller attached to system bus, with USB mouse and key 
> >> > > > board
> >> > > >    installed by default.
> >> > > >  - E1000E ethernet card on PCIE bus.
> >> > > >  - VGA display adaptor on PCIE bus.
> >> > > >  - Default CPU type cortex-a57, 4 cores, and 1G bytes memory.
> >> > > >  - No virtio functions enabled, since this is to emulate real 
> >> > > > hardware.
> >> > >
> >> > > In the last review it was pointed out that using virtio-pci should 
> >> > > still
> >> > > be "real" enough, so there's not much reason to avoid it. Well, unless
> >> > > there's some concern as to what drivers are available in the firmware 
> >> > > and
> >> > > guest kernel. But that concern usually only applies to legacy firmwares
> >> > > and kernels, and therefore shouldn't apply to AArch64.
> >> >
> >> > I think the difference from last time is Ard's comments earlier in this
> >> > thread:
> >> >
> >> >     The purpose of the SBSA machine is not to provide a minimal
> >> >     configuration. It is intended to exercise all the moving parts one
> >> >     might find in a server firmware/OS stack, including pieces that are
> >> >     not usually found on x86 machines, such as DRAM starting above 4 GB
> >> >     and SATA/USB controllers that are not PCIe based.
> >> >
> >> > that suggests that the intent of this board is to provide everything
> >> > which a firmware writer might want to test;  that's quite different
> >> > from forming the basis of a virtualised machine for real use.
> >> >
> >>
> >> I think I understand the purpose, and I also don't believe anything I've
> >> said is counter to it. Whether or not one drives a virtio-pci nic with a
> >> virtio-pci-net driver or drives an E1000e, also on the PCIe bus, makes
> >> little difference to the firmware, nor to the guest kernel - besides which
> >> driver gets used. And, nothing stops somebody from not plugging the
> >> virtio-pci nic (use -nodefaults) and then plugging the E1000e (-device)
> >> instead. Machine models don't need to hard code these assumptions. For
> >> this patch it'd probably be best if we just ensured there were no
> >> default devices at all, rather than replace one with another.
> >
> > with that thinking it might be better to put this machine in completely
> > different file so not mess with 'production' virt machine code and
> > call it intentionally ambiguous: "[linaro|generic|dev|...]_armv8"
> > so it would be just another sbsa compliant board with  a bunch of
> > default devices needed for testing purposes if users/authors think
> > that it serves their purpose better.
> >
> Yes, before sending, I had to solutions: a separate file and share with 
> virt.c.
> For a internal release, I have a separate file sbsa.c
> http://git.linaro.org/people/hongbo.zhang/qemu-enterprise.git/tree/hw/arm/sbsa.c?h=sbsa-v2.12
> 
> If separate file is used as above, there are much duplicate with
> virt.c, such as both have create_pcie() etc, I need to move all the
> common parts to a new file say virt-common.c, then the virt.c and
> sbsa.c can only handle their specific parts, in such a way, the
> previous still cannot be left untouched.
> 
> so I send out this solution for discuss, people mainly concern the
> necessity of it till now, not the code and file format yet.
> Thanks for you advice.
>

Let's try to figure out how the "sbsa" machine type memory map should look
first. If it isn't adding a bunch of new stuff then it may not matter that
it's in the same file. OTOH, separating virt machine class device and fdt
building functions from virt machine instance parameters would be a nice
cleanup anyway. Doing that cleanup first, and then simply adding a new
machine instance file, would probably be the cleanest approach.

Thanks,
drew