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, 0x0000b010, 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, 0x0000b010, 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
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, 0x0000b010, 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, 0x0000b010, 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
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, 0x0000b010, 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: <id> <length> <fixed params> <variable param list>. The <length> 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
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,
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
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?
-
Isaku Yamahata -
Kevin O'Connor -
Michael S. Tsirkin