Re: [PATCH] [RFC] tools/hvmloader: Control ACPI debugging using a platform flag

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Jan 17, 2014 at 02:54:58PM +0000, Andrew Cooper wrote:
> Since Qemu-trad
>   c/s 147f83f9b7d87a698c200c4f3eb2d36a0e4fe54b
>   "hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work."
> 
> there has been quite a lot of extra logging appearing in the VM logs.
> 
> The hotplug debugging contributes 2 vmexits per slot per hotplug event, which
> are simply a waste of time unless a developer is trying to debug VM
> hotplugging problems.
> 
> Introduce a platform flag, "acpi-debug" to indicate in the AML whether
> debugging writes are wanted or not.

Oddly I don't see my response. But why not just do #ifdef DEBUG around
the hvmloader code, and this patch:


diff --git a/hw/piix4acpi.c b/hw/piix4acpi.c
index bf916d9..2b0ce32 100644
--- a/hw/piix4acpi.c
+++ b/hw/piix4acpi.c
@@ -287,7 +287,6 @@ static inline void clear_bit(uint8_t *map, int bit)
 static void acpi_dbg_writel(void *opaque, uint32_t addr, uint32_t val)
 {
     PIIX4ACPI_LOG(PIIX4ACPI_LOG_DEBUG, "ACPI: DBG: 0x%08x\n", val);
-    PIIX4ACPI_LOG(PIIX4ACPI_LOG_INFO, "ACPI:debug: write addr=0x%x, val=0x%x.\n", addr, val);
 }
 
 #ifdef CONFIG_PASSTHROUGH


That will it will only happen when you build with 'debug=y' or such.

> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  docs/misc/xenstore-paths.markdown       |    1 +
>  tools/firmware/hvmloader/acpi/build.c   |    5 +++++
>  tools/firmware/hvmloader/acpi/dsdt.asl  |    1 +
>  tools/firmware/hvmloader/acpi/mk_dsdt.c |    6 ++++++
>  4 files changed, 13 insertions(+)
> 
> diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
> index 70ab7f4..593a7b1 100644
> --- a/docs/misc/xenstore-paths.markdown
> +++ b/docs/misc/xenstore-paths.markdown
> @@ -190,6 +190,7 @@ Various platform properties.
>  * acpi -- is ACPI enabled for this domain
>  * acpi_s3 -- is ACPI S3 support enabled for this domain
>  * acpi_s4 -- is ACPI S4 support enabled for this domain
> +* acpi-debug -- whether the AML code should write debugging messages to qemu
>  
>  #### ~/platform/generation-id = INTEGER ":" INTEGER [HVM,INTERNAL]
>  
> diff --git a/tools/firmware/hvmloader/acpi/build.c b/tools/firmware/hvmloader/acpi/build.c
> index f1dd3f0..59716bb 100644
> --- a/tools/firmware/hvmloader/acpi/build.c
> +++ b/tools/firmware/hvmloader/acpi/build.c
> @@ -47,6 +47,7 @@ struct acpi_info {
>      uint8_t  com2_present:1;    /* 0[1] - System has COM2? */
>      uint8_t  lpt1_present:1;    /* 0[2] - System has LPT1? */
>      uint8_t  hpet_present:1;    /* 0[3] - System has HPET? */
> +    uint8_t  acpi_debugging:1;  /* 0[4] - ACPI debugging enabled ? */
>      uint32_t pci_min, pci_len;  /* 4, 8 - PCI I/O hole boundaries */
>      uint32_t madt_csum_addr;    /* 12   - Address of MADT checksum */
>      uint32_t madt_lapic0_addr;  /* 16   - Address of first MADT LAPIC struct */
> @@ -404,6 +405,7 @@ void acpi_build_tables(struct acpi_config *config, unsigned int physical)
>      unsigned char       *dsdt;
>      unsigned long        secondary_tables[ACPI_MAX_SECONDARY_TABLES];
>      int                  nr_secondaries, i;
> +    const char          *xs_str;
>  
>      /* Allocate and initialise the acpi info area. */
>      mem_hole_populate_ram(ACPI_INFO_PHYSICAL_ADDRESS >> PAGE_SHIFT, 1);
> @@ -519,10 +521,13 @@ void acpi_build_tables(struct acpi_config *config, unsigned int physical)
>      if ( !new_vm_gid(acpi_info) )
>          goto oom;
>  
> +    xs_str = xenstore_read("platform/acpi-debug", "0");
> +
>      acpi_info->com1_present = uart_exists(0x3f8);
>      acpi_info->com2_present = uart_exists(0x2f8);
>      acpi_info->lpt1_present = lpt_exists(0x378);
>      acpi_info->hpet_present = hpet_exists(ACPI_HPET_ADDRESS);
> +    acpi_info->acpi_debugging = (xs_str[0] == '1');
>      acpi_info->pci_min = pci_mem_start;
>      acpi_info->pci_len = pci_mem_end - pci_mem_start;
>  
> diff --git a/tools/firmware/hvmloader/acpi/dsdt.asl b/tools/firmware/hvmloader/acpi/dsdt.asl
> index 247a8ad..e753286 100644
> --- a/tools/firmware/hvmloader/acpi/dsdt.asl
> +++ b/tools/firmware/hvmloader/acpi/dsdt.asl
> @@ -51,6 +51,7 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "Xen", "HVM", 0)
>             UAR2, 1,
>             LTP1, 1,
>             HPET, 1,
> +           ADBG, 1,
>             Offset(4),
>             PMIN, 32,
>             PLEN, 32,
> diff --git a/tools/firmware/hvmloader/acpi/mk_dsdt.c b/tools/firmware/hvmloader/acpi/mk_dsdt.c
> index a4b693b..3f0ca74 100644
> --- a/tools/firmware/hvmloader/acpi/mk_dsdt.c
> +++ b/tools/firmware/hvmloader/acpi/mk_dsdt.c
> @@ -347,14 +347,18 @@ int main(int argc, char **argv)
>              /* _SUN == dev */
>              stmt("Name", "_SUN, 0x%08x", slot >> 3);
>              push_block("Method", "_EJ0, 1");
> +            push_block("If", "LEqual( \\_SB.ADBG, ONE )");
>              stmt("Store", "0x%02x, \\_GPE.DPT1", slot);
>              stmt("Store", "0x88, \\_GPE.DPT2");
> +            pop_block();
>              stmt("Store", "0x%02x, \\_GPE.PH%02X", /* eject */
>                   (slot & 1) ? 0x10 : 0x01, slot & ~1);
>              pop_block();
>              push_block("Method", "_STA, 0");
> +            push_block("If", "LEqual( \\_SB.ADBG, ONE )");
>              stmt("Store", "0x%02x, \\_GPE.DPT1", slot);
>              stmt("Store", "0x89, \\_GPE.DPT2");
> +            pop_block();
>              if ( slot & 1 )
>                  stmt("ShiftRight", "0x4, \\_GPE.PH%02X, Local1", slot & ~1);
>              else
> @@ -426,8 +430,10 @@ int main(int argc, char **argv)
>          stmt("Store", "PSTB, Local1"); /* XXX: Store (PSTB, SLT) ? */
>          stmt("And", "Local1, 0xff, SLT");
>          /* Debug */
> +        push_block("If", "LEqual( \\_SB.ADBG, ONE )");
>          stmt("Store", "SLT, DPT1");
>          stmt("Store", "EVT, DPT2");
> +        pop_block();
>          /* Decision tree */
>          decision_tree(0x00, 0x100, "SLT", pci_hotplug_notify);
>          pop_block();
> -- 
> 1.7.10.4
>