Re: [PATCH v2 00/13] microvm: add acpi support

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, May 06, 2020 at 01:46:35PM +0200, Gerd Hoffmann wrote:
> On Tue, May 05, 2020 at 10:04:02AM -0400, Michael S. Tsirkin wrote:
> > On Tue, May 05, 2020 at 03:42:52PM +0200, Gerd Hoffmann wrote:
> > > I know that not supporting ACPI in microvm is intentional.  If you still
> > > don't want ACPI this is perfectly fine, you can use the usual -no-acpi
> > > switch to toggle ACPI support.
> > > 
> > > These are the advantages you are going to loose then:
> > > 
> > >   (1) virtio-mmio device discovery without command line hacks (tweaking
> > >       the command line is a problem when not using direct kernel boot).
> > >   (2) Better IO-APIC support, we can use IRQ lines 16-23.
> > >   (3) ACPI power button (aka powerdown request) works.
> > >   (4) machine poweroff (aka S5 state) works.
> > 
> > Questions
> > 
> > - what's the tradeoff in startup time?
> 
> In the noise.  0.28-0.29 seconds on my hardware to the "i8042: PNP: No
> PS/2 controller found" message, no matter whenever acpi is on or off.
> With "quiet" (acpi prints more and logging to the serial console is
> slow).
> 
> At that point -no-acpi takes one second to figure the ps2 controller
> really isn't there (as discussed before).
> 
> Another interesting difference is interrupt handling.
> 
> The -no-acpi version:
> 
>            CPU0       
>   2:          0    XT-PIC      cascade
>   4:        284   IO-APIC   4-edge      ttyS0
>   8:          0   IO-APIC   8-edge      rtc0
>  14:       5399   IO-APIC  14-edge      virtio1
>  15:         58   IO-APIC  15-edge      virtio0
> NMI:          0   Non-maskable interrupts
> [ ... ]
> 
> The acpi version:
> 
>            CPU0       
>   1:          0   IO-APIC   9-edge      ACPI:Ged
>   2:        231   IO-APIC  23-fasteoi   virtio0
>   3:       6291   IO-APIC  22-fasteoi   virtio1
>   4:       1758   IO-APIC   4-edge      ttyS0
>   5:          0   IO-APIC   8-edge      rtc0
> NMI:          0   Non-maskable interrupts
> [ ... ]
> 
> > - what should be the default?
> 
> IMO it makes sense to enable it by default.  You get working
> power management.  You can boot stock cloud images (patched
> seabios parses the dsdt to find virtio-mmio devices to boot
> from virtio-mmio disks).
> 
> It's easier to leave behind legacy stuff:  The kernel trusts the
> firmware and doesn't go into "trying harder to find ps2 kbd" mode.
> Also what is this "cascade" thing in /proc/interrupts above? [1]
> 
> I expect dropping the rtc is easier with acpi too, the kernel probably
> wouldn't try to find it then.  Right now seabios needs rtc cmos for
> ram size probing, so I didn't test that yet.
> 
> On the other hand I don't really see any disadvantages.  The tables are
> small ...
> 
> # find /sys/firmware/acpi/tables/ -type f | xargs ls -l
> -r--------. 1 root root  70 May  6 06:48 /sys/firmware/acpi/tables/APIC
> -r--------. 1 root root 472 May  6 06:48 /sys/firmware/acpi/tables/DSDT
> -r--------. 1 root root 268 May  6 06:48 /sys/firmware/acpi/tables/FACP
> 
> ... and simple (no methods) so you can hardly call that "bloat".
> 
> > Based on above I'd be inclined to say default should stay no acpi and
> > users should enable acpi with an option.
> 
> I disagree, but I can live with off by default too.  We already have
> acpi=OnOffAuto for X86MachineState, so it is just a matter of handling
> microvm.acpi=auto accordingly in x86_machine_is_acpi_enabled().
> 
> take care,
>   Gerd
> 
> [1] Rhetorical question, I know what it is. [2]
> [2] I don't want remember though.

Let's leave flipping the default as a separate patch, to be
decided on merits after a bunch of people test with/without.

-- 
MST