From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35069) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aANxO-0007FB-H2 for qemu-devel@nongnu.org; Sat, 19 Dec 2015 15:18:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aANxK-0007t6-AY for qemu-devel@nongnu.org; Sat, 19 Dec 2015 15:18:22 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58202) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aANxK-0007sy-2p for qemu-devel@nongnu.org; Sat, 19 Dec 2015 15:18:18 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 86F3FC0B7E13 for ; Sat, 19 Dec 2015 20:18:17 +0000 (UTC) Date: Sat, 19 Dec 2015 22:18:14 +0200 From: "Michael S. Tsirkin" Message-ID: <20151219221129-mutt-send-email-mst@redhat.com> References: <1449704528-289297-1-git-send-email-imammedo@redhat.com> <1449704528-289297-36-git-send-email-imammedo@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1449704528-289297-36-git-send-email-imammedo@redhat.com> Subject: Re: [Qemu-devel] [PATCH 35/74] pc: acpi: factor out memhp code from build_ssdt() into separate function List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: qemu-devel@nongnu.org On Thu, Dec 10, 2015 at 12:41:29AM +0100, Igor Mammedov wrote: > before consolidating memhp code in memory_hotplug_acpi_table.c > and for simplifying review, first factor out memhp code into > new function build_memory_devices() in i386/acpi-build.c > > Signed-off-by: Igor Mammedov > ---- > PS: > no functional change, only code movement. > --- > hw/i386/acpi-build.c | 239 +++++++++++++++++++++++++++------------------------ > 1 file changed, 126 insertions(+), 113 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 1b609e6..765fccd 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -903,6 +903,130 @@ static Aml *build_crs(PCIHostState *host, > return crs; > } > > +static void build_memory_devices(Aml *sb_scope, int nr_mem, > + uint16_t io_base, uint16_t io_len) > +{ > + int i; > + Aml *scope; > + Aml *crs; > + Aml *field; > + Aml *dev; > + Aml *method; > + Aml *ifctx; > + > + /* build memory devices */ > + assert(nr_mem <= ACPI_MAX_RAM_SLOTS); > + scope = aml_scope("\\_SB.PCI0." stringify(MEMORY_HOTPLUG_DEVICE)); > + aml_append(scope, > + aml_name_decl(stringify(MEMORY_SLOTS_NUMBER), aml_int(nr_mem)) > + ); > + > + crs = aml_resource_template(); > + aml_append(crs, > + aml_io(AML_DECODE16, io_base, io_base, 0, io_len) > + ); > + aml_append(scope, aml_name_decl("_CRS", crs)); > + > + aml_append(scope, aml_operation_region( > + stringify(MEMORY_HOTPLUG_IO_REGION), AML_SYSTEM_IO, > + io_base, io_len) > + ); > + > + field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), AML_DWORD_ACC, > + AML_NOLOCK, AML_PRESERVE); > + aml_append(field, /* read only */ > + aml_named_field(stringify(MEMORY_SLOT_ADDR_LOW), 32)); > + aml_append(field, /* read only */ > + aml_named_field(stringify(MEMORY_SLOT_ADDR_HIGH), 32)); > + aml_append(field, /* read only */ > + aml_named_field(stringify(MEMORY_SLOT_SIZE_LOW), 32)); > + aml_append(field, /* read only */ > + aml_named_field(stringify(MEMORY_SLOT_SIZE_HIGH), 32)); > + aml_append(field, /* read only */ > + aml_named_field(stringify(MEMORY_SLOT_PROXIMITY), 32)); > + aml_append(scope, field); > + > + field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), AML_BYTE_ACC, > + AML_NOLOCK, AML_WRITE_AS_ZEROS); > + aml_append(field, aml_reserved_field(160 /* bits, Offset(20) */)); > + aml_append(field, /* 1 if enabled, read only */ > + aml_named_field(stringify(MEMORY_SLOT_ENABLED), 1)); > + aml_append(field, > + /*(read) 1 if has a insert event. (write) 1 to clear event */ > + aml_named_field(stringify(MEMORY_SLOT_INSERT_EVENT), 1)); > + aml_append(field, > + /* (read) 1 if has a remove event. (write) 1 to clear event */ > + aml_named_field(stringify(MEMORY_SLOT_REMOVE_EVENT), 1)); > + aml_append(field, > + /* initiates device eject, write only */ > + aml_named_field(stringify(MEMORY_SLOT_EJECT), 1)); > + aml_append(scope, field); > + > + field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), AML_DWORD_ACC, > + AML_NOLOCK, AML_PRESERVE); > + aml_append(field, /* DIMM selector, write only */ > + aml_named_field(stringify(MEMORY_SLOT_SLECTOR), 32)); > + aml_append(field, /* _OST event code, write only */ > + aml_named_field(stringify(MEMORY_SLOT_OST_EVENT), 32)); > + aml_append(field, /* _OST status code, write only */ > + aml_named_field(stringify(MEMORY_SLOT_OST_STATUS), 32)); > + aml_append(scope, field); > + aml_append(sb_scope, scope); > + > + for (i = 0; i < nr_mem; i++) { > + #define BASEPATH "\\_SB.PCI0." stringify(MEMORY_HOTPLUG_DEVICE) "." Again we'll have to clean this up down the line. All #defines should start at the 1st column. And we should not pollute global namespace like this. Would ACPI_MEMHP_BASEPATH be all that bad? I think not. Existing code so can be a separate patch on top. > + const char *s; > + > + dev = aml_device("MP%02X", i); > + aml_append(dev, aml_name_decl("_UID", aml_string("0x%02X", i))); > + aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C80"))); > + > + method = aml_method("_CRS", 0, AML_NOTSERIALIZED); > + s = BASEPATH stringify(MEMORY_SLOT_CRS_METHOD); > + aml_append(method, aml_return(aml_call1(s, aml_name("_UID")))); > + aml_append(dev, method); > + > + method = aml_method("_STA", 0, AML_NOTSERIALIZED); > + s = BASEPATH stringify(MEMORY_SLOT_STATUS_METHOD); > + aml_append(method, aml_return(aml_call1(s, aml_name("_UID")))); > + aml_append(dev, method); > + > + method = aml_method("_PXM", 0, AML_NOTSERIALIZED); > + s = BASEPATH stringify(MEMORY_SLOT_PROXIMITY_METHOD); > + aml_append(method, aml_return(aml_call1(s, aml_name("_UID")))); > + aml_append(dev, method); > + > + method = aml_method("_OST", 3, AML_NOTSERIALIZED); > + s = BASEPATH stringify(MEMORY_SLOT_OST_METHOD); > + aml_append(method, aml_return(aml_call4( > + s, aml_name("_UID"), aml_arg(0), aml_arg(1), aml_arg(2) > + ))); > + aml_append(dev, method); > + > + method = aml_method("_EJ0", 1, AML_NOTSERIALIZED); > + s = BASEPATH stringify(MEMORY_SLOT_EJECT_METHOD); > + aml_append(method, aml_return(aml_call2( > + s, aml_name("_UID"), aml_arg(0)))); > + aml_append(dev, method); > + > + aml_append(sb_scope, dev); > + } > + > + /* build Method(MEMORY_SLOT_NOTIFY_METHOD, 2) { > + * If (LEqual(Arg0, 0x00)) {Notify(MP00, Arg1)} ... } > + */ BTW this needs a cleanup. It does nothing to clarify what is the method doing, which by now I have forgotten :( ASL is generally barely readable, if one finds documenting C using ASL one knows one is in trouble :) > + method = aml_method(stringify(MEMORY_SLOT_NOTIFY_METHOD), 2, > + AML_NOTSERIALIZED); > + for (i = 0; i < nr_mem; i++) { > + ifctx = aml_if(aml_equal(aml_arg(0), aml_int(i))); > + aml_append(ifctx, > + aml_notify(aml_name("MP%.02X", i), aml_arg(1)) > + ); > + aml_append(method, ifctx); > + } > + aml_append(sb_scope, method); > +} > + > static void > build_ssdt(GArray *table_data, GArray *linker, > AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc, > @@ -1206,119 +1330,8 @@ build_ssdt(GArray *table_data, GArray *linker, > } > aml_append(sb_scope, aml_name_decl("CPON", pkg)); > > - /* build memory devices */ > - assert(nr_mem <= ACPI_MAX_RAM_SLOTS); > - scope = aml_scope("\\_SB.PCI0." stringify(MEMORY_HOTPLUG_DEVICE)); > - aml_append(scope, > - aml_name_decl(stringify(MEMORY_SLOTS_NUMBER), aml_int(nr_mem)) > - ); > - > - crs = aml_resource_template(); > - aml_append(crs, > - aml_io(AML_DECODE16, pm->mem_hp_io_base, pm->mem_hp_io_base, 0, > - pm->mem_hp_io_len) > - ); > - aml_append(scope, aml_name_decl("_CRS", crs)); > - > - aml_append(scope, aml_operation_region( > - stringify(MEMORY_HOTPLUG_IO_REGION), AML_SYSTEM_IO, > - pm->mem_hp_io_base, pm->mem_hp_io_len) > - ); > - > - field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), AML_DWORD_ACC, > - AML_NOLOCK, AML_PRESERVE); > - aml_append(field, /* read only */ > - aml_named_field(stringify(MEMORY_SLOT_ADDR_LOW), 32)); > - aml_append(field, /* read only */ > - aml_named_field(stringify(MEMORY_SLOT_ADDR_HIGH), 32)); > - aml_append(field, /* read only */ > - aml_named_field(stringify(MEMORY_SLOT_SIZE_LOW), 32)); > - aml_append(field, /* read only */ > - aml_named_field(stringify(MEMORY_SLOT_SIZE_HIGH), 32)); > - aml_append(field, /* read only */ > - aml_named_field(stringify(MEMORY_SLOT_PROXIMITY), 32)); > - aml_append(scope, field); > - > - field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), AML_BYTE_ACC, > - AML_NOLOCK, AML_WRITE_AS_ZEROS); > - aml_append(field, aml_reserved_field(160 /* bits, Offset(20) */)); > - aml_append(field, /* 1 if enabled, read only */ > - aml_named_field(stringify(MEMORY_SLOT_ENABLED), 1)); > - aml_append(field, > - /*(read) 1 if has a insert event. (write) 1 to clear event */ > - aml_named_field(stringify(MEMORY_SLOT_INSERT_EVENT), 1)); > - aml_append(field, > - /* (read) 1 if has a remove event. (write) 1 to clear event */ > - aml_named_field(stringify(MEMORY_SLOT_REMOVE_EVENT), 1)); > - aml_append(field, > - /* initiates device eject, write only */ > - aml_named_field(stringify(MEMORY_SLOT_EJECT), 1)); > - aml_append(scope, field); > - > - field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), AML_DWORD_ACC, > - AML_NOLOCK, AML_PRESERVE); > - aml_append(field, /* DIMM selector, write only */ > - aml_named_field(stringify(MEMORY_SLOT_SLECTOR), 32)); > - aml_append(field, /* _OST event code, write only */ > - aml_named_field(stringify(MEMORY_SLOT_OST_EVENT), 32)); > - aml_append(field, /* _OST status code, write only */ > - aml_named_field(stringify(MEMORY_SLOT_OST_STATUS), 32)); > - aml_append(scope, field); > - > - aml_append(sb_scope, scope); > - > - for (i = 0; i < nr_mem; i++) { > - #define BASEPATH "\\_SB.PCI0." stringify(MEMORY_HOTPLUG_DEVICE) "." > - const char *s; > - > - dev = aml_device("MP%02X", i); > - aml_append(dev, aml_name_decl("_UID", aml_string("0x%02X", i))); > - aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C80"))); > - > - method = aml_method("_CRS", 0, AML_NOTSERIALIZED); > - s = BASEPATH stringify(MEMORY_SLOT_CRS_METHOD); > - aml_append(method, aml_return(aml_call1(s, aml_name("_UID")))); > - aml_append(dev, method); > - > - method = aml_method("_STA", 0, AML_NOTSERIALIZED); > - s = BASEPATH stringify(MEMORY_SLOT_STATUS_METHOD); > - aml_append(method, aml_return(aml_call1(s, aml_name("_UID")))); > - aml_append(dev, method); > - > - method = aml_method("_PXM", 0, AML_NOTSERIALIZED); > - s = BASEPATH stringify(MEMORY_SLOT_PROXIMITY_METHOD); > - aml_append(method, aml_return(aml_call1(s, aml_name("_UID")))); > - aml_append(dev, method); > - > - method = aml_method("_OST", 3, AML_NOTSERIALIZED); > - s = BASEPATH stringify(MEMORY_SLOT_OST_METHOD); > - aml_append(method, aml_return(aml_call4( > - s, aml_name("_UID"), aml_arg(0), aml_arg(1), aml_arg(2) > - ))); > - aml_append(dev, method); > - > - method = aml_method("_EJ0", 1, AML_NOTSERIALIZED); > - s = BASEPATH stringify(MEMORY_SLOT_EJECT_METHOD); > - aml_append(method, aml_return(aml_call2( > - s, aml_name("_UID"), aml_arg(0)))); > - aml_append(dev, method); > - > - aml_append(sb_scope, dev); > - } > - > - /* build Method(MEMORY_SLOT_NOTIFY_METHOD, 2) { > - * If (LEqual(Arg0, 0x00)) {Notify(MP00, Arg1)} ... } > - */ > - method = aml_method(stringify(MEMORY_SLOT_NOTIFY_METHOD), 2, > - AML_NOTSERIALIZED); > - for (i = 0; i < nr_mem; i++) { > - ifctx = aml_if(aml_equal(aml_arg(0), aml_int(i))); > - aml_append(ifctx, > - aml_notify(aml_name("MP%.02X", i), aml_arg(1)) > - ); > - aml_append(method, ifctx); > - } > - aml_append(sb_scope, method); > + build_memory_devices(sb_scope, nr_mem, pm->mem_hp_io_base, > + pm->mem_hp_io_len); > > { > Object *pci_host; > -- > 1.8.3.1 >