Re: [Qemu-arm] [PATCH v2 01/11] hw/acpi: Move constant definitions to header files

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Auger Eric [mailto:address@hidden
> Sent: 08 March 2019 16:09
> To: Shameerali Kolothum Thodi <address@hidden>;
> address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden;
> address@hidden; address@hidden
> Cc: Linuxarm <address@hidden>; xuwei (O) <address@hidden>
> Subject: Re: [PATCH v2 01/11] hw/acpi: Move constant definitions to header
> files
> 
> Hi,
> 
> On 3/8/19 12:42 PM, Shameer Kolothum wrote:
> > From: Sebastien Boeuf <address@hidden>
> >
> > By moving the definition of memory hotplug related constants used
> > by ACPI for both CPU and memory, this commits allows those to be
> > used from  other parts of the code.
> 
> Maybe elaborate on where you intend to use them.

Ok.
 
> >
> > Signed-off-by: Sebastien Boeuf <address@hidden>
> > Signed-off-by: Shameer Kolothum <address@hidden>
> > ---
> >  hw/acpi/memory_hotplug.c         | 26 --------------------------
> >  include/hw/acpi/memory_hotplug.h | 26 ++++++++++++++++++++++++++
> >  2 files changed, 26 insertions(+), 26 deletions(-)
> >
> > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> > index 921cad2..a6beb10 100644
> > --- a/hw/acpi/memory_hotplug.c
> > +++ b/hw/acpi/memory_hotplug.c
> > @@ -8,32 +8,6 @@
> >  #include "qapi/error.h"
> >  #include "qapi/qapi-events-misc.h"
> >
> > -#define MEMORY_SLOTS_NUMBER          "MDNR"
> > -#define MEMORY_HOTPLUG_IO_REGION     "HPMR"
> > -#define MEMORY_SLOT_ADDR_LOW         "MRBL"
> > -#define MEMORY_SLOT_ADDR_HIGH        "MRBH"
> > -#define MEMORY_SLOT_SIZE_LOW         "MRLL"
> > -#define MEMORY_SLOT_SIZE_HIGH        "MRLH"
> > -#define MEMORY_SLOT_PROXIMITY        "MPX"
> > -#define MEMORY_SLOT_ENABLED          "MES"
> > -#define MEMORY_SLOT_INSERT_EVENT     "MINS"
> > -#define MEMORY_SLOT_REMOVE_EVENT     "MRMV"
> > -#define MEMORY_SLOT_EJECT            "MEJ"
> > -#define MEMORY_SLOT_SLECTOR          "MSEL"
> > -#define MEMORY_SLOT_OST_EVENT        "MOEV"
> > -#define MEMORY_SLOT_OST_STATUS       "MOSC"
> > -#define MEMORY_SLOT_LOCK             "MLCK"
> > -#define MEMORY_SLOT_STATUS_METHOD    "MRST"
> > -#define MEMORY_SLOT_CRS_METHOD       "MCRS"
> > -#define MEMORY_SLOT_OST_METHOD       "MOST"
> > -#define MEMORY_SLOT_PROXIMITY_METHOD "MPXM"
> > -#define MEMORY_SLOT_EJECT_METHOD     "MEJ0"
> > -#define MEMORY_SLOT_NOTIFY_METHOD    "MTFY"
> > -#define MEMORY_SLOT_SCAN_METHOD      "MSCN"
> > -#define MEMORY_HOTPLUG_DEVICE        "MHPD"
> > -#define MEMORY_HOTPLUG_IO_LEN         24
> > -#define MEMORY_DEVICES_CONTAINER     "\\_SB.MHPC"
> 
> Do we really need to expose all of them. I just can see
> MEMORY_SLOT_SCAN_METHOD used in hw/acpi/ged.c? Maybe I missed some
> though?

That is right. This was taken from the Nemu approach and I thought it might be
useful in future changes. But if there is no major motivation in keeping this 
in header,
I may skip this patch and just move the MEMORY_SLOT_SCAN_METHOD.

> Also at the beginning I tried to find some specification details for
> those stuff but I failed. It would be helpful for a non specialist
> reader to add a comment that helps to understand what is part of a
> specification (link?) and what is arbitrarily defined.

Ok. I could only find this Nemu wiki description here
https://github.com/intel/nemu/wiki/Memory-hotplug

Not sure Qemu or ACPI has a similar documentation elsewhere.

Thanks,
Shameer

> Thanks
> 
> Eric
> > -
> >  static uint16_t memhp_io_base;
> >
> >  static ACPIOSTInfo *acpi_memory_device_status(int slot, MemStatus
> *mdev)
> > diff --git a/include/hw/acpi/memory_hotplug.h
> b/include/hw/acpi/memory_hotplug.h
> > index 77c6576..fbfcbe6 100644
> > --- a/include/hw/acpi/memory_hotplug.h
> > +++ b/include/hw/acpi/memory_hotplug.h
> > @@ -5,6 +5,32 @@
> >  #include "hw/acpi/acpi.h"
> >  #include "hw/acpi/aml-build.h"
> >
> > +#define MEMORY_SLOTS_NUMBER          "MDNR"
> > +#define MEMORY_HOTPLUG_IO_REGION     "HPMR"
> > +#define MEMORY_SLOT_ADDR_LOW         "MRBL"
> > +#define MEMORY_SLOT_ADDR_HIGH        "MRBH"
> > +#define MEMORY_SLOT_SIZE_LOW         "MRLL"
> > +#define MEMORY_SLOT_SIZE_HIGH        "MRLH"
> > +#define MEMORY_SLOT_PROXIMITY        "MPX"
> > +#define MEMORY_SLOT_ENABLED          "MES"
> > +#define MEMORY_SLOT_INSERT_EVENT     "MINS"
> > +#define MEMORY_SLOT_REMOVE_EVENT     "MRMV"
> > +#define MEMORY_SLOT_EJECT            "MEJ"
> > +#define MEMORY_SLOT_SLECTOR          "MSEL"
> > +#define MEMORY_SLOT_OST_EVENT        "MOEV"
> > +#define MEMORY_SLOT_OST_STATUS       "MOSC"
> > +#define MEMORY_SLOT_LOCK             "MLCK"
> > +#define MEMORY_SLOT_STATUS_METHOD    "MRST"
> > +#define MEMORY_SLOT_CRS_METHOD       "MCRS"
> > +#define MEMORY_SLOT_OST_METHOD       "MOST"
> > +#define MEMORY_SLOT_PROXIMITY_METHOD "MPXM"
> > +#define MEMORY_SLOT_EJECT_METHOD     "MEJ0"
> > +#define MEMORY_SLOT_NOTIFY_METHOD    "MTFY"
> > +#define MEMORY_SLOT_SCAN_METHOD      "MSCN"
> > +#define MEMORY_HOTPLUG_DEVICE        "MHPD"
> > +#define MEMORY_HOTPLUG_IO_LEN         24
> > +#define MEMORY_DEVICES_CONTAINER     "\\_SB.MHPC"
> > +
> >  /**
> >   * MemStatus:
> >   * @is_removing: the memory device in slot has been requested to be
> ejected.
> >