Patch : Add ACPI poweroff
407 views
Skip to first unread message
Xiaoguang Sun
May 21, 2013, 2:58:22 AM5/21/13
to min...@googlegroups.com
Add ACPI poweroff so poweroff and shutdown -p can actually work on real system with ACPI support.
This patch doesn't use full AML interpreter to look for _S5_, instead it uses a way described at here http://forum.osdev.org/viewtopic.php?t=16990 which is simple and good enough.
I found Bochs and QEmu doesn't work well with current acpi driver, and it also doesn't work with this patch. So the original magic Bochs/QEmu shutdown command is still there in case ACPI poweroff is not possible.
This patch doesn't use full AML interpreter to look for _S5_, instead it uses a way described at here http://forum.osdev.org/viewtopic.php?t=16990 which is simple and good enough.
I found Bochs and QEmu doesn't work well with current acpi driver, and it also doesn't work with this patch. So the original magic Bochs/QEmu shutdown command is still there in case ACPI poweroff is not possible.
Tomas Hruby
May 21, 2013, 4:06:44 AM5/21/13
to min...@googlegroups.com
Hi,
On Mon, May 20, 2013 at 11:58:22PM -0700, Xiaoguang Sun wrote:
> Add ACPI poweroff so poweroff and shutdown -p can actually work on real
> system with ACPI support.
Nice!
> This patch doesn't use full AML interpreter to look for _S5_, instead it
> uses a way described at here http://forum.osdev.org/viewtopic.php?t=16990which is simple and good enough.
This is good enough. Have you considered the option that the acpi
driver would provide the information?
I have a few comments, see below. In general, you do not follow the
coding style.
T.
> From 758a7ec01c8430d7a8dc6f8fdea32137516649b8 Mon Sep 17 00:00:00 2001
> From: Xiaoguang Sun <sun.xi...@yoyosys.com>
> Date: Tue, 21 May 2013 13:26:16 +0800
> Subject: [PATCH] Add acpi poweroff
>
> Use acpi poweroff if it's possible.
> ---
> kernel/arch/i386/acpi.c | 99 ++++++++++++++++++++++++++++++++++++++++++-
> kernel/arch/i386/acpi.h | 66 +++++++++++++++++++++++++++++
> kernel/arch/i386/arch_reset.c | 7 +++
> 3 files changed, 171 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/arch/i386/acpi.c b/kernel/arch/i386/acpi.c
> index 225467b..d2ae46c 100644
> --- a/kernel/arch/i386/acpi.c
> +++ b/kernel/arch/i386/acpi.c
> @@ -24,6 +24,11 @@ static struct {
> } sdt_trans[MAX_RSDT];
>
> static int sdt_count;
> +static u16_t pm1a_cnt_blk = 0;
> +static u16_t pm1b_cnt_blk = 0;
> +static u16_t slp_typa = 0;
Use the same indentation as the files you are changing!
> +static u16_t slp_typb = 0;
> +static u16_t slp_en = 1 << 13;
Define numeral constants so the code is more readable!
> static int acpi_check_csum(struct acpi_sdt_header * tb, size_t size)
> {
> @@ -210,11 +215,85 @@ static int get_acpi_rsdp(void)
> return 0;
> }
>
> +static void acpi_init_poweroff()
Use (void) where the function does not take any arguments.
> +{
> + u8_t *ptr = NULL;
> + u8_t *start = NULL;
> + u8_t *end = NULL;
> + struct acpi_fadt_header *fadt_header = NULL;
> + struct acpi_rsdt * dsdt_header = NULL;
> + char *msg = NULL;
> +
> + /* everything used here existed since ACPI spec 1.0 so we can safely use them */
> + fadt_header = (struct acpi_fadt_header *) acpi_phys2vir(acpi_get_table_base("FACP"));
Do not exceed 80 characters per line, even if it is sometimes difficult.
> + if (fadt_header == NULL) {
> + msg = "Could not load FACP";
> + goto exit;
> + }
> +
> + dsdt_header = (struct acpi_rsdt *) acpi_phys2vir((phys_bytes) fadt_header->dsdt);
> + if (dsdt_header == NULL) {
> + msg = "Could not load DSDT";
> + goto exit;
> + }
> +
> + ptr = start = (u8_t *) dsdt_header->data;
> + end = start + dsdt_header->hdr.length - 4;
> +
> + while (ptr < end && memcmp(ptr, "_S5_", 4) != 0) { ++ptr; }
Put ++ptr on a new line. In general, do not use { } around single
statements unless it is a very long, multi-line one.
Imo, ptr++ is more common than ++ptr in this case.
> +
> + msg = "Could not read S5 data";
> + if (ptr >= end || ptr == start) {
> + goto exit;
> + }
> +
> + /* validate AML structure */
> + if (*(ptr + 4) != 0x12) {
Again, 0x12 should be a meaningful constatnt. This function uses a lot
of constatnts. You could consider using defines from acpica headers,
howeve, redefining them in acpi.h is probably better.
> + goto exit;
> + }
> +
> + if ((ptr < start + 2 || (*(ptr - 2) != 0x08 || *(ptr - 1) != '\\')) && *(ptr - 1) != 0x8) {
> + goto exit;
> + }
> +
> + ptr += 5;
> + if (ptr >= end) {
> + goto exit;
> + }
> +
> + /* package length */
> + ptr += ((*ptr & 0xC0) >> 6) + 2;
> + if (ptr >= end) {
> + goto exit;
> + }
> +
> + if (*ptr == 0xA) {
> + ++ptr; /* skip byte prefix */
> + }
> +
> + slp_typa = (*ptr) << 10;
> +
> + if (*(++ptr) == 0xA) {
> + ++ptr; /* skip byte prefix */
> + }
> + slp_typb = (*ptr) << 10;
> +
> + pm1a_cnt_blk = fadt_header->pm1a_cnt_blk;
> + pm1b_cnt_blk = fadt_header->pm1b_cnt_blk;
> +
> + msg = "poweroff initialized";
> +
> +exit:
> + if (msg) {
> + printf("acpi: %s\n", msg);
> + }
> +}
> +
> void acpi_init(void)
> {
> int s, i;
> read_func = acpi_phys_copy;
> -
> +
Do not introduce unrelated white space changes! Do not leave white
space at the end of lines. Editors can highlight it and git am helps
you when you test whether your patch applies cleanly.
> if (!get_acpi_rsdp()) {
> printf("WARNING : Cannot configure ACPI\n");
> return;
> @@ -239,6 +318,8 @@ void acpi_init(void)
> sdt_trans[i].signature[ACPI_SDT_SIGNATURE_LEN] = '\0';
> sdt_trans[i].length = hdr.length;
> }
> +
> + acpi_init_poweroff();
> }
>
> struct acpi_madt_ioapic * acpi_get_ioapic_next(void)
> @@ -293,3 +374,19 @@ struct acpi_madt_lapic * acpi_get_lapic_next(void)
>
> return ret;
> }
> +
> +void __k_unpaged_acpi_poweroff(void)
> +{
> + /* NO OP poweroff symbol*/
> +}
> +
> +void acpi_poweroff(void)
> +{
> + if (pm1a_cnt_blk == 0) {
> + return;
> + }
> + outw(pm1a_cnt_blk, slp_typa | slp_en);
> + if (pm1b_cnt_blk != 0) {
> + outw(pm1b_cnt_blk, slp_typb | slp_en);
> + }
> +}
> diff --git a/kernel/arch/i386/acpi.h b/kernel/arch/i386/acpi.h
> index ddbd504..132d9d0 100644
> --- a/kernel/arch/i386/acpi.h
> +++ b/kernel/arch/i386/acpi.h
> @@ -30,6 +30,70 @@ struct acpi_sdt_header {
> u32_t creator_revision;
> };
>
> +struct acpi_generic_address {
> + u8_t address_space_id;
> + u8_t register_bit_width;
> + u8_t register_bit_offset;
> + u8_t access_size;
> + u64_t address;
> +};
> +
> +struct acpi_fadt_header
> +{
> + struct acpi_sdt_header hdr;
> + u32_t facs;
> + u32_t dsdt;
> + u8_t model;
> + u8_t preferred_pm_profile;
> + u16_t sci_int;
> + u32_t smi_cmd;
> + u8_t acpi_enable;
> + u8_t acpi_disable;
> + u8_t s4bios_req;
> + u8_t pstate_cnt;
> + u32_t pm1a_evt_blk;
> + u32_t pm1b_evt_blk;
> + u32_t pm1a_cnt_blk;
> + u32_t pm1b_cnt_blk;
> + u32_t pm2_cnt_blk;
> + u32_t pm_tmr_blk;
> + u32_t gpe0_blk;
> + u32_t gpe1_blk;
> + u8_t pm1_evt_len;
> + u8_t pm1_cnt_len;
> + u8_t pm2_cnt_len;
> + u8_t pm_tmr_len;
> + u8_t gpe0_blk_len;
> + u8_t gpe1_blk_len;
> + u8_t gpe1_base;
> + u8_t cst_cnt;
> + u16_t p_lvl2_lat;
> + u16_t p_lvl3_lat;
> + u16_t flush_size;
> + u16_t flush_stride;
> + u8_t duty_offset;
> + u8_t duty_width;
> + u8_t day_alrm;
> + u8_t mon_alrm;
> + u8_t century;
> + u16_t iapc_boot_arch;
> + u8_t reserved1;
> + u32_t flags;
> + struct acpi_generic_address reset_reg;
> + u8_t reset_value;
> + u8_t reserved2[3];
> + u64_t xfacs;
> + u64_t xdsdt;
> + struct acpi_generic_address xpm1a_evt_blk;
> + struct acpi_generic_address xpm1b_evt_blk;
> + struct acpi_generic_address xpm1a_cnt_blk;
> + struct acpi_generic_address xpm1b_cnt_blk;
> + struct acpi_generic_address xpm2_cnt_blk;
> + struct acpi_generic_address xpm_tmr_blk;
> + struct acpi_generic_address xgpe0_blk;
> + struct acpi_generic_address xgpe1_blk;
> +};
> +
> struct acpi_madt_hdr {
> struct acpi_sdt_header hdr;
> u32_t local_apic_address;
> @@ -84,6 +148,8 @@ struct acpi_madt_nmi {
>
> void acpi_init(void);
>
> +void acpi_poweroff(void);
> +
> /*
> * Returns a pointer to the io acpi structure in the MADT table in ACPI. The
> * pointer is valid only until paging is turned off. No memory is allocated in
> diff --git a/kernel/arch/i386/arch_reset.c b/kernel/arch/i386/arch_reset.c
> index 2aad6f0..e75fb5a 100644
> --- a/kernel/arch/i386/arch_reset.c
> +++ b/kernel/arch/i386/arch_reset.c
> @@ -22,6 +22,10 @@
> #include "direct_utils.h"
> #include <machine/multiboot.h>
>
> +#ifdef USE_ACPI
> +#include "acpi.h"
> +#endif
> +
> #define KBCMDP 4 /* kbd controller port (O) */
> #define KBC_PULSE0 0xfe /* pulse output bit 0 */
> #define IO_KBD 0x060 /* 8042 Keyboard */
> @@ -85,6 +89,9 @@ poweroff(void)
> {
> const char *shutdown_str;
>
> +#ifdef USE_ACPI
> + acpi_poweroff();
Test whether ACPI is actually used. It is not by default. Only when
APIC is enabled.
> +#endif
> /* Bochs/QEMU poweroff */
> shutdown_str = "Shutdown";
> while (*shutdown_str) outb(0x8900, *(shutdown_str++));
> --
> 1.7.12.4 (Apple Git-37)
>
On Mon, May 20, 2013 at 11:58:22PM -0700, Xiaoguang Sun wrote:
> Add ACPI poweroff so poweroff and shutdown -p can actually work on real
> system with ACPI support.
> This patch doesn't use full AML interpreter to look for _S5_, instead it
This is good enough. Have you considered the option that the acpi
driver would provide the information?
I have a few comments, see below. In general, you do not follow the
coding style.
T.
> From 758a7ec01c8430d7a8dc6f8fdea32137516649b8 Mon Sep 17 00:00:00 2001
> From: Xiaoguang Sun <sun.xi...@yoyosys.com>
> Date: Tue, 21 May 2013 13:26:16 +0800
> Subject: [PATCH] Add acpi poweroff
>
> Use acpi poweroff if it's possible.
> ---
> kernel/arch/i386/acpi.c | 99 ++++++++++++++++++++++++++++++++++++++++++-
> kernel/arch/i386/acpi.h | 66 +++++++++++++++++++++++++++++
> kernel/arch/i386/arch_reset.c | 7 +++
> 3 files changed, 171 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/arch/i386/acpi.c b/kernel/arch/i386/acpi.c
> index 225467b..d2ae46c 100644
> --- a/kernel/arch/i386/acpi.c
> +++ b/kernel/arch/i386/acpi.c
> @@ -24,6 +24,11 @@ static struct {
> } sdt_trans[MAX_RSDT];
>
> static int sdt_count;
> +static u16_t pm1a_cnt_blk = 0;
> +static u16_t pm1b_cnt_blk = 0;
> +static u16_t slp_typa = 0;
Use the same indentation as the files you are changing!
> +static u16_t slp_typb = 0;
> +static u16_t slp_en = 1 << 13;
Define numeral constants so the code is more readable!
> static int acpi_check_csum(struct acpi_sdt_header * tb, size_t size)
> {
> @@ -210,11 +215,85 @@ static int get_acpi_rsdp(void)
> return 0;
> }
>
> +static void acpi_init_poweroff()
Use (void) where the function does not take any arguments.
> +{
> + u8_t *ptr = NULL;
> + u8_t *start = NULL;
> + u8_t *end = NULL;
> + struct acpi_fadt_header *fadt_header = NULL;
> + struct acpi_rsdt * dsdt_header = NULL;
> + char *msg = NULL;
> +
> + /* everything used here existed since ACPI spec 1.0 so we can safely use them */
> + fadt_header = (struct acpi_fadt_header *) acpi_phys2vir(acpi_get_table_base("FACP"));
Do not exceed 80 characters per line, even if it is sometimes difficult.
> + if (fadt_header == NULL) {
> + msg = "Could not load FACP";
> + goto exit;
> + }
> +
> + dsdt_header = (struct acpi_rsdt *) acpi_phys2vir((phys_bytes) fadt_header->dsdt);
> + if (dsdt_header == NULL) {
> + msg = "Could not load DSDT";
> + goto exit;
> + }
> +
> + ptr = start = (u8_t *) dsdt_header->data;
> + end = start + dsdt_header->hdr.length - 4;
> +
> + while (ptr < end && memcmp(ptr, "_S5_", 4) != 0) { ++ptr; }
Put ++ptr on a new line. In general, do not use { } around single
statements unless it is a very long, multi-line one.
Imo, ptr++ is more common than ++ptr in this case.
> +
> + msg = "Could not read S5 data";
> + if (ptr >= end || ptr == start) {
> + goto exit;
> + }
> +
> + /* validate AML structure */
> + if (*(ptr + 4) != 0x12) {
Again, 0x12 should be a meaningful constatnt. This function uses a lot
of constatnts. You could consider using defines from acpica headers,
howeve, redefining them in acpi.h is probably better.
> + goto exit;
> + }
> +
> + if ((ptr < start + 2 || (*(ptr - 2) != 0x08 || *(ptr - 1) != '\\')) && *(ptr - 1) != 0x8) {
> + goto exit;
> + }
> +
> + ptr += 5;
> + if (ptr >= end) {
> + goto exit;
> + }
> +
> + /* package length */
> + ptr += ((*ptr & 0xC0) >> 6) + 2;
> + if (ptr >= end) {
> + goto exit;
> + }
> +
> + if (*ptr == 0xA) {
> + ++ptr; /* skip byte prefix */
> + }
> +
> + slp_typa = (*ptr) << 10;
> +
> + if (*(++ptr) == 0xA) {
> + ++ptr; /* skip byte prefix */
> + }
> + slp_typb = (*ptr) << 10;
> +
> + pm1a_cnt_blk = fadt_header->pm1a_cnt_blk;
> + pm1b_cnt_blk = fadt_header->pm1b_cnt_blk;
> +
> + msg = "poweroff initialized";
> +
> +exit:
> + if (msg) {
> + printf("acpi: %s\n", msg);
> + }
> +}
> +
> void acpi_init(void)
> {
> int s, i;
> read_func = acpi_phys_copy;
> -
> +
Do not introduce unrelated white space changes! Do not leave white
space at the end of lines. Editors can highlight it and git am helps
you when you test whether your patch applies cleanly.
> if (!get_acpi_rsdp()) {
> printf("WARNING : Cannot configure ACPI\n");
> return;
> @@ -239,6 +318,8 @@ void acpi_init(void)
> sdt_trans[i].signature[ACPI_SDT_SIGNATURE_LEN] = '\0';
> sdt_trans[i].length = hdr.length;
> }
> +
> + acpi_init_poweroff();
> }
>
> struct acpi_madt_ioapic * acpi_get_ioapic_next(void)
> @@ -293,3 +374,19 @@ struct acpi_madt_lapic * acpi_get_lapic_next(void)
>
> return ret;
> }
> +
> +void __k_unpaged_acpi_poweroff(void)
> +{
> + /* NO OP poweroff symbol*/
> +}
> +
> +void acpi_poweroff(void)
> +{
> + if (pm1a_cnt_blk == 0) {
> + return;
> + }
> + outw(pm1a_cnt_blk, slp_typa | slp_en);
> + if (pm1b_cnt_blk != 0) {
> + outw(pm1b_cnt_blk, slp_typb | slp_en);
> + }
> +}
> diff --git a/kernel/arch/i386/acpi.h b/kernel/arch/i386/acpi.h
> index ddbd504..132d9d0 100644
> --- a/kernel/arch/i386/acpi.h
> +++ b/kernel/arch/i386/acpi.h
> @@ -30,6 +30,70 @@ struct acpi_sdt_header {
> u32_t creator_revision;
> };
>
> +struct acpi_generic_address {
> + u8_t address_space_id;
> + u8_t register_bit_width;
> + u8_t register_bit_offset;
> + u8_t access_size;
> + u64_t address;
> +};
> +
> +struct acpi_fadt_header
> +{
> + struct acpi_sdt_header hdr;
> + u32_t facs;
> + u32_t dsdt;
> + u8_t model;
> + u8_t preferred_pm_profile;
> + u16_t sci_int;
> + u32_t smi_cmd;
> + u8_t acpi_enable;
> + u8_t acpi_disable;
> + u8_t s4bios_req;
> + u8_t pstate_cnt;
> + u32_t pm1a_evt_blk;
> + u32_t pm1b_evt_blk;
> + u32_t pm1a_cnt_blk;
> + u32_t pm1b_cnt_blk;
> + u32_t pm2_cnt_blk;
> + u32_t pm_tmr_blk;
> + u32_t gpe0_blk;
> + u32_t gpe1_blk;
> + u8_t pm1_evt_len;
> + u8_t pm1_cnt_len;
> + u8_t pm2_cnt_len;
> + u8_t pm_tmr_len;
> + u8_t gpe0_blk_len;
> + u8_t gpe1_blk_len;
> + u8_t gpe1_base;
> + u8_t cst_cnt;
> + u16_t p_lvl2_lat;
> + u16_t p_lvl3_lat;
> + u16_t flush_size;
> + u16_t flush_stride;
> + u8_t duty_offset;
> + u8_t duty_width;
> + u8_t day_alrm;
> + u8_t mon_alrm;
> + u8_t century;
> + u16_t iapc_boot_arch;
> + u8_t reserved1;
> + u32_t flags;
> + struct acpi_generic_address reset_reg;
> + u8_t reset_value;
> + u8_t reserved2[3];
> + u64_t xfacs;
> + u64_t xdsdt;
> + struct acpi_generic_address xpm1a_evt_blk;
> + struct acpi_generic_address xpm1b_evt_blk;
> + struct acpi_generic_address xpm1a_cnt_blk;
> + struct acpi_generic_address xpm1b_cnt_blk;
> + struct acpi_generic_address xpm2_cnt_blk;
> + struct acpi_generic_address xpm_tmr_blk;
> + struct acpi_generic_address xgpe0_blk;
> + struct acpi_generic_address xgpe1_blk;
> +};
> +
> struct acpi_madt_hdr {
> struct acpi_sdt_header hdr;
> u32_t local_apic_address;
> @@ -84,6 +148,8 @@ struct acpi_madt_nmi {
>
> void acpi_init(void);
>
> +void acpi_poweroff(void);
> +
> /*
> * Returns a pointer to the io acpi structure in the MADT table in ACPI. The
> * pointer is valid only until paging is turned off. No memory is allocated in
> diff --git a/kernel/arch/i386/arch_reset.c b/kernel/arch/i386/arch_reset.c
> index 2aad6f0..e75fb5a 100644
> --- a/kernel/arch/i386/arch_reset.c
> +++ b/kernel/arch/i386/arch_reset.c
> @@ -22,6 +22,10 @@
> #include "direct_utils.h"
> #include <machine/multiboot.h>
>
> +#ifdef USE_ACPI
> +#include "acpi.h"
> +#endif
> +
> #define KBCMDP 4 /* kbd controller port (O) */
> #define KBC_PULSE0 0xfe /* pulse output bit 0 */
> #define IO_KBD 0x060 /* 8042 Keyboard */
> @@ -85,6 +89,9 @@ poweroff(void)
> {
> const char *shutdown_str;
>
> +#ifdef USE_ACPI
> + acpi_poweroff();
Test whether ACPI is actually used. It is not by default. Only when
APIC is enabled.
> +#endif
> /* Bochs/QEMU poweroff */
> shutdown_str = "Shutdown";
> while (*shutdown_str) outb(0x8900, *(shutdown_str++));
> --
> 1.7.12.4 (Apple Git-37)
>
Xiaoguang Sun
May 21, 2013, 4:25:59 AM5/21/13
to min...@googlegroups.com
Hi Tomas,
I'm not sure If I understand your indentation comment right. I guess you probably meant the whole patch has different indentation but not the lines you quoted for the first comment. But I do have a question, I saw the file used tabs for indentation and my vim setting automatically change tab to space. So the whole minix3 source code suppose to only use tab?
acpi_poweroff itself already checks if acpi is initialized properly. So are you suggesting to have another acpi_enabled() function and then call acpi_poweroff if it returns true.
I will take some time to look at /usr/share/misc/style and see what it says. It's different in many ways than what I use normally.
Xiaoguang
I'm not sure If I understand your indentation comment right. I guess you probably meant the whole patch has different indentation but not the lines you quoted for the first comment. But I do have a question, I saw the file used tabs for indentation and my vim setting automatically change tab to space. So the whole minix3 source code suppose to only use tab?
acpi_poweroff itself already checks if acpi is initialized properly. So are you suggesting to have another acpi_enabled() function and then call acpi_poweroff if it returns true.
I will take some time to look at /usr/share/misc/style and see what it says. It's different in many ways than what I use normally.
Xiaoguang
Tomas Hruby
May 21, 2013, 5:26:22 AM5/21/13
to min...@googlegroups.com
On Tue, May 21, 2013 at 01:25:59AM -0700, Xiaoguang Sun wrote:
> Hi Tomas,
>
> I'm not sure If I understand your indentation comment right. I guess you
> probably meant the whole patch has different indentation but not the lines
> you quoted for the first comment.
In general, indentation of your changes uses 2 spaces instead of TAB
> Hi Tomas,
>
> I'm not sure If I understand your indentation comment right. I guess you
> probably meant the whole patch has different indentation but not the lines
> you quoted for the first comment.
> But I do have a question, I saw the file
> used tabs for indentation and my vim setting automatically change tab to
> space. So the whole minix3 source code suppose to only use tab?
old/different indentation. The general rule is to stick with the
indentation which is "original" for a specific file.
>
> acpi_poweroff itself already checks if acpi is initialized properly. So are
+void acpi_poweroff(void)
+{
+ if (pm1a_cnt_blk == 0) {
+ return;
+ }
I see, this is the test, right? This is fine.
+{
+ if (pm1a_cnt_blk == 0) {
+ return;
+ }
In general, I am fine with your patch. Just fix the coding style and
there is no reason why not to accept it :-)
Xiaoguang Sun
May 21, 2013, 6:32:48 AM5/21/13
to min...@googlegroups.com
Sure, Let me fix the coding style and send patch again.
On Tuesday, May 21, 2013 2:58:22 PM UTC+8, Xiaoguang Sun wrote:
On Tuesday, May 21, 2013 2:58:22 PM UTC+8, Xiaoguang Sun wrote:
Xiaoguang Sun
May 21, 2013, 8:50:47 AM5/21/13
to min...@googlegroups.com
New patch attached.
And Tomas I have another question for you. I recently worked on adding thread support which is a lot of work, but since I'm quite busy and doing this only for hobby, I'm pretty sure it's gonna be a long time before it's finished. There could be many changes during development process and potentially introduce many collisions. In addition, I guess having a huge patch which includes everything is going to be a nightmare for people to review and integrate to source tree. I'm not sure what's the best way of doing it, Can you guys give me some suggestions? Or major features like this should happen inside Minix3 core team instead of having people working outside. If that's the case, I could stop earlier and avoid wasting time on this. Looking forward to hear comments on this from you and other core team developers. Thanks.
Xiaoguang
On Tuesday, May 21, 2013 2:58:22 PM UTC+8, Xiaoguang Sun wrote:
And Tomas I have another question for you. I recently worked on adding thread support which is a lot of work, but since I'm quite busy and doing this only for hobby, I'm pretty sure it's gonna be a long time before it's finished. There could be many changes during development process and potentially introduce many collisions. In addition, I guess having a huge patch which includes everything is going to be a nightmare for people to review and integrate to source tree. I'm not sure what's the best way of doing it, Can you guys give me some suggestions? Or major features like this should happen inside Minix3 core team instead of having people working outside. If that's the case, I could stop earlier and avoid wasting time on this. Looking forward to hear comments on this from you and other core team developers. Thanks.
Xiaoguang
On Tuesday, May 21, 2013 2:58:22 PM UTC+8, Xiaoguang Sun wrote:
Tomas Hruby
May 21, 2013, 11:43:19 AM5/21/13
to min...@googlegroups.com
> And Tomas I have another question for you.
Looks much better. A few more comments below.
> I recently worked on adding
> thread support which is a lot of work, but since I'm quite busy and doing
> this only for hobby, I'm pretty sure it's gonna be a long time before it's
> finished. There could be many changes during development process and
> potentially introduce many collisions. In addition, I guess having a huge
> patch which includes everything is going to be a nightmare for people to
> review and integrate to source tree. I'm not sure what's the best way of
> doing it, Can you guys give me some suggestions? Or major features like
> this should happen inside Minix3 core team instead of having people working
> outside. If that's the case, I could stop earlier and avoid wasting time on
> this. Looking forward to hear comments on this from you and other core team
> developers. Thanks.
chunks, do it! The chunks should make logical sense and should be self
contained. That means, applying it must not break the system and
must compile.
> From 266be14c8f2dbd49df2ad6ff72078355486038e6 Mon Sep 17 00:00:00 2001
> From: Xiaoguang Sun <sun.xi...@yoyosys.com>
> Date: Tue, 21 May 2013 19:55:55 +0800
> Subject: [PATCH] Add acpi poweroff
>
> Use acpi poweroff if it's possible.
> ---
> kernel/arch/i386/acpi.c | 119 ++++++++++++++++++++++++++++++++++++++++++
>
> Use acpi poweroff if it's possible.
> ---
> kernel/arch/i386/acpi.h | 66 +++++++++++++++++++++++
> kernel/arch/i386/arch_reset.c | 7 +++
> 3 files changed, 192 insertions(+)
>
> diff --git a/kernel/arch/i386/acpi.c b/kernel/arch/i386/acpi.c
> index 225467b..2dc5fae 100644
> --- a/kernel/arch/i386/acpi.c
> +++ b/kernel/arch/i386/acpi.c
> @@ -12,6 +12,20 @@ struct acpi_rsdp acpi_rsdp;
> static acpi_read_t read_func;
>
> #define MAX_RSDT 35 /* ACPI defines 35 signatures */
> +#define SLP_EN_CODE (1 << 13) /* ACPI SLP_EN_CODE code */
> +#define AMI_PACKAGE_OP_CODE (0x12)
> +#define AMI_NAME_OP_CODE (0x8)
> +#define AMI_BYTE_PREFIX_CODE (0xA)
> +#define AMI_PACKAGE_LENGTH_ENCODING_BITS_MASK (0xC0)
> +#define AMI_PACKAGE_LENGTH_ENCODING_BITS_SHIFT (0x6)
> +#define AMI_MIN_PACKAGE_LENGTH (1)
> +#define AMI_NUM_ELEMENTS_LENGTH (1)
> +#define AMI_SLP_TYPA_SHIFT (10)
> +#define AMI_SLP_TYPB_SHIFT (10)
> +#define AMI_S5_NAME_OP_OFFSET_1 (-1)
> +#define AMI_S5_NAME_OP_OFFSET_2 (-2)
> +#define AMI_S5_PACKAGE_OP_OFFSET (4)
> +#define AMI_S5_PACKET_LENGTH_OFFSET (5)
>
> static struct acpi_rsdt {
> struct acpi_sdt_header hdr;
> @@ -24,6 +38,10 @@ static struct {
> } sdt_trans[MAX_RSDT];
>
> static int sdt_count;
> +static u16_t pm1a_cnt_blk = 0;
> +static u16_t pm1b_cnt_blk = 0;
> +static u16_t slp_typa = 0;
>
> static int sdt_count;
> +static u16_t pm1a_cnt_blk = 0;
> +static u16_t pm1b_cnt_blk = 0;
> +static u16_t slp_typa = 0;
> +static u16_t slp_typb = 0;
>
>
> static int acpi_check_csum(struct acpi_sdt_header * tb, size_t size)
> {
> @@ -210,6 +228,89 @@ static int get_acpi_rsdp(void)
> {
> return 0;
> }
>
> +static void acpi_init_poweroff(void)
> +{
> + u8_t *ptr = NULL;
> + u8_t *start = NULL;
> + u8_t *end = NULL;
> + struct acpi_fadt_header *fadt_header = NULL;
> + struct acpi_rsdt * dsdt_header = NULL;
> + char *msg = NULL;
> +
> + /* everything used here existed since ACPI spec 1.0 so we can safely use them */
line too long
> + u8_t *ptr = NULL;
> + u8_t *start = NULL;
> + u8_t *end = NULL;
> + struct acpi_fadt_header *fadt_header = NULL;
> + struct acpi_rsdt * dsdt_header = NULL;
> + char *msg = NULL;
> +
> + /* everything used here existed since ACPI spec 1.0 so we can safely use them */
> + fadt_header = (struct acpi_fadt_header *)
> + if (fadt_header == NULL) {
> + msg = "Could not load FACP";
> + goto exit;
> + }
> +
> + dsdt_header = (struct acpi_rsdt *)
> + acpi_phys2vir((phys_bytes) fadt_header->dsdt);
> + msg = "Could not load FACP";
> + goto exit;
> + }
> +
> + dsdt_header = (struct acpi_rsdt *)
> + if (dsdt_header == NULL) {
> + msg = "Could not load DSDT";
> + goto exit;
> + }
> +
> + ptr = start = (u8_t *) dsdt_header->data;
> + end = start + dsdt_header->hdr.length - 4;
> +
> + /* See http://forum.osdev.org/viewtopic.php?t=16990 for layout of \_S5 */
> + msg = "Could not load DSDT";
> + goto exit;
> + }
> +
> + ptr = start = (u8_t *) dsdt_header->data;
> + end = start + dsdt_header->hdr.length - 4;
> +
> + while (ptr < end && memcmp(ptr, "_S5_", 4) != 0)
> + ptr++;
> +
> + msg = "Could not read S5 data";
> + if (ptr >= end || ptr == start) {
> + goto exit;
> + }
> +
> + /* validate AML structure */
> + if (*(ptr + AMI_S5_PACKAGE_OP_OFFSET) != AMI_PACKAGE_OP_CODE) {
> + msg = "Could not read S5 data";
> + if (ptr >= end || ptr == start) {
> + goto exit;
> + }
> +
> + /* validate AML structure */
> + goto exit;
> + }
> +
> + if ((ptr < start + (-AMI_S5_NAME_OP_OFFSET_2) ||
> + }
> +
> + (*(ptr + AMI_S5_NAME_OP_OFFSET_2) != AMI_NAME_OP_CODE ||
> + *(ptr + AMI_S5_NAME_OP_OFFSET_2 + 1) != '\\'))
> + && *(ptr - AMI_S5_NAME_OP_OFFSET_1) != AMI_NAME_OP_CODE) {
lines too long
> + goto exit;
> + }
> +
> + if (ptr >= end) {
> + goto exit;
> + }
> +
> + /* package length */
> + ptr += ((*ptr & AMI_PACKAGE_LENGTH_ENCODING_BITS_MASK) >>
> + goto exit;
> + }
> +
> + /* package length */
> + AMI_PACKAGE_LENGTH_ENCODING_BITS_SHIFT) +
> + AMI_MIN_PACKAGE_LENGTH + AMI_NUM_ELEMENTS_LENGTH;
> + if (ptr >= end) {
> + goto exit;
> + }
> +
> + if (*ptr == AMI_BYTE_PREFIX_CODE) {
> + goto exit;
> + }
> +
> + ++ptr; /* skip byte prefix */
> + }
> +
> + slp_typa = (*ptr) << AMI_SLP_TYPA_SHIFT;
> + }
> +
> +
> + if (*(++ptr) == AMI_BYTE_PREFIX_CODE) {
> + ++ptr; /* skip byte prefix */
> + }
You still use ++ptr and { } around a single line. Both repeats in
> + }
many places. We can argue about ++ptr, but, please, fix the braces.
(*(++ptr)) is correct, however, I'd prefer one extra ptr++ for
the sake of easy reading.
> + slp_typb = (*ptr) << AMI_SLP_TYPB_SHIFT;
> +
> + pm1a_cnt_blk = fadt_header->pm1a_cnt_blk;
> + pm1b_cnt_blk = fadt_header->pm1b_cnt_blk;
> +
> + msg = "poweroff initialized";
> +
> +exit:
> + if (msg) {
> + printf("acpi: %s\n", msg);
> + }
> +}
> +
> void acpi_init(void)
> {
> int s, i;
> @@ -239,6 +340,8 @@ void acpi_init(void)
> + pm1a_cnt_blk = fadt_header->pm1a_cnt_blk;
> + pm1b_cnt_blk = fadt_header->pm1b_cnt_blk;
> +
> + msg = "poweroff initialized";
> +
> +exit:
> + if (msg) {
> + printf("acpi: %s\n", msg);
> + }
> +}
> +
> void acpi_init(void)
> {
> int s, i;
> sdt_trans[i].signature[ACPI_SDT_SIGNATURE_LEN] = '\0';
> sdt_trans[i].length = hdr.length;
> }
> +
> + acpi_init_poweroff();
> }
>
> struct acpi_madt_ioapic * acpi_get_ioapic_next(void)
> @@ -293,3 +396,19 @@ struct acpi_madt_lapic * acpi_get_lapic_next(void)
> sdt_trans[i].length = hdr.length;
> }
> +
> + acpi_init_poweroff();
> }
>
> struct acpi_madt_ioapic * acpi_get_ioapic_next(void)
>
> return ret;
> }
> +
> +void __k_unpaged_acpi_poweroff(void)
> +{
> + /* NO OP poweroff symbol*/
> +}
> +
> return ret;
> }
> +
> +void __k_unpaged_acpi_poweroff(void)
> +{
> + /* NO OP poweroff symbol*/
> +}
> +
> +void acpi_poweroff(void)
> +{
> + if (pm1a_cnt_blk == 0) {
> + return;
> + }
> + outw(pm1a_cnt_blk, slp_typa | SLP_EN_CODE);
> +{
> + if (pm1a_cnt_blk == 0) {
> + return;
> + }
> + if (pm1b_cnt_blk != 0) {
> + outw(pm1b_cnt_blk, slp_typb | SLP_EN_CODE);
> + }
> +}
> diff --git a/kernel/arch/i386/acpi.h b/kernel/arch/i386/acpi.h
> index ddbd504..06e978c 100644
> +}
> diff --git a/kernel/arch/i386/acpi.h b/kernel/arch/i386/acpi.h
> --- a/kernel/arch/i386/arch_reset.c
> +++ b/kernel/arch/i386/arch_reset.c
> @@ -22,6 +22,10 @@
> #include "direct_utils.h"
> #include <machine/multiboot.h>
>
> +#ifdef USE_ACPI
> +#include "acpi.h"
> +#endif
> +
> #define KBCMDP 4 /* kbd controller port (O) */
> #define KBC_PULSE0 0xfe /* pulse output bit 0 */
> #define IO_KBD 0x060 /* 8042 Keyboard */
> @@ -85,6 +89,9 @@ poweroff(void)
> {
> const char *shutdown_str;
>
> +#ifdef USE_ACPI
> + acpi_poweroff();
> +++ b/kernel/arch/i386/arch_reset.c
> @@ -22,6 +22,10 @@
> #include "direct_utils.h"
> #include <machine/multiboot.h>
>
> +#ifdef USE_ACPI
> +#include "acpi.h"
> +#endif
> +
> #define KBCMDP 4 /* kbd controller port (O) */
> #define KBC_PULSE0 0xfe /* pulse output bit 0 */
> #define IO_KBD 0x060 /* 8042 Keyboard */
> @@ -85,6 +89,9 @@ poweroff(void)
> {
> const char *shutdown_str;
>
> +#ifdef USE_ACPI
> + acpi_poweroff();
Xiaoguang Sun
May 21, 2013, 12:04:17 PM5/21/13
to min...@googlegroups.com
I finally know why you say lines too long and I don't see them. That's because I use 2 spaces for tab. So if it's possible, can you share your .vimrc file with me so I can take a look at it? The ptr things have been changed as well. BTW: arch_reset.c file has mixed setting which expand tab to spaces, I choose tab to make it the same as acpi.c
The thread patch will be very hard to break to many small ones. Almost every single file of kernel need to be changed to add thread support. e.g. syscall functions have to take thread now. So I think it's pretty hard to make it work that way.
On Tuesday, May 21, 2013 2:58:22 PM UTC+8, Xiaoguang Sun wrote:
The thread patch will be very hard to break to many small ones. Almost every single file of kernel need to be changed to add thread support. e.g. syscall functions have to take thread now. So I think it's pretty hard to make it work that way.
On Tuesday, May 21, 2013 2:58:22 PM UTC+8, Xiaoguang Sun wrote:
Tomas Hruby
May 21, 2013, 12:15:27 PM5/21/13
to min...@googlegroups.com
On Tue, May 21, 2013 at 09:04:17AM -0700, Xiaoguang Sun wrote:
> I finally know why you say lines too long and I don't see them. That's
> because I use 2 spaces for tab.
Right, I see :-) 8 is the standard.
> I finally know why you say lines too long and I don't see them. That's
> because I use 2 spaces for tab.
Thanks for applying the changes.
> So if it's possible, can you share your
> .vimrc file with me so I can take a look at it?
set ts=8
> The thread patch will be very hard to break to many small ones. Almost
> every single file of kernel need to be changed to add thread support. e.g.
> syscall functions have to take thread now. So I think it's pretty hard to
> make it work that way.
though. Such is life ...
Xiaoguang Sun
May 21, 2013, 12:35:20 PM5/21/13
to min...@googlegroups.com
Thanks, I will change my .vimrc to use this setup for Minix3 code.
Speaking of thread, I guess I should give it up now. But I do want to contribute some thing to Minix3 just for fun. Is there some small thing that could be easier to accept by core team for people to work on?
Another thing not related to Minix3 I don't understand, every time I hit POST REPLY button, the quoted message is my original first message. But your reply can always quote the previous message. I don't know if I used groups wrong as I never used it before or it's just stupid Chinese Great Firewall is blocking some link so the web page looks different and missing some buttons. I have this kind of problem with LinkedIn most of time.
Xiaoguang
On Tuesday, May 21, 2013 2:58:22 PM UTC+8, Xiaoguang Sun wrote:
Speaking of thread, I guess I should give it up now. But I do want to contribute some thing to Minix3 just for fun. Is there some small thing that could be easier to accept by core team for people to work on?
Another thing not related to Minix3 I don't understand, every time I hit POST REPLY button, the quoted message is my original first message. But your reply can always quote the previous message. I don't know if I used groups wrong as I never used it before or it's just stupid Chinese Great Firewall is blocking some link so the web page looks different and missing some buttons. I have this kind of problem with LinkedIn most of time.
Xiaoguang
On Tuesday, May 21, 2013 2:58:22 PM UTC+8, Xiaoguang Sun wrote:
PrasannaKumar Muralidharan
May 21, 2013, 1:36:05 PM5/21/13
to min...@googlegroups.com
> Speaking of thread, I guess I should give it up now. But I do want to
> contribute some thing to Minix3 just for fun. Is there some small thing that
> could be easier to accept by core team for people to work on?
It will be great for users if you continue to work on threads and try
> contribute some thing to Minix3 just for fun. Is there some small thing that
> could be easier to accept by core team for people to work on?
to upstream it. If I am not up to date please correct me.
>
> Another thing not related to Minix3 I don't understand, every time I hit
> POST REPLY button, the quoted message is my original first message. But your
> reply can always quote the previous message. I don't know if I used groups
> wrong as I never used it before or it's just stupid Chinese Great Firewall
> is blocking some link so the web page looks different and missing some
> buttons. I have this kind of problem with LinkedIn most of time.
>
Regards,
PrasannaKumar
Xiaoguang Sun
May 21, 2013, 7:45:10 PM5/21/13
to min...@googlegroups.com
Nop, I'm using Safari on OS X. Never mind it's not that important.
On Tuesday, May 21, 2013 2:58:22 PM UTC+8, Xiaoguang Sun wrote:
On Tuesday, May 21, 2013 2:58:22 PM UTC+8, Xiaoguang Sun wrote:
Ben Gras
May 22, 2013, 5:26:17 AM5/22/13
to min...@googlegroups.com
All,
Thanks for the contribution!
I've tested this change on my minix hw box and it doesn't actually seem to work; "acpi: poweroff initialized" is displayed but the machine doesn't power off on 'shutdown -p.' The osdev webpage you linked said qemu should work.
My debug output says on init:
pm1a_cnt_blk 0xf804, pm1b_cnt_blk 0x860, SLP_EN_CODE 0x2000
On poweroff:
outw 0xf804, 0x2000
outw 0x860, 0x3c00
so that looks okay but it doesn't actually work.
It doesn't seem to work on trunk kvm either, which would be a reasonable improvement in itself as the poweroff hack doesn't work there anymore. The osdev page reports success.
Thanks for the contribution!
I've tested this change on my minix hw box and it doesn't actually seem to work; "acpi: poweroff initialized" is displayed but the machine doesn't power off on 'shutdown -p.' The osdev webpage you linked said qemu should work.
My debug output says on init:
pm1a_cnt_blk 0xf804, pm1b_cnt_blk 0x860, SLP_EN_CODE 0x2000
On poweroff:
outw 0xf804, 0x2000
outw 0x860, 0x3c00
so that looks okay but it doesn't actually work.
It doesn't seem to work on trunk kvm either, which would be a reasonable improvement in itself as the poweroff hack doesn't work there anymore. The osdev page reports success.
Xiaoguang Sun
May 22, 2013, 5:38:36 AM5/22/13
to min...@googlegroups.com
I probably introduced bug when I fix coding style. It doesn't work on my vmware virtual machine now whereas the original patch works. Let me try to look at it when I have time.
On Tuesday, May 21, 2013 2:58:22 PM UTC+8, Xiaoguang Sun wrote:
On Tuesday, May 21, 2013 2:58:22 PM UTC+8, Xiaoguang Sun wrote:
Ben Gras
May 22, 2013, 5:39:54 AM5/22/13
to min...@googlegroups.com
Nice, thank you!
Xiaoguang Sun
May 22, 2013, 9:56:06 AM5/22/13
to min...@googlegroups.com
I found a bug when I change magic codes to defines and it works under VMware fusion now. But I guess the problem you see is different. let me try to verify it on qemu later and see why it doesn't work. Will update this thread later.
On Tuesday, May 21, 2013 2:58:22 PM UTC+8, Xiaoguang Sun wrote:
On Tuesday, May 21, 2013 2:58:22 PM UTC+8, Xiaoguang Sun wrote:
Xiaoguang Sun
May 22, 2013, 12:12:47 PM5/22/13
to min...@googlegroups.com
Hi Ben,
I checked qemu source code, looks like they toke some code from bochs and used it in early version. And later on, for some reason, they removed the bochs_bios_write from hw/pc.c since version 1.4.0. So the old Bochs/QEmu trick doesn't work since then. And the BIOS used by current QEmu doesn't have _S5 object which is the reason why my patch doesn't work. Strangely, QEmu does have hard coded support for ACPI poweroff in hw/acpi/core.c, here is what they do.
494 /* ACPI PM1aCNT */
495 static void acpi_pm1_cnt_write(ACPIREGS *ar, uint16_t val)
496 {
497 ar->pm1.cnt.cnt = val & ~(ACPI_BITMASK_SLEEP_ENABLE);
498
499 if (val & ACPI_BITMASK_SLEEP_ENABLE) {
500 /* change suspend type */
501 uint16_t sus_typ = (val >> 10) & 7;
502 switch(sus_typ) {
503 case 0: /* soft power off */
504 qemu_system_shutdown_request();
505 break;
506 case 1:
507 qemu_system_suspend_request();
508 break;
509 default:
510 if (sus_typ == ar->pm1.cnt.s4_val) { /* S4 request */
511 monitor_protocol_event(QEVENT_SUSPEND_DISK, NULL);
512 qemu_system_shutdown_request();
513 }
514 break;
515 }
516 }
517 }
Bochs since 2.4 on the other hand has a newer bios with _S5 object defined yet still support the old trick. When I tried minix3 on Bochs, it's ethernet card didn't work, So I didn't bother to type my patch manually and verify it. But I guess it would work if I manually type code inside virtual machine and compile it. Even if it doesn't do so, the old way still works.
I have no access to physical machine other then my mac book pro which uses elf to boot. So I can't try it on real hardware but looks like the problem you have seen is different, you actually found _S5 object, but probably the SLP_TYPa and SLP_TYPb code isn't read correctly for some reason. Maybe they way I got from osdev doesn't cover your DSDT format.
Here is my suggestion, from QEmu's code and Bochs' acpi-dsdt.dsl and also osdev's comment, SLP_TYPa and SLP_TYPb most likely should be always zero. I have changed the patch to look for _S5 object and if not possible to find it, use 0 for both. This way doesn't cover the issue you have with your real hardware. So another way is always use 0 and don't even bother looking for _S5 object. This doesn't sound nice. So if really want to fix this, you can try to dump the content of the whole DSDT table in hex format and send it to me. I will try to fix it in another patch.
Xiaoguang
I checked qemu source code, looks like they toke some code from bochs and used it in early version. And later on, for some reason, they removed the bochs_bios_write from hw/pc.c since version 1.4.0. So the old Bochs/QEmu trick doesn't work since then. And the BIOS used by current QEmu doesn't have _S5 object which is the reason why my patch doesn't work. Strangely, QEmu does have hard coded support for ACPI poweroff in hw/acpi/core.c, here is what they do.
494 /* ACPI PM1aCNT */
495 static void acpi_pm1_cnt_write(ACPIREGS *ar, uint16_t val)
496 {
497 ar->pm1.cnt.cnt = val & ~(ACPI_BITMASK_SLEEP_ENABLE);
498
499 if (val & ACPI_BITMASK_SLEEP_ENABLE) {
500 /* change suspend type */
501 uint16_t sus_typ = (val >> 10) & 7;
502 switch(sus_typ) {
503 case 0: /* soft power off */
504 qemu_system_shutdown_request();
505 break;
506 case 1:
507 qemu_system_suspend_request();
508 break;
509 default:
510 if (sus_typ == ar->pm1.cnt.s4_val) { /* S4 request */
511 monitor_protocol_event(QEVENT_SUSPEND_DISK, NULL);
512 qemu_system_shutdown_request();
513 }
514 break;
515 }
516 }
517 }
Bochs since 2.4 on the other hand has a newer bios with _S5 object defined yet still support the old trick. When I tried minix3 on Bochs, it's ethernet card didn't work, So I didn't bother to type my patch manually and verify it. But I guess it would work if I manually type code inside virtual machine and compile it. Even if it doesn't do so, the old way still works.
I have no access to physical machine other then my mac book pro which uses elf to boot. So I can't try it on real hardware but looks like the problem you have seen is different, you actually found _S5 object, but probably the SLP_TYPa and SLP_TYPb code isn't read correctly for some reason. Maybe they way I got from osdev doesn't cover your DSDT format.
Here is my suggestion, from QEmu's code and Bochs' acpi-dsdt.dsl and also osdev's comment, SLP_TYPa and SLP_TYPb most likely should be always zero. I have changed the patch to look for _S5 object and if not possible to find it, use 0 for both. This way doesn't cover the issue you have with your real hardware. So another way is always use 0 and don't even bother looking for _S5 object. This doesn't sound nice. So if really want to fix this, you can try to dump the content of the whole DSDT table in hex format and send it to me. I will try to fix it in another patch.
Xiaoguang
Xiaoguang Sun
May 22, 2013, 10:08:18 PM5/22/13
to min...@googlegroups.com
Hi Ben, Can you use method described here to get a dump of dsdt or full acpi table for me to examine the problem. Thanks.
http://smackerelofopinion.blogspot.com/2009/10/dumping-acpi-tables-using-acpidump-and.html
On Tuesday, May 21, 2013 2:58:22 PM UTC+8, Xiaoguang Sun wrote:
http://smackerelofopinion.blogspot.com/2009/10/dumping-acpi-tables-using-acpidump-and.html
On Tuesday, May 21, 2013 2:58:22 PM UTC+8, Xiaoguang Sun wrote:
Ben Gras
May 23, 2013, 6:17:01 AM5/23/13
to min...@googlegroups.com
I'd be happy to do that but it'll take a little time to boot my minix box into linux and get the results somewhere.
Does your 2nd patch work in kvm trunk? That would be progress already.
Do you have a public git repo I could get your change from? While applying the attached patchfile last time I got some messy ^M line endings.
Does your 2nd patch work in kvm trunk? That would be progress already.
Do you have a public git repo I could get your change from? While applying the attached patchfile last time I got some messy ^M line endings.
Xiaoguang Sun
May 23, 2013, 7:08:48 AM5/23/13
to min...@googlegroups.com
Yea, it works with QEmu trunk which I have tested and should work with earlier version as well. I haven't tried Bochs, but theoretically it should work. It also works with my VMware fusion 5 which I use to develop and test. You can try to pull from branch acpi_power of https://github.com/sunxiaoguang/minix.git If you have problem please let me know.
Xiaoguang Sun
May 24, 2013, 11:18:29 AM5/24/13
to min...@googlegroups.com
Any update to this patch? And I think you can use live cd, most distribution has it, to boot and get full acpi table dump.
Alex B.
May 26, 2013, 5:40:26 AM5/26/13
to min...@googlegroups.com
That's a great patch, however, that specific patch doesn't work with all chip sets nor all version of ACPI but it is a great start. Thank you for your contribution though!
Xiaoguang Sun
May 26, 2013, 7:42:11 AM5/26/13
to min...@googlegroups.com
It's probably a lot of work to make it works well with most hardware and the problem is that I have no access to other physical machines at this time. In addition I found acpica used in acpi driver crashes under qemu 1.5, so it probably has problem with other hardware also. Consider acpi is not enabled by default, maybe implementing a mini implementation of AML interpreter in kernel to just make it possible to poweroff is a better choice. And leave most of other acpi functions outside of kernel with acpi driver.
Alex B.
May 26, 2013, 4:25:02 PM5/26/13
to min...@googlegroups.com
At this time, Minix 3 doesn't even boot on my computers, I have to use a VM as well, I use Oracle's VM VirtualBox, https://www.virtualbox.org/ .
I see what you are saying, hopefully the maintainer of the tree will notice you message, as I am sending a message upstream to see if they will insert it. I can "put" stuff in GIT but it's not all the time they accept patches to the core (which is unfortunate).
On Sun, May 26, 2013 at 6:42 AM, Xiaoguang Sun <sunxia...@gmail.com> wrote:
It's probably a lot of work to make it works well with most hardware and the problem is that I have no access to other physical machines at this time. In addition I found acpica used in acpi driver crashes under qemu 1.5, so it probably has problem with other hardware also. Consider acpi is not enabled by default, maybe implementing a mini implementation of AML interpreter in kernel to just make it possible to poweroff is a better choice. And leave most of other acpi functions outside of kernel with acpi driver.
That's a great patch, however, that specific patch doesn't work with all chip sets nor all version of ACPI but it is a great start. Thank you for your contribution though!
--You received this message because you are subscribed to a topic in the Google Groups "minix3" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/minix3/yUVLnN99tZA/unsubscribe?hl=en-US.
To unsubscribe from this group and all its topics, send an email to minix3+un...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
Ben Gras
May 29, 2013, 12:19:45 PM5/29/13
to min...@googlegroups.com
I tested that your change works under kvm trunk, and it does, so we can actually poweroff kvm again now, which is nice. Your change is merged with master, thank you very much for your contribution!
keesj
Jun 10, 2013, 4:33:39 AM6/10/13
to min...@googlegroups.com
HI Alex,
On Sunday, May 26, 2013 10:25:02 PM UTC+2, Alex B. wrote:
On Sunday, May 26, 2013 10:25:02 PM UTC+2, Alex B. wrote:
At this time, Minix 3 doesn't even boot on my computers, I have to use a VM as well, I use Oracle's VM VirtualBox, https://www.virtualbox.org/ .I see what you are saying, hopefully the maintainer of the tree will notice you message, as I am sending a message upstream to see if they will insert it. I can "put" stuff in GIT but it's not all the time they accept patches to the core (which is unfortunate).
I just stumbled upon this message. Can you please clarify what you want to upstream and what needs to be accepted? Are there pending patches around?
Greetings
Xiaoguang Sun
Jun 10, 2013, 8:01:14 AM6/10/13
to min...@googlegroups.com
Ben already helped me to commit my patch. Thanks for checking with this. I'm currently testing another patch to add getrusage. Will post patch in this group later and hopefully it can be accepted as well.
Alex B.
Jun 14, 2013, 4:28:21 PM6/14/13
to min...@googlegroups.com
keesj,
It wasn't my patch, it was Xiaoguang Sun's patch and it's already been approved for upstream main and it's been submitted already. Wow, I think Google need to make the groups a bit easier to read so that we're not tripping over anything or anyone's request to submit upstream. Google Groups and Communities really do need, "sticky topics", that would solve a lot of problem.
Reply all
Reply to author
Forward
0 new messages