Re: [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled

[Andrew Jones]

On Fri, Jul 03, 2020 at 03:41:05PM +0100, Peter Maydell wrote:
> On Fri, 3 Jul 2020 at 15:36, Heyi Guo <guoheyi@linux.alibaba.com> wrote:
> >
> >
> > 在 2020/7/3 下午6:37, Peter Maydell 写道:
> > > On Fri, 3 Jul 2020 at 10:44, Heyi Guo <guoheyi@linux.alibaba.com> wrote:
> > >> vms->psci_conduit being disabled only means PSCI is not implemented by
> > >> qemu; it doesn't mean PSCI is not supported on this virtual machine.
> > >> Actually vms->psci_conduit is set to disabled when vms->secure and
> > >> firmware_loaded are both set, which means we will run ARM trusted
> > >> firmware, which will definitely provide PSCI.
> > >>
> > >> The issue can be reproduced when running qemu in TCG mode with secure
> > >> enabled, while using ARM trusted firmware + qemu virt UEFI as firmware
> > >> binaries, and we can see secondary cores will not be waken up.
> > > If you're using a real EL3 guest firmware then it's the job of
> > > the guest firmware to provide a DTB to the guest EL2/EL1 that says
> > > "and I support PSCI" if it supports PSCI, surely? QEMU can't tell
> > > whether the EL3 code does or doesn't do that...
> >
> > Thanks, Peter. Does that mean the ACPI tables generated in qemu are only
> > templates and firmware should update them if necessary?
> 
> I don't really know enough about ACPI to say. I hadn't noticed
> that this patch only updated the ACPI tables, sorry. Perhaps it
> is correct; Andrew will probably know better than me.
>

This seems a bit messy to me. With an EL3 firmware, the DTB is provided
by the EL3 firmware. I guess that's why when I look at the DTB generation
in virt.c we don't properly set "enable-method" of the CPUs to
"spin-table", even though we don't set it to "psci"[*]. However, if the
EL3 firmware isn't providing the ACPI tables too, then how do the DTB
and ACPI tables stay in synch? We can't be sure that just because we
have (vms->secure && firmware_loaded) that we should / should not
generate certain ACPI table entries when we don't also know what the
corresponding DTB will be.

So, I think the EL3 firmware should also provide the ACPI tables.
However, this patch it probably fine too. For a configuration where
the EL3 firmware provides the ACPI tables, it will do no harm. For
configurations where EL3 firmware isn't involved, it will do no harm.
And, for configurations like this, which I consider a bit hacky, it's
probably better to assume PSCI than not.

Thanks,
drew

[*] kernel doc Documentation/devicetree/bindings/arm/cpus.yaml says
    "enable-method" must be "spin-table" or "psci"