On Tue, Feb 19, 2019 at 4:15 PM Waldemar Kozaczuk <jwkozac...@gmail.com>
wrote:
> This patch makes OSv boot without requiring ACPI
> to be present which for example is the case on firecracker.
>
> We simply treat failure to find ACPI root pointer
> as an indicator that ACPI is not available and
> mark it off as such. Also if ACPI is off we power OSv off
> using non-ACPI method and skip probing panic driver as it
> relies on ACPI as well.
>
> Lastly we skip MADT table parsing if APCI is off and
> for now assume there is single vCPU only. Eventually
> we should parse vCPU information from MP table as
> an alternative.
>
Looks good. A few minor comments below.
> Signed-off-by: Waldemar Kozaczuk <jwkozac...@gmail.com>
> ---
> arch/x64/power.cc | 26 ++++++++++++++++++--------
> arch/x64/smp.cc | 15 ++++++++++++++-
> drivers/acpi.cc | 17 +++++++++++++++--
> drivers/acpi.hh | 1 +
> drivers/pvpanic.cc | 4 ++++
> 5 files changed, 52 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x64/power.cc b/arch/x64/power.cc
> index 81849335..0dc1ebb8 100644
> --- a/arch/x64/power.cc
> +++ b/arch/x64/power.cc
> @@ -15,6 +15,8 @@ extern "C" {
> #include "acpi.h"
> }
>
> +#include <drivers/acpi.hh>
> +
> namespace osv {
>
> void halt(void)
> @@ -27,15 +29,23 @@ void halt(void)
>
> void poweroff(void)
> {
> - ACPI_STATUS status = AcpiEnterSleepStatePrep(ACPI_STATE_S5);
> - if (ACPI_FAILURE(status)) {
> - debug("AcpiEnterSleepStatePrep failed: %s\n",
> AcpiFormatException(status));
> - halt();
> + if (acpi::is_enabled()) {
> + ACPI_STATUS status = AcpiEnterSleepStatePrep(ACPI_STATE_S5);
> + if (ACPI_FAILURE(status)) {
> + debug("AcpiEnterSleepStatePrep failed: %s\n",
> AcpiFormatException(status));
> + halt();
> + }
> + status = AcpiEnterSleepState(ACPI_STATE_S5);
> + if (ACPI_FAILURE(status)) {
> + debug("AcpiEnterSleepState failed: %s\n",
> AcpiFormatException(status));
> + halt();
> + }
> }
> - status = AcpiEnterSleepState(ACPI_STATE_S5);
> - if (ACPI_FAILURE(status)) {
> - debug("AcpiEnterSleepState failed: %s\n",
> AcpiFormatException(status));
> - halt();
> + else {
>
Please put the else on the same line as the previous }, that's how most of
the OSv code is styled.
+ // On hypervisors that do not support ACPI like firecracker we
> + // resort to a reset using the 8042 PS/2 Controller ("keyboard
> controller")
> + // as a way to shutdown the VM
> + processor::outb(0xfe, 0x64);
>
This is basically what reboot() does (although it's not currently its first
option). It's not a poweroff, but a reboot. Does it cause a poweroff in
firecracker and not a reboot?
> }
>
> // We shouldn't get here on x86.
> diff --git a/arch/x64/smp.cc b/arch/x64/smp.cc
> index 073ef206..883216d3 100644
> --- a/arch/x64/smp.cc
> +++ b/arch/x64/smp.cc
> @@ -15,6 +15,7 @@
> extern "C" {
> #include "acpi.h"
> }
> +#include <drivers/acpi.hh>
> #include <boost/intrusive/parent_from_member.hpp>
> #include <osv/debug.hh>
> #include <osv/sched.hh>
> @@ -74,7 +75,19 @@ void parse_madt()
>
> void smp_init()
> {
> - parse_madt();
> + if (acpi::is_enabled()) {
> + parse_madt();
> + }
> + else {
> + //TODO: This a nasty hack to support single vCPU. Eventually we
> should
> + // parse out equivalent information about all vCPUs from MP table
> + auto c = new sched::cpu(0);
> + c->arch.apic_id = 0;
> + c->arch.initstack.next = smp_stack_free;
> + smp_stack_free = &c->arch.initstack;
> + sched::cpus.push_back(c);
>
You can put this case in a new function, say parse_mp(), and add to it a
comment that its implementation is incomplete.
Can you include in that TODO a link describing what "MP table" is?
+ }
> +
> sched::current_cpu = sched::cpus[0];
> for (auto c : sched::cpus) {
> c->incoming_wakeups =
> aligned_array_new<sched::cpu::incoming_wakeup_queue>(sched::cpus.size());
> diff --git a/drivers/acpi.cc b/drivers/acpi.cc
> index 506ca68f..451e8f79 100644
> --- a/drivers/acpi.cc
> +++ b/drivers/acpi.cc
> @@ -52,7 +52,7 @@ ACPI_PHYSICAL_ADDRESS AcpiOsGetRootPointer()
> ACPI_SIZE rsdp;
> auto st = AcpiFindRootPointer(&rsdp);
> if (ACPI_FAILURE(st)) {
> - abort();
> + return 0;
>
This change will not be necessary if we never call the ACPI functions when
AcpiFindRootPointer() returns zero. More on this below.
}
> return rsdp;
> }
> @@ -539,13 +539,20 @@ namespace acpi {
>
> static ACPI_TABLE_DESC TableArray[ACPI_MAX_INIT_TABLES];
>
> +static bool enabled = false;
> +
> +bool is_enabled() {
> + return enabled;
> +}
> +
> void early_init()
> {
> ACPI_STATUS status;
>
> status = AcpiInitializeTables(TableArray, ACPI_MAX_INIT_TABLES, TRUE);
> if (ACPI_FAILURE(status)) {
>
I think it would be good to call AcpiFindRootPointer() explicitly first,
and if it's zero, stop immediately.
This will make the changes you did to AcpiOsGetRootPointer() above, and to
the error reporting
below, unnecessary.
- acpi_e("AcpiInitializeTables failed: %s\n",
> AcpiFormatException(status));
> + //TODO: Report error differently -> AcpiFormatException seems to
> trigger exit on firecracker
> + //acpi_e("AcpiInitializeTables failed: %s\n",
> AcpiFormatException(status));
>
return;
> }
>
> @@ -569,6 +576,8 @@ void early_init()
> acpi_e("AcpiLoadTables failed: %s\n",
> AcpiFormatException(status));
> return;
> }
> +
> + enabled = true;
> }
>
> UINT32 acpi_poweroff(void *unused)
> @@ -581,6 +590,10 @@ UINT32 acpi_poweroff(void *unused)
> // The following function comes from the documentation example page 262
> void init()
> {
> + if (!enabled) {
> + return;
> + }
> +
> ACPI_STATUS status;
>
>
> diff --git a/drivers/acpi.hh b/drivers/acpi.hh
> index 8373745b..10f82f58 100644
> --- a/drivers/acpi.hh
> +++ b/drivers/acpi.hh
> @@ -12,6 +12,7 @@
> namespace acpi {
>
> void init();
> +bool is_enabled();
>
> }
>
> diff --git a/drivers/pvpanic.cc b/drivers/pvpanic.cc
> index 2f114fb3..e643b5a0 100644
> --- a/drivers/pvpanic.cc
> +++ b/drivers/pvpanic.cc
> @@ -21,6 +21,10 @@ static u32 port;
>
> void probe_and_setup()
> {
> + if (!acpi::is_enabled()) {
> + return;
> + }
> +
> ACPI_BUFFER results;
> ACPI_OBJECT obj;
> ACPI_STATUS status;
> --
> 2.19.1
>
> --
> You received this message because you are subscribed to the Google Groups
> "OSv Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to osv-dev+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
--
You received this message because you are subscribed to the Google Groups "OSv
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
