On Mon, 19 Nov 2018 at 18:49, Hongbo Zhang <hongbo.zh...@linaro.org> wrote: > > On Fri, 16 Nov 2018 at 19:29, Peter Maydell <peter.mayd...@linaro.org> wrote: > > > > On 16 November 2018 at 10:46, Hongbo Zhang <hongbo.zh...@linaro.org> wrote: > > > On Fri, 16 Nov 2018 at 00:05, Peter Maydell <peter.mayd...@linaro.org> > > > 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