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

[Hongbo Zhang]

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.

>> Thanks,
>> drew
>>
>