On Thu, 22 Dec 2016 15:53:55 +0200
Marcel Apfelbaum <mar...@redhat.com> wrote:
> On 12/22/2016 03:31 PM, Igor Mammedov wrote:
> > On Thu, 22 Dec 2016 14:31:19 +0200
> > Marcel Apfelbaum <mar...@redhat.com> wrote:
> >
> >> On 12/06/2016 01:32 AM, Igor Mammedov wrote:
> >>> Move DIMM devices from global _SB scope to a new \_SB.MHPC
> >>> container along with common methods used by DIMMs:
> >>> MCRS, MRST, MPXM, MOST, MEJ00, MSCN, MTFY
> >>>
> >>> this reduces AML size on 12 * #slots bytes,
> >>> i.e. up to 3072 bytes for 265 slots.
> >>>
> >>
> >> Can you please explain how the bytes number are reduced?
> >> Is it because the devices path is now relative to the new container?
> > yes
> >
> >> If so, can you give an example of before/after?
> > as example:
> >
> > - Device (MP02)
> > - {
> > - Name (_UID, "0x02") // _UID: Unique ID
> > - Name (_HID, EisaId ("PNP0C80") /* Memory Device */) // _HID:
> > Hardware ID
> > - Method (_CRS, 0, NotSerialized) // _CRS: Current Resource
> > Settings
> > - {
> > - Return (\_SB.PCI0.MHPD.MCRS (_UID))
> > - }
> > -
> > - Method (_STA, 0, NotSerialized) // _STA: Status
> > - {
> > - Return (\_SB.PCI0.MHPD.MRST (_UID))
> > - }
> > -
> > - Method (_PXM, 0, NotSerialized) // _PXM: Device Proximity
> > - {
> > - Return (\_SB.PCI0.MHPD.MPXM (_UID))
> > - }
> > -
> > - Method (_OST, 3, NotSerialized) // _OST: OSPM Status
> > Indication
> > - {
> > - Return (\_SB.PCI0.MHPD.MOST (_UID, Arg0, Arg1, Arg2))
> > - }
> > -
> > - Method (_EJ0, 1, NotSerialized) // _EJx: Eject Device
> > - {
> > - Return (\_SB.PCI0.MHPD.MEJ0 (_UID, Arg0))
> > - }
> > - }
> > ----
> > + Device (MP02)
> > + {
> > + Name (_UID, "0x02") // _UID: Unique ID
> > + Name (_HID, EisaId ("PNP0C80") /* Memory Device */) // _HID:
> > Hardware ID
> > + Method (_CRS, 0, NotSerialized) // _CRS: Current Resource
> > Settings
> > + {
> > + Return (MCRS (_UID))
> > + }
> > +
> > + Method (_STA, 0, NotSerialized) // _STA: Status
> > + {
> > + Return (MRST (_UID))
> > + }
> > +
> > + Method (_PXM, 0, NotSerialized) // _PXM: Device Proximity
> > + {
> > + Return (MPXM (_UID))
> > + }
> > +
> > + Method (_OST, 3, NotSerialized) // _OST: OSPM Status
> > Indication
> > + {
> > + Return (MOST (_UID, Arg0, Arg1, Arg2))
> > + }
> > +
> > + Method (_EJ0, 1, NotSerialized) // _EJx: Eject Device
> > + {
> > + Return (MEJ0 (_UID, Arg0))
> > + }
> > + }
> >
>
> Thanks for the confirmation.
>
> >>
> >>> Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> >>> ---
> >>> hw/acpi/memory_hotplug.c | 190
> >>> ++++++++++++++++++++++++-----------------------
> >>> 1 file changed, 97 insertions(+), 93 deletions(-)
> >>>
> >>> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> >>> index fb04d24..210073d 100644
> >>> --- a/hw/acpi/memory_hotplug.c
> >>> +++ b/hw/acpi/memory_hotplug.c
> >>> @@ -31,6 +31,7 @@
> >>> #define MEMORY_SLOT_SCAN_METHOD "MSCN"
> >>> #define MEMORY_HOTPLUG_DEVICE "MHPD"
> >>> #define MEMORY_HOTPLUG_IO_LEN 24
> >>> +#define MEMORY_DEVICES_CONTAINER "\\_SB.MHPC"
> >>>
> >>> static uint16_t memhp_io_base;
> >>>
> >>> @@ -343,9 +344,8 @@ void build_memory_hotplug_aml(Aml *table, uint32_t
> >>> nr_mem,
> >>> int i;
> >>> Aml *ifctx;
> >>> Aml *method;
> >>> - Aml *sb_scope;
> >>> + Aml *dev_container;
> >>> Aml *mem_ctrl_dev;
> >>> - char *scan_path;
> >>> char *mhp_res_path;
> >>>
> >>> if (!memhp_io_base) {
> >>> @@ -356,24 +356,11 @@ void build_memory_hotplug_aml(Aml *table, uint32_t
> >>> nr_mem,
> >>> mem_ctrl_dev = aml_device("%s", mhp_res_path);
> >>> {
> >>> Aml *crs;
> >>> - Aml *field;
> >>> - Aml *one = aml_int(1);
> >>> - Aml *zero = aml_int(0);
> >>> - Aml *ret_val = aml_local(0);
> >>> - Aml *slot_arg0 = aml_arg(0);
> >>> - Aml *slots_nr = aml_name(MEMORY_SLOTS_NUMBER);
> >>> - Aml *ctrl_lock = aml_name(MEMORY_SLOT_LOCK);
> >>> - Aml *slot_selector = aml_name(MEMORY_SLOT_SLECTOR);
> >>>
> >>> aml_append(mem_ctrl_dev, aml_name_decl("_HID",
> >>> aml_string("PNP0A06")));
> >>
> >> Do we need the above declaration twice? Here is the first occurrence.
> >>
> >>> aml_append(mem_ctrl_dev,
> >>> aml_name_decl("_UID", aml_string("Memory hotplug
> >>> resources")));
> > [...]
> >
> >>> +
> >>> + dev_container = aml_device(MEMORY_DEVICES_CONTAINER);
> >>> + {
> > [...]
> >>> + aml_append(dev_container, aml_name_decl("_HID",
> >>> aml_string("PNP0A06")));
> >>
> >> And here is the second occurrence.
> > It's different containers, the first one is for MMIO registers
> > for x86 it's placed \_SB.PCI0.MHPD consumes resources from parent's
> > PCI0._CRS
> > and the second is for memory devices \_SB.MHPC which consumes resources
> > outside of PCI0._CRS scope.
> >
>
> Understood, here is my last R-b tag, however I want to point out
> that I only looked at the re-factoring steps and my acpi-based memory hotplug
> knowledge is rather limited.
>
>
> Reviewed-by: Marcel Apfelbaum <mar...@redhat.com>
Thanks,
It's pretty much series subj says cleanups/consolidation.
The only sort of 'functional' change is that memory hotplug
AML is generated only if memory hotplug is enabled, freeing
some guest memory when memory hotplug in not in use.
> Thanks,
> Marcel
>
> >
> >>
> >> Thanks,
> >> Marcel
> >>
> >>> + aml_append(dev_container,
> >>> + aml_name_decl("_UID", aml_string("DIMM devices")));
> >>> +
> >>> + assert(nr_mem <= ACPI_MAX_RAM_SLOTS);
> >>> + aml_append(dev_container,
> >>> + aml_name_decl(MEMORY_SLOTS_NUMBER, aml_int(nr_mem))
> >>> + );
> >>> +
> >>> + field = aml_field(mmio_path, AML_DWORD_ACC,
> >>> AML_NOLOCK, AML_PRESERVE);
> >>> aml_append(field, /* read only */
> >>> aml_named_field(MEMORY_SLOT_ADDR_LOW, 32));
> >>> @@ -398,9 +410,9 @@ void build_memory_hotplug_aml(Aml *table, uint32_t
> >>> nr_mem,
> >>> aml_named_field(MEMORY_SLOT_SIZE_HIGH, 32));
> >>> aml_append(field, /* read only */
> >>> aml_named_field(MEMORY_SLOT_PROXIMITY, 32));
> >>> - aml_append(mem_ctrl_dev, field);
> >>> + aml_append(dev_container, field);
> >>>
> >>> - field = aml_field(MEMORY_HOTPLUG_IO_REGION, AML_BYTE_ACC,
> >>> + field = aml_field(mmio_path, 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 */
> >>> @@ -414,9 +426,9 @@ void build_memory_hotplug_aml(Aml *table, uint32_t
> >>> nr_mem,
> >>> aml_append(field,
> >>> /* initiates device eject, write only */
> >>> aml_named_field(MEMORY_SLOT_EJECT, 1));
> >>> - aml_append(mem_ctrl_dev, field);
> >>> + aml_append(dev_container, field);
> >>>
> >>> - field = aml_field(MEMORY_HOTPLUG_IO_REGION, AML_DWORD_ACC,
> >>> + field = aml_field(mmio_path, AML_DWORD_ACC,
> >>> AML_NOLOCK, AML_PRESERVE);
> >>> aml_append(field, /* DIMM selector, write only */
> >>> aml_named_field(MEMORY_SLOT_SLECTOR, 32));
> >>> @@ -424,7 +436,8 @@ void build_memory_hotplug_aml(Aml *table, uint32_t
> >>> nr_mem,
> >>> aml_named_field(MEMORY_SLOT_OST_EVENT, 32));
> >>> aml_append(field, /* _OST status code, write only */
> >>> aml_named_field(MEMORY_SLOT_OST_STATUS, 32));
> >>> - aml_append(mem_ctrl_dev, field);
> >>> + aml_append(dev_container, field);
> >>> + g_free(mmio_path);
> >>>
> >>> method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> >>> ifctx = aml_if(aml_equal(slots_nr, zero));
> >>> @@ -434,9 +447,9 @@ void build_memory_hotplug_aml(Aml *table, uint32_t
> >>> nr_mem,
> >>> aml_append(method, ifctx);
> >>> /* present, functioning, decoding, not shown in UI */
> >>> aml_append(method, aml_return(aml_int(0xB)));
> >>> - aml_append(mem_ctrl_dev, method);
> >>> + aml_append(dev_container, method);
> >>>
> >>> - aml_append(mem_ctrl_dev, aml_mutex(MEMORY_SLOT_LOCK, 0));
> >>> + aml_append(dev_container, aml_mutex(MEMORY_SLOT_LOCK, 0));
> >>>
> >>> method = aml_method(MEMORY_SLOT_SCAN_METHOD, 0,
> >>> AML_NOTSERIALIZED);
> >>> {
> >>> @@ -492,7 +505,7 @@ void build_memory_hotplug_aml(Aml *table, uint32_t
> >>> nr_mem,
> >>> aml_append(method, aml_release(ctrl_lock));
> >>> aml_append(method, aml_return(one));
> >>> }
> >>> - aml_append(mem_ctrl_dev, method);
> >>> + aml_append(dev_container, method);
> >>>
> >>> method = aml_method(MEMORY_SLOT_STATUS_METHOD, 1,
> >>> AML_NOTSERIALIZED);
> >>> {
> >>> @@ -512,7 +525,7 @@ void build_memory_hotplug_aml(Aml *table, uint32_t
> >>> nr_mem,
> >>> aml_append(method, aml_release(ctrl_lock));
> >>> aml_append(method, aml_return(ret_val));
> >>> }
> >>> - aml_append(mem_ctrl_dev, method);
> >>> + aml_append(dev_container, method);
> >>>
> >>> method = aml_method(MEMORY_SLOT_CRS_METHOD, 1, AML_SERIALIZED);
> >>> {
> >>> @@ -603,7 +616,7 @@ void build_memory_hotplug_aml(Aml *table, uint32_t
> >>> nr_mem,
> >>> aml_append(method, aml_release(ctrl_lock));
> >>> aml_append(method, aml_return(mr64));
> >>> }
> >>> - aml_append(mem_ctrl_dev, method);
> >>> + aml_append(dev_container, method);
> >>>
> >>> method = aml_method(MEMORY_SLOT_PROXIMITY_METHOD, 1,
> >>> AML_NOTSERIALIZED);
> >>> @@ -617,7 +630,7 @@ void build_memory_hotplug_aml(Aml *table, uint32_t
> >>> nr_mem,
> >>> aml_append(method, aml_release(ctrl_lock));
> >>> aml_append(method, aml_return(ret_val));
> >>> }
> >>> - aml_append(mem_ctrl_dev, method);
> >>> + aml_append(dev_container, method);
> >>>
> >>> method = aml_method(MEMORY_SLOT_OST_METHOD, 4,
> >>> AML_NOTSERIALIZED);
> >>> {
> >>> @@ -631,7 +644,7 @@ void build_memory_hotplug_aml(Aml *table, uint32_t
> >>> nr_mem,
> >>> aml_append(method, aml_store(aml_arg(2), ost_status));
> >>> aml_append(method, aml_release(ctrl_lock));
> >>> }
> >>> - aml_append(mem_ctrl_dev, method);
> >>> + aml_append(dev_container, method);
> >>>
> >>> method = aml_method(MEMORY_SLOT_EJECT_METHOD, 2,
> >>> AML_NOTSERIALIZED);
> >>> {
> >>> @@ -643,75 +656,66 @@ void build_memory_hotplug_aml(Aml *table, uint32_t
> >>> nr_mem,
> >>> aml_append(method, aml_store(one, eject));
> >>> aml_append(method, aml_release(ctrl_lock));
> >>> }
> >>> - aml_append(mem_ctrl_dev, method);
> >>> - }
> >>> - aml_append(table, mem_ctrl_dev);
> >>> -
> >>> - sb_scope = aml_scope("_SB");
> >>> - /* build memory devices */
> >>> - for (i = 0; i < nr_mem; i++) {
> >>> - Aml *dev;
> >>> - 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 = g_strdup_printf("%s.%s", mhp_res_path,
> >>> MEMORY_SLOT_CRS_METHOD);
> >>> - aml_append(method, aml_return(aml_call1(s, aml_name("_UID"))));
> >>> - g_free(s);
> >>> - aml_append(dev, method);
> >>> -
> >>> - method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> >>> - s = g_strdup_printf("%s.%s", mhp_res_path,
> >>> MEMORY_SLOT_STATUS_METHOD);
> >>> - aml_append(method, aml_return(aml_call1(s, aml_name("_UID"))));
> >>> - g_free(s);
> >>> - aml_append(dev, method);
> >>> -
> >>> - method = aml_method("_PXM", 0, AML_NOTSERIALIZED);
> >>> - s = g_strdup_printf("%s.%s", mhp_res_path,
> >>> - MEMORY_SLOT_PROXIMITY_METHOD);
> >>> - aml_append(method, aml_return(aml_call1(s, aml_name("_UID"))));
> >>> - g_free(s);
> >>> - aml_append(dev, method);
> >>> -
> >>> - method = aml_method("_OST", 3, AML_NOTSERIALIZED);
> >>> - s = g_strdup_printf("%s.%s", mhp_res_path,
> >>> MEMORY_SLOT_OST_METHOD);
> >>> - aml_append(method, aml_return(aml_call4(
> >>> - s, aml_name("_UID"), aml_arg(0), aml_arg(1), aml_arg(2)
> >>> - )));
> >>> - g_free(s);
> >>> - aml_append(dev, method);
> >>> -
> >>> - method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
> >>> - s = g_strdup_printf("%s.%s", mhp_res_path,
> >>> MEMORY_SLOT_EJECT_METHOD);
> >>> - aml_append(method, aml_return(aml_call2(
> >>> - s, aml_name("_UID"), aml_arg(0))));
> >>> - g_free(s);
> >>> - aml_append(dev, method);
> >>> -
> >>> - aml_append(sb_scope, dev);
> >>> - }
> >>> + aml_append(dev_container, method);
> >>> +
> >>> + /* build memory devices */
> >>> + for (i = 0; i < nr_mem; i++) {
> >>> + Aml *dev;
> >>> + 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 = 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 = 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 = 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 = 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 = MEMORY_SLOT_EJECT_METHOD;
> >>> + aml_append(method, aml_return(aml_call2(
> >>> + s, aml_name("_UID"), aml_arg(0))));
> >>> + aml_append(dev, method);
> >>> +
> >>> + aml_append(dev_container, dev);
> >>> + }
> >>>
> >>> - /* build Method(MEMORY_SLOT_NOTIFY_METHOD, 2) {
> >>> - * If (LEqual(Arg0, 0x00)) {Notify(MP00, Arg1)} ... }
> >>> - */
> >>> - method = aml_method(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);
> >>> + /* build Method(MEMORY_SLOT_NOTIFY_METHOD, 2) {
> >>> + * If (LEqual(Arg0, 0x00)) {Notify(MP00, Arg1)} ... }
> >>> + */
> >>> + method = aml_method(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(dev_container, method);
> >>> }
> >>> - aml_append(sb_scope, method);
> >>> - aml_append(table, sb_scope);
> >>> + aml_append(table, dev_container);
> >>>
> >>> method = aml_method(event_handler_method, 0, AML_NOTSERIALIZED);
> >>> - scan_path = g_strdup_printf("%s.%s", mhp_res_path,
> >>> MEMORY_SLOT_SCAN_METHOD);
> >>> - aml_append(method, aml_call0(scan_path));
> >>> - g_free(scan_path);
> >>> + aml_append(method,
> >>> + aml_call0(MEMORY_DEVICES_CONTAINER "." MEMORY_SLOT_SCAN_METHOD));
> >>> aml_append(table, method);
> >>>
> >>> g_free(mhp_res_path);
> >>>
> >>
> >
>
>
