Re: [Qemu-devel] [RFC 4/9] acpi: replace opencoded opcodes with defines

[Michael S. Tsirkin]

On Fri, Feb 07, 2014 at 01:51:31PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <address@hidden>


The reason I avoided doing this is that this
conflicts with qemu coding style which
only uses camel case for types.

So as a minimum this needs a comment
explaining that we are using the names from
ACPI spec as-is, that's why we deviate from
the coding style, to simplify matching against
that.

Something like below:

> ---
>  hw/i386/acpi-build.c |   28 ++++++++++++++++++----------
>  1 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 6a43a7d..1dbe5ce 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -224,6 +224,14 @@ static void acpi_get_pci_info(PcPciInfo *info)
>  #define ACPI_BUILD_TABLE_FILE "etc/acpi/tables"
>  #define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp"


/* Constants from ACPI spec 5.0a:
 *  ACPI Machine Language (AML) Specification
 */

We probably should add in spec link as well.


> +#define BytePrefix         0x0A
> +#define WordPrefix         0x0B
> +#define DWordPrefix        0x0C

Not sure about these ones.
There's a single user, and naming is different
from rest of operators which makes it
a bit confusing.
Maybe define near the user?

> +
> +#define NameOp             0x08
> +#define ScopeOp            0x10
> +#define DeviceOp           0x82

Hmm if we are doing this let's do this for all Ops.

> +
>  static void
>  build_header(GArray *linker, GArray *table_data,
>               AcpiTableHeader *h, uint32_t sig, int len, uint8_t rev)
> @@ -364,13 +372,13 @@ static void build_append_value(GArray *table, uint32_t 
> value, int size)
>  
>      switch (size) {
>      case 1:
> -        prefix = 0x0A; /* BytePrefix */
> +        prefix = BytePrefix;
>          break;
>      case 2:
> -        prefix = 0x0B; /* WordPrefix */
> +        prefix = WordPrefix;
>          break;
>      case 4:
> -        prefix = 0x0C; /* DWordPrefix */
> +        prefix = DWordPrefix;
>          break;
>      default:
>          assert(0);
> @@ -762,24 +770,24 @@ static void build_pci_bus_end(PCIBus *bus, void 
> *bus_state)
>      bool bus_hotplug_support = false;
>  
>      if (bus->parent_dev) {
> -        op = 0x82; /* DeviceOp */
> +        op = DeviceOp;
>          build_append_nameseg(bus_table, "S%.02X_",
>                               bus->parent_dev->devfn);
> -        build_append_byte(bus_table, 0x08); /* NameOp */
> +        build_append_byte(bus_table, NameOp);
>          build_append_nameseg(bus_table, "_SUN");
>          build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
> -        build_append_byte(bus_table, 0x08); /* NameOp */
> +        build_append_byte(bus_table, NameOp);
>          build_append_nameseg(bus_table, "_ADR");
>          build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 
> 16) |
>                             PCI_FUNC(bus->parent_dev->devfn), 4);
>      } else {
> -        op = 0x10; /* ScopeOp */;
> +        op = ScopeOp;
>          build_append_nameseg(bus_table, "PCI0");
>      }
>  
>      bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, 
> NULL);
>      if (bsel) {
> -        build_append_byte(bus_table, 0x08); /* NameOp */
> +        build_append_byte(bus_table, NameOp);
>          build_append_nameseg(bus_table, "BSEL");
>          build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
>      }
> @@ -962,7 +970,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>  
>      {
>          GArray *sb_scope = build_alloc_array();
> -        uint8_t op = 0x10; /* ScopeOp */
> +        uint8_t op = ScopeOp;
>  
>          build_append_nameseg(sb_scope, "_SB_");
>  
> @@ -983,7 +991,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>          build_append_notify_method(sb_scope, "NTFY", "CP%0.02X", acpi_cpus);
>  
>          /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" 
> */
> -        build_append_byte(sb_scope, 0x08); /* NameOp */
> +        build_append_byte(sb_scope, NameOp);
>          build_append_nameseg(sb_scope, "CPON");
>  
>          {
> -- 
> 1.7.1