Re: [SeaBIOS] [PATCHv3 4/4] acpi: automatically generated ssdt proc
On Tue, Oct 18, 2011 at 02:47:29AM +0900, Isaku Yamahata wrote: > On Wed, Oct 05, 2011 at 10:15:26PM -0400, Kevin O'Connor wrote: > > - Some time back there were patches floating around to pass the DSDT > > into SeaBIOS via fw_cfg interface. Those patches never made it in > > (I forget why), but the basic functionality seemed sound. Patching > > the DSDT in SeaBIOS would seem to eliminate that possibility. > > Maybe a bit late comment. > At that time, the patch for qemu-side was NOT merged. > Right now it is merged into the qemu upstream as > 104bf02eb50e080ac9d0de5905f80f9a09730154 > (It took very long time to draw attention from the maintainers. sigh.) > > Keven, if you like, I'm willing to rebase/resend it. Sure - if the qemu part is upstream, send the patch again. -Kevin -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 4/4] acpi: automatically generated ssdt proc
On Wed, Oct 05, 2011 at 10:15:26PM -0400, Kevin O'Connor wrote: > Sure: > > - The DSDT is big and has several cross-functional users. Patching up > the DSDT for hotplug when the DSDT also has unrelated stuff (eg, > mouse) seems ugly. > > - The PCI hotplug stuff is generating a whole bunch of devices and the > dynamic code is effectively disabling the unwanted ones. It seems > nicer to dynamically generate the desired entries instead of bulk > generating and dynamically blanking. > > - The CPU hotplug has similar requirements, but is implemented > differently - it generates the CPU objects dynamically. It's not > desirable to bulk generate the CPU objects and "blank" them > dynamically, because 255 CPU objects would noticeably increase > SeaBIOS' static size. > > - Some time back there were patches floating around to pass the DSDT > into SeaBIOS via fw_cfg interface. Those patches never made it in > (I forget why), but the basic functionality seemed sound. Patching > the DSDT in SeaBIOS would seem to eliminate that possibility. > > None of these would be road-blocks. However, they make me want to > consider other approaches. So if we had the hotplug stuff in a separate ssdt, and patched that in the same way my patches do, this seems to address 3 comments otu of 4 (all except the second one). We'll want to do something else for a bridge, but for now this seems a sane compromise? -- MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [SeaBIOS] [PATCHv3 4/4] acpi: automatically generated ssdt proc
On Wed, Oct 05, 2011 at 10:15:26PM -0400, Kevin O'Connor wrote: > - Some time back there were patches floating around to pass the DSDT > into SeaBIOS via fw_cfg interface. Those patches never made it in > (I forget why), but the basic functionality seemed sound. Patching > the DSDT in SeaBIOS would seem to eliminate that possibility. Maybe a bit late comment. At that time, the patch for qemu-side was NOT merged. Right now it is merged into the qemu upstream as 104bf02eb50e080ac9d0de5905f80f9a09730154 (It took very long time to draw attention from the maintainers. sigh.) Keven, if you like, I'm willing to rebase/resend it. thanks, -- yamahata -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 4/4] acpi: automatically generated ssdt proc
On Tue, Oct 04, 2011 at 03:26:19PM +0200, Michael S. Tsirkin wrote: > Get rid of manually cut and pasted ssdt_proc, > use ssdt compiled by iasl and offsets extracted > by acpi_extract instead. > > Signed-off-by: Michael S. Tsirkin FYI - I pushed the "ACPI DSDT simplifications" series and patch 1 and 4 from this series. -Kevin -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 4/4] acpi: automatically generated ssdt proc
On Wed, Oct 05, 2011 at 08:35:26AM -0200, Michael S. Tsirkin wrote:
> On Tue, Oct 04, 2011 at 10:52:33PM -0400, Kevin O'Connor wrote:
> > Something like:
> >
> > ACPI_EXTRACT ssdt_proc_obj
> > Processor (CPAA, 0xAA, 0xb010, 0x06) {
> >
>
> I considered this, sure. We could parse AML to figure out
> what is the object type we are trying to look up.
>
> However I decided sanity-checking that we get the right type of object
> in AML is better. This way if iasl output format breaks
> we will have a better chance to detect that.
> Makes sense?
Yes. I guess one could do ACPI_EXTRACT_PROCESSOR for the sanity
check.
> Also this can not be as generic as it seems: each type of
> object in AML bytecode is encoded slightly differently.
> So we would still have types of objects we support
> and types of object we don't.
Yes.
> > which would produce something like:
> >
> > static struct aml_object ssdt_proc_obj = {.addr=0x24, .size=0x40,
> > .param=0x28};
>
> What is the param offset here?
The location of the first byte of the parameters (the same as you had
for ssdt_proc_name). Normally, AML objects take the form:
. The is itself
of variable length, so passing in the start of the fixed parameters
would make manipulating the results easier.
> > As for the other parts of this patch series - I'm still leary of
> > changing the DSDT dynamically.
>
> Hmm, not sure I understand why. Could you explain pls?
Sure:
- The DSDT is big and has several cross-functional users. Patching up
the DSDT for hotplug when the DSDT also has unrelated stuff (eg,
mouse) seems ugly.
- The PCI hotplug stuff is generating a whole bunch of devices and the
dynamic code is effectively disabling the unwanted ones. It seems
nicer to dynamically generate the desired entries instead of bulk
generating and dynamically blanking.
- The CPU hotplug has similar requirements, but is implemented
differently - it generates the CPU objects dynamically. It's not
desirable to bulk generate the CPU objects and "blank" them
dynamically, because 255 CPU objects would noticeably increase
SeaBIOS' static size.
- Some time back there were patches floating around to pass the DSDT
into SeaBIOS via fw_cfg interface. Those patches never made it in
(I forget why), but the basic functionality seemed sound. Patching
the DSDT in SeaBIOS would seem to eliminate that possibility.
None of these would be road-blocks. However, they make me want to
consider other approaches.
> > and then just memcpy the "hotplug_obj" N number of times into the ssdt
> > for each available slot. (This would be on top of the DSDT
> > simplification patch series that I posted previously.)
>
> This assumes all devices are the same. But unfortunately this will not
> work for other devices such as VGA.
The VGA device can't be hotplugged, so I don't see why that would be
an issue.
-Kevin
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 4/4] acpi: automatically generated ssdt proc
On Tue, Oct 04, 2011 at 10:52:33PM -0400, Kevin O'Connor wrote:
> On Tue, Oct 04, 2011 at 03:26:19PM +0200, Michael S. Tsirkin wrote:
> > Get rid of manually cut and pasted ssdt_proc,
> > use ssdt compiled by iasl and offsets extracted
> > by acpi_extract instead.
>
> Thanks - I like the idea of auto-generating the offsets.
>
> [...]
> > +#define AmlCode static ssdp_proc_aml
> > +#include "ssdt-proc.hex"
> > +#undef AmlCode
>
> Side note - since you're post-processing the acpi data, it would be
> nice to update the name in the hex file too.
That would mean we will output hex as well, ignore the
hex produced by iasl. Sure, I can do that.
Another benefit would be that we can skip the ssdt
preamble generated by iasl in the hex we output.
> > +/* 0x5B 0x83 ProcessorOp PkgLength NameString ProcID */
> > +#define SD_OFFSET_CPUHEX (*ssdt_proc_name - *ssdt_proc_start + 2)
> > +#define SD_OFFSET_CPUID1 (*ssdt_proc_name - *ssdt_proc_start + 4)
> > +#define SD_OFFSET_CPUID2 (*ssdt_proc_id - *ssdt_proc_start)
> > +#define SD_SIZEOF (*ssdt_proc_end - *ssdt_proc_start)
> > +#define SD_PROC (ssdp_proc_aml + *ssdt_proc_start)
> [...]
> > DefinitionBlock ("ssdt-proc.aml", "SSDT", 0x01, "BXPC", "BXSSDT", 0x1)
> > -/* v-- DO NOT EDIT --v */
> > {
> > +ACPI_EXTRACT_PROCESSOR_START ssdt_proc_start
> > +ACPI_EXTRACT_PROCESSOR_END ssdt_proc_end
> > +ACPI_EXTRACT_PROCESSOR_STRING ssdt_proc_name
> > Processor (CPAA, 0xAA, 0xb010, 0x06) {
>
> Since the acpi.c code needs to know the processor object format
> anyway, what about making a generic "ACPI_EXTRACT" indicator that
> exports the location, size, and parameter location in one go.
> Something like:
>
> ACPI_EXTRACT ssdt_proc_obj
> Processor (CPAA, 0xAA, 0xb010, 0x06) {
>
I considered this, sure. We could parse AML to figure out
what is the object type we are trying to look up.
However I decided sanity-checking that we get the right type of object
in AML is better. This way if iasl output format breaks
we will have a better chance to detect that.
Makes sense?
Also this can not be as generic as it seems: each type of
object in AML bytecode is encoded slightly differently.
So we would still have types of objects we support
and types of object we don't.
> which would produce something like:
>
> static struct aml_object ssdt_proc_obj = {.addr=0x24, .size=0x40,
> .param=0x28};
What is the param offset here?
I really think multiple arrays are better so we don't waste memory
on fields that we don't need and alignment.
I also dislike hardcoding field names. With a struct,
if you want to know where did 'param' come from you must
read python. My way, all names come from DSL source.
> As for the other parts of this patch series - I'm still leary of
> changing the DSDT dynamically.
Hmm, not sure I understand why. Could you explain pls?
> I'd be curious to see if we can add
> the following to ssdt-proc.dsl:
>
> ACPI_EXTRACT hotplug_obj
> Device (SL00) {
> ACPI_EXTRACT_NAME_DWORD_CONST hotplog_id
> Name (ID, 0xAABBCCDD)
> Name (_ADR, ID)
> Method (_EJ0, 1) { Return(PCEJ(ID)) }
> Name (_SUN, ID)
> }
>
> and then just memcpy the "hotplug_obj" N number of times into the ssdt
> for each available slot. (This would be on top of the DSDT
> simplification patch series that I posted previously.)
This assumes all devices are the same. But unfortunately this will not
work for other devices such as VGA.
>
> -Kevin
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 4/4] acpi: automatically generated ssdt proc
On Tue, Oct 04, 2011 at 03:26:19PM +0200, Michael S. Tsirkin wrote:
> Get rid of manually cut and pasted ssdt_proc,
> use ssdt compiled by iasl and offsets extracted
> by acpi_extract instead.
Thanks - I like the idea of auto-generating the offsets.
[...]
> +#define AmlCode static ssdp_proc_aml
> +#include "ssdt-proc.hex"
> +#undef AmlCode
Side note - since you're post-processing the acpi data, it would be
nice to update the name in the hex file too.
> +/* 0x5B 0x83 ProcessorOp PkgLength NameString ProcID */
> +#define SD_OFFSET_CPUHEX (*ssdt_proc_name - *ssdt_proc_start + 2)
> +#define SD_OFFSET_CPUID1 (*ssdt_proc_name - *ssdt_proc_start + 4)
> +#define SD_OFFSET_CPUID2 (*ssdt_proc_id - *ssdt_proc_start)
> +#define SD_SIZEOF (*ssdt_proc_end - *ssdt_proc_start)
> +#define SD_PROC (ssdp_proc_aml + *ssdt_proc_start)
[...]
> DefinitionBlock ("ssdt-proc.aml", "SSDT", 0x01, "BXPC", "BXSSDT", 0x1)
> -/* v-- DO NOT EDIT --v */
> {
> +ACPI_EXTRACT_PROCESSOR_START ssdt_proc_start
> +ACPI_EXTRACT_PROCESSOR_END ssdt_proc_end
> +ACPI_EXTRACT_PROCESSOR_STRING ssdt_proc_name
> Processor (CPAA, 0xAA, 0xb010, 0x06) {
Since the acpi.c code needs to know the processor object format
anyway, what about making a generic "ACPI_EXTRACT" indicator that
exports the location, size, and parameter location in one go.
Something like:
ACPI_EXTRACT ssdt_proc_obj
Processor (CPAA, 0xAA, 0xb010, 0x06) {
which would produce something like:
static struct aml_object ssdt_proc_obj = {.addr=0x24, .size=0x40, .param=0x28};
As for the other parts of this patch series - I'm still leary of
changing the DSDT dynamically. I'd be curious to see if we can add
the following to ssdt-proc.dsl:
ACPI_EXTRACT hotplug_obj
Device (SL00) {
ACPI_EXTRACT_NAME_DWORD_CONST hotplog_id
Name (ID, 0xAABBCCDD)
Name (_ADR, ID)
Method (_EJ0, 1) { Return(PCEJ(ID)) }
Name (_SUN, ID)
}
and then just memcpy the "hotplug_obj" N number of times into the ssdt
for each available slot. (This would be on top of the DSDT
simplification patch series that I posted previously.)
-Kevin
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv3 4/4] acpi: automatically generated ssdt proc
Get rid of manually cut and pasted ssdt_proc,
use ssdt compiled by iasl and offsets extracted
by acpi_extract instead.
Signed-off-by: Michael S. Tsirkin
---
Makefile |2 +-
src/acpi.c| 33 +
src/ssdt-proc.dsl | 19 +++
3 files changed, 21 insertions(+), 33 deletions(-)
diff --git a/Makefile b/Makefile
index 541b080..fee68aa 100644
--- a/Makefile
+++ b/Makefile
@@ -200,7 +200,7 @@ src/%.hex: src/%.dsl ./tools/acpi_extract_preprocess.py
./tools/acpi_extract.py
$(Q)./tools/acpi_extract.py $(OUT)$*.lst > $(OUT)$*.off
$(Q)cat $(OUT)$*.hex $(OUT)$*.off > $@
-$(OUT)ccode32flat.o: src/acpi-dsdt.hex
+$(OUT)ccode32flat.o: src/acpi-dsdt.hex src/ssdt-proc.hex
### Kconfig rules
export HOSTCC := $(CC)
diff --git a/src/acpi.c b/src/acpi.c
index f65f974..cf60257 100644
--- a/src/acpi.c
+++ b/src/acpi.c
@@ -398,23 +398,16 @@ encodeLen(u8 *ssdt_ptr, int length, int bytes)
return ssdt_ptr + bytes;
}
-// AML Processor() object. See src/ssdt-proc.dsl for info.
-static unsigned char ssdt_proc[] = {
-0x5b,0x83,0x42,0x05,0x43,0x50,0x41,0x41,
-0xaa,0x10,0xb0,0x00,0x00,0x06,0x08,0x49,
-0x44,0x5f,0x5f,0x0a,0xaa,0x08,0x5f,0x48,
-0x49,0x44,0x0d,0x41,0x43,0x50,0x49,0x30,
-0x30,0x30,0x37,0x00,0x14,0x0f,0x5f,0x4d,
-0x41,0x54,0x00,0xa4,0x43,0x50,0x4d,0x41,
-0x49,0x44,0x5f,0x5f,0x14,0x0f,0x5f,0x53,
-0x54,0x41,0x00,0xa4,0x43,0x50,0x53,0x54,
-0x49,0x44,0x5f,0x5f,0x14,0x0f,0x5f,0x45,
-0x4a,0x30,0x01,0x43,0x50,0x45,0x4a,0x49,
-0x44,0x5f,0x5f,0x68
-};
-#define SD_OFFSET_CPUHEX 6
-#define SD_OFFSET_CPUID1 8
-#define SD_OFFSET_CPUID2 20
+#define AmlCode static ssdp_proc_aml
+#include "ssdt-proc.hex"
+#undef AmlCode
+
+/* 0x5B 0x83 ProcessorOp PkgLength NameString ProcID */
+#define SD_OFFSET_CPUHEX (*ssdt_proc_name - *ssdt_proc_start + 2)
+#define SD_OFFSET_CPUID1 (*ssdt_proc_name - *ssdt_proc_start + 4)
+#define SD_OFFSET_CPUID2 (*ssdt_proc_id - *ssdt_proc_start)
+#define SD_SIZEOF (*ssdt_proc_end - *ssdt_proc_start)
+#define SD_PROC (ssdp_proc_aml + *ssdt_proc_start)
#define SSDT_SIGNATURE 0x54445353 // SSDT
static void*
@@ -423,7 +416,7 @@ build_ssdt(void)
int acpi_cpus = MaxCountCPUs > 0xff ? 0xff : MaxCountCPUs;
// length = ScopeOp + procs + NTYF method + CPON package
int length = ((1+3+4)
- + (acpi_cpus * sizeof(ssdt_proc))
+ + (acpi_cpus * SD_SIZEOF)
+ (1+2+5+(12*acpi_cpus))
+ (6+2+1+(1*acpi_cpus)));
u8 *ssdt = malloc_high(sizeof(struct acpi_table_header) + length);
@@ -444,12 +437,12 @@ build_ssdt(void)
// build Processor object for each processor
int i;
for (i=0; i> 4);
ssdt_ptr[SD_OFFSET_CPUHEX+1] = getHex(i);
ssdt_ptr[SD_OFFSET_CPUID1] = i;
ssdt_ptr[SD_OFFSET_CPUID2] = i;
-ssdt_ptr += sizeof(ssdt_proc);
+ssdt_ptr += SD_SIZEOF;
}
// build "Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, Arg1)}
...}"
diff --git a/src/ssdt-proc.dsl b/src/ssdt-proc.dsl
index 358afa8..a461636 100644
--- a/src/ssdt-proc.dsl
+++ b/src/ssdt-proc.dsl
@@ -1,16 +1,9 @@
-/* This file is the basis for the ssdt_proc[] variable in src/acpi.c.
+/* This file is the basis for the ssdt table generated in src/acpi.c.
* It defines the contents of the per-cpu Processor() object. At
* runtime, a dynamically generated SSDT will contain one copy of this
* AML snippet for every possible cpu in the system. The objects will
* be placed in the \_SB_ namespace.
*
- * To generate a new ssdt_proc[], run the commands:
- * cpp -P src/ssdt-proc.dsl > out/ssdt-proc.dsl.i
- * iasl -ta -p out/ssdt-proc out/ssdt-proc.dsl.i
- * tail -c +37 < out/ssdt-proc.aml | hexdump -e '"" 8/1 "0x%02x," "\n"'
- * and then cut-and-paste the output into the src/acpi.c ssdt_proc[]
- * array.
- *
* In addition to the aml code generated from this file, the
* src/acpi.c file creates a NTFY method with an entry for each cpu:
* Method(NTFY, 2) {
@@ -22,13 +15,15 @@
* Name(CPON, Package() { One, One, ..., Zero, Zero, ... })
*/
DefinitionBlock ("ssdt-proc.aml", "SSDT", 0x01, "BXPC", "BXSSDT", 0x1)
-/* v-- DO NOT EDIT --v */
{
+ACPI_EXTRACT_PROCESSOR_START ssdt_proc_start
+ACPI_EXTRACT_PROCESSOR_END ssdt_proc_end
+ACPI_EXTRACT_PROCESSOR_STRING ssdt_proc_name
Processor (CPAA, 0xAA, 0xb010, 0x06) {
+ACPI_EXTRACT_NAME_BYTE_CONST ssdt_proc_id
Name (ID, 0xAA)
-/* ^-- DO NOT EDIT --^
- *
- * The src/acpi.c code requires the above layout so that it can update
+/*
+ * The src/acpi.c code requires the above ACP_EXTRACT tags so that it can
update
* CPAA and 0xAA with the appropriate CPU id (see
* SD_OFFSET_CPUHEX/CPUID1/CPUID2). Don't change the above without
* also updating the C code.
--
1.7.5.53.gc233e
--
To unsubscribe from this list
