Re: [PATCH] xen/arm: Remove EXPERT dependancy

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 23 Oct 2020, Julien Grall wrote:
> Hi Stefano,
> 
> On 22/10/2020 22:17, Stefano Stabellini wrote:
> > On Thu, 22 Oct 2020, Julien Grall wrote:
> > > On 22/10/2020 02:43, Elliott Mitchell wrote:
> > > > Linux requires UEFI support to be enabled on ARM64 devices.  While many
> > > > ARM64 devices lack ACPI, the writing seems to be on the wall of
> > > > UEFI/ACPI
> > > > potentially taking over.  Some common devices may need ACPI table
> > > > support.
> > > > 
> > > > Presently I think it is worth removing the dependancy on CONFIG_EXPERT.
> > > 
> > > The idea behind EXPERT is to gate any feature that is not considered to be
> > > stable/complete enough to be used in production.
> > 
> > Yes, and from that point of view I don't think we want to remove EXPERT
> > from ACPI yet. However, the idea of hiding things behind EXPERT works
> > very well for new esoteric features, something like memory introspection
> > or memory overcommit.
> 
> Memaccess is not very new ;).
> 
> > It does not work well for things that are actually
> > required to boot on the platform.
> 
> I am not sure where is the problem. It is easy to select EXPERT from the
> menuconfig. It also hints the user that the feature may not fully work.
> 
> > 
> > Typically ACPI systems don't come with device tree at all (RPi4 being an
> > exception), so users don't really have much of a choice in the matter.
> 
> And they typically have IOMMUs.
> 
> > 
> >  From that point of view, it would be better to remove EXPERT from ACPI,
> > maybe even build ACPI by default, *but* to add a warning at boot saying
> > something like:
> > 
> > "ACPI support is experimental. Boot using Device Tree if you can."
> > 
> > 
> > That would better convey the risks of using ACPI, while at the same time
> > making it a bit easier for users to boot on their ACPI-only platforms.
> 
> Right, I agree that this make easier for users to boot Xen on ACPI-only
> platform. However, based on above, it is easy enough for a developper to
> rebuild Xen with ACPI and EXPERT enabled.
> 
> So what sort of users are you targeting?

Somebody trying Xen for the first time, they might know how to build it
but they might not know that ACPI is not available by default, and they
might not know that they need to enable EXPERT in order to get the ACPI
option in the menu. It is easy to do once you know it is there,
otherwise one might not know where to look in the menu.


> I am sort of okay to remove EXPERT. 

OK. This would help (even without building it by default) because as you
go and look at the menu the first time, you'll find ACPI among the
options right away.


> But I still think building ACPI by default
> is still wrong because our default .config is meant to be (security)
> supported. I don't think ACPI can earn this qualification today.

Certainly we don't want to imply ACPI is security supported. I was
looking at SUPPORT.md and it is only says:

"""
EXPERT and DEBUG Kconfig options are not security supported. Other
Kconfig options are supported, if the related features are marked as
supported in this document.
"""

So technically we could enable ACPI in the build by default as ACPI for
ARM is marked as experimental. However, I can see that it is not a
great idea to enable by default an unsupported option in the kconfig, so
from that point of view it might be best to leave ACPI disabled by
default. Probably the best compromise at this time.



> In order to remove EXPERT, there are a few things to needs to be done (or
> checked):
>     1) SUPPORT.MD has a statement about ACPI on Arm

### Host ACPI (via Domain 0)

    Status, x86 PV: Supported
    Status, ARM: Experimental



>     2) DT is favored over ACPI if the two firmware tables are present.

Good idea. xen/arch/arm/acpi/boot.c:acpi_boot_table_init has:

    /*
     * Enable ACPI instead of device tree unless
     * - ACPI has been disabled explicitly (acpi=off), or
     * - the device tree is not empty (it has more than just a /chosen node)
     *   and ACPI has not been force enabled (acpi=force)
     */
    if ( param_acpi_off)
        goto disable;
    if ( !param_acpi_force &&
         device_tree_for_each_node(device_tree_flattened, 0,
                                   dt_scan_depth1_nodes, NULL) )
        goto disable;

We should be fine.