qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [PATCH v4] hw/arm: Add arm SBSA reference machine


From: Hongbo Zhang
Subject: Re: [Qemu-arm] [PATCH v4] hw/arm: Add arm SBSA reference machine
Date: Mon, 19 Nov 2018 18:54:52 +0800

On Mon, 19 Nov 2018 at 18:49, Hongbo Zhang <address@hidden> wrote:
>
> On Fri, 16 Nov 2018 at 19:29, Peter Maydell <address@hidden> wrote:
> >
> > On 16 November 2018 at 10:46, Hongbo Zhang <address@hidden> wrote:
> > > On Fri, 16 Nov 2018 at 00:05, Peter Maydell <address@hidden> wrote:
> > >> If after you've done that this patch is still more than
> > >> about 500 lines long, I would recommend that you split it
> > >> up into coherent pieces, to make it easier to review.
> >
> > > I think however I re-work this file, it is still more than 500 lines.
> > > If split it, then how? one for most basic skeleton, including memory
> > > map, class and instance init etc, and another adding peripheral
> > > devices, like pcie etc?
> >
> > Yes, that's a reasonable split.
> >
> OK, let me see how many lines left when I remove unnecessary DT and EL2/3 
> lines.
>
> > >> This is still generating a lot of device tree nodes.
> > >> I thought we had agreed that we were going to just have
> > >> a minimal device tree that says "here is the RAM" and nothing
> > >> else ?
> > >>
> > > If I remember correct, it was not only the RAM, but also the CPUs
> > > because it depends on the command parameters dynamically, and also the
> > > GIC as well since it depends on the CPU number.
> > > And should NUMA info if used, be added to DT?
> > > And by the way, I didn't find the RAM size info in the DT.
> >
> > You should include the absolute minimum you need that you
> > cannot either say "we know what this value is because we
> > know the board" or probe for anyway. I think that is basically
> > ram inf and number of CPUs. (Ram size and numa configuration
> > is added to the dtb by the hw/arm/boot.c code, which sets up
> > the "/memory" nodes.)
> >
> > You shouldn't need to put the GIC info into the DT, because
> > you can just hard code into UEFI where it is in memory.
> >
> I can drop GIC, since memory is handled at boot.c, then only CPU node left.
> And some items insides the CPU node can be removed too, I'll check.
>
> > >> > +static void create_ehci(const VirtMachineState *vms, qemu_irq *pic)
> > >> > +{
> > >> > +    hwaddr base = vms->memmap[VIRT_EHCI].base;
> > >> > +    int irq = vms->irqmap[VIRT_EHCI];
> > >> > +    USBBus *usb_bus;
> > >> > +
> > >> > +    sysbus_create_simple("exynos4210-ehci-usb", base, pic[irq]);
> > >>
> > >> What is this using the exynos4210 USB device for? That
> > >> is definitely not correct for a generic board.
> > >>
> > > Checked the code:
> > > #define TYPE_SYS_BUS_EHCI "sysbus-ehci-usb"
> > > #define TYPE_EXYNOS4210_EHCI "exynos4210-ehci-usb"
> > > #define TYPE_TEGRA2_EHCI "tegra2-ehci-usb"
> > > #define TYPE_PPC4xx_EHCI "ppc4xx-ehci-usb"
> > > #define TYPE_FUSBH200_EHCI "fusbh200-ehci-usb"
> > >
> > > The first one seems only a parent type, not a general instance, cannot
> > > be used directly.
> > > The other four are specific instances using the first as parent type,
> > > one of them can be chosen to use.
> > > That is my understanding, any comments, or do we need to implement one
> > > which seems generic?
> >
> > You need to work out what you want and implement that;
> > I don't really know enough about USB to advise.
> > You probably also need to consider how you want
> > the USB companion chip stuff to work -- I know that
> > we've had a bug report that the Exynos4210 USB setup
> > is not getting that right, so it may be a poor example
> > to copy from.
> >
> I don't think we need to implement any new 'generic' one, if we do
> this in QEMU, we have to finish its driver in kernel too.
> For the current condition, if there is something wrong with exynos4210
> usb, we have to pick other one, such as the tegra2 one, the only issue
> for such a usb block is its name is not general.
> (But I found there is a generic 'qemu-xhci', I will check further more.)
>
The generic 'qemu-xhci' is a PCI one, not so good here.

> > >> > +
> > >> > +    usb_bus = usb_bus_find(-1);
> > >> > +    usb_create_simple(usb_bus, "usb-kbd");
> > >> > +    usb_create_simple(usb_bus, "usb-mouse");
> > >>
> > >> Don't create USB devices by default -- let the user do it on
> > >> the command line.
> > >>
> > > Some users hope that, I can remove it if not reasonable.
> >
> > Almost no other board models do it (only a few PPC ones
> > and an sh4 board). Generally for QEMU the approach we take
> > is that we don't provide defaults, we expect the user
> > (or the 'management layer' software like libvirt) to
> > specify what they want. This allows users to specify that
> > they *don't* want a usb keyboard, for instance.
> >
> > >> > +    /* If we have an EL3 boot ROM then the assumption is that it will
> > >> > +     * implement PSCI itself, so disable QEMU's internal 
> > >> > implementation
> > >> > +     * so it doesn't get in the way. Instead of starting secondary
> > >> > +     * CPUs in PSCI powerdown state we will start them all running and
> > >> > +     * let the boot ROM sort them out.
> > >> > +     * The usual case is that we do use QEMU's PSCI implementation;
> > >> > +     * if the guest has EL2 then we will use SMC as the conduit,
> > >> > +     * and otherwise we will use HVC (for backwards compatibility and
> > >> > +     * because if we're using KVM then we must use HVC).
> > >> > +     */
> > >> > +    if (vms->secure && firmware_loaded) {
> > >> > +        vms->psci_conduit = QEMU_PSCI_CONDUIT_DISABLED;
> > >> > +    } else if (vms->virt) {
> > >> > +        vms->psci_conduit = QEMU_PSCI_CONDUIT_SMC;
> > >> > +    } else {
> > >> > +        vms->psci_conduit = QEMU_PSCI_CONDUIT_HVC;
> > >> > +    }
> > >>
> > >> Do you actually intend this to work in all these cases? I
> > >> thought the intention was "always start the boot rom in EL3"
> > >> for this board, like the real hardware would. In that case, most
> > >> of these if clauses are dead code.
> > >>
> > > Well, I myself prefer "always enable EL3" too.
> > > But during this work, I found that if EL2/EL3 enabled, QEMU runs much
> > > slower than without EL2/3, so I guess if there would be some user of
> > > this platform, for the reason of speed they choose to disable EL2/3,
> > > should we give them such a chance?
> > > If get confirmation that we always enable EL2/3, that is fine for me,
> > > this will make codes cleaner.
> > >
> > > (By the way, I wonder why QEMU becomes much slower with EL2/3 enabled,
> > > because when OS is running, most of the time, we don't call into
> > > EL2/3, does this mean there is a big space for us to optimize it?)
> >
> > I think users who don't care about the EL2/EL3 parts of the software
> > stack should just use the "virt" machine instead -- that is the
> > board we have for "run as fast as possible and just boot a kernel".
> >
> OK, since we have 'virt' for speed, I'll remove the EL2/3
> enable/disable codes, let it enabled always.
>
> > It would be interesting at some point to find out why EL2 and EL3
> > are causing slowdowns, yes.
> >
> Hope it can be optimized too.
>
> > thanks
> > -- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]