[PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work.

[PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work.

[Konrad Rzeszutek Wilk]

The ACPI AML code has little snippets where it uses two
memory locations to stash debug information when doing PCI
hotplug, such as:

Device (S20)
{
    Name (_ADR, 0x00040000)
    Name (_SUN, 0x04)
    Method (_EJ0, 1, NotSerialized)
    {
	Store (0x20, \_GPE.DPT1)
	Store (0x88, \_GPE.DPT2)
	Store (One, \_GPE.PH20)
    }

    Method (_STA, 0, NotSerialized)
    {
	Store (0x20, \_GPE.DPT1)
	Store (0x89, \_GPE.DPT2)
    }
}

and DPT1 (and DPT2) is defined as:

OperationRegion ( DG1, SystemIO, 0xb044, 0x04 )
Field ( DG1, ByteAcc, NoLock, Preserve ) {
    DPT1, 8, DPT2, 8
}

But unfortunately when we do the writes they are done
as byte writes, not as 4-byte writes (long). Hence
any debug values are never show in QEMU.

This fixes it so that we can see them in the log file.

CC: ian.jackson@eu.citrix.com
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 hw/piix4acpi.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/piix4acpi.c b/hw/piix4acpi.c
index bf916d9..ddbe8e0 100644
--- a/hw/piix4acpi.c
+++ b/hw/piix4acpi.c
@@ -284,7 +284,7 @@ static inline void clear_bit(uint8_t *map, int bit)
     map[bit / 8] &= ~(1 << (bit % 8));
 }
 
-static void acpi_dbg_writel(void *opaque, uint32_t addr, uint32_t val)
+static void acpi_dbg_writeb(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);
@@ -776,7 +776,7 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
 #ifdef CONFIG_PASSTHROUGH
     php_devfn_init();
 #endif
-    register_ioport_write(ACPI_DBG_IO_ADDR, 4, 4, acpi_dbg_writel, d);
+    register_ioport_write(ACPI_DBG_IO_ADDR, 4, 1, acpi_dbg_writeb, d);
 
     register_savevm("piix4acpi", 0, 2, piix4acpi_save, piix4acpi_load, d);
 
-- 
1.7.7.6

Re: [PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work.

[Ross Philipson]

On 11/11/2013 01:42 PM, Konrad Rzeszutek Wilk wrote:
> The ACPI AML code has little snippets where it uses two
> memory locations to stash debug information when doing PCI
> hotplug, such as:
>
> Device (S20)
> {
>      Name (_ADR, 0x00040000)
>      Name (_SUN, 0x04)
>      Method (_EJ0, 1, NotSerialized)
>      {
> 	Store (0x20, \_GPE.DPT1)
> 	Store (0x88, \_GPE.DPT2)
> 	Store (One, \_GPE.PH20)
>      }
>
>      Method (_STA, 0, NotSerialized)
>      {
> 	Store (0x20, \_GPE.DPT1)
> 	Store (0x89, \_GPE.DPT2)
>      }
> }
>
> and DPT1 (and DPT2) is defined as:
>
> OperationRegion ( DG1, SystemIO, 0xb044, 0x04 )
> Field ( DG1, ByteAcc, NoLock, Preserve ) {
>      DPT1, 8, DPT2, 8
> }

Just as a note for the sake of correctness, that Field only defines 2 
bytes but the OpRegion is defined as 4 bytes (and doesn't need to be).

>
> But unfortunately when we do the writes they are done
> as byte writes, not as 4-byte writes (long). Hence
> any debug values are never show in QEMU.
>
> This fixes it so that we can see them in the log file.
>
> CC: ian.jackson@eu.citrix.com
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>   hw/piix4acpi.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/piix4acpi.c b/hw/piix4acpi.c
> index bf916d9..ddbe8e0 100644
> --- a/hw/piix4acpi.c
> +++ b/hw/piix4acpi.c
> @@ -284,7 +284,7 @@ static inline void clear_bit(uint8_t *map, int bit)
>       map[bit / 8] &= ~(1 << (bit % 8));
>   }
>
> -static void acpi_dbg_writel(void *opaque, uint32_t addr, uint32_t val)
> +static void acpi_dbg_writeb(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);
> @@ -776,7 +776,7 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>   #ifdef CONFIG_PASSTHROUGH
>       php_devfn_init();
>   #endif
> -    register_ioport_write(ACPI_DBG_IO_ADDR, 4, 4, acpi_dbg_writel, d);
> +    register_ioport_write(ACPI_DBG_IO_ADDR, 4, 1, acpi_dbg_writeb, d);
>
>       register_savevm("piix4acpi", 0, 2, piix4acpi_save, piix4acpi_load, d);
>
>

Re: [PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work.

[Konrad Rzeszutek Wilk]

On Mon, Nov 11, 2013 at 02:24:43PM -0500, Ross Philipson wrote:
> On 11/11/2013 01:42 PM, Konrad Rzeszutek Wilk wrote:
> >The ACPI AML code has little snippets where it uses two
> >memory locations to stash debug information when doing PCI
> >hotplug, such as:
> >
> >Device (S20)
> >{
> >     Name (_ADR, 0x00040000)
> >     Name (_SUN, 0x04)
> >     Method (_EJ0, 1, NotSerialized)
> >     {
> >	Store (0x20, \_GPE.DPT1)
> >	Store (0x88, \_GPE.DPT2)
> >	Store (One, \_GPE.PH20)
> >     }
> >
> >     Method (_STA, 0, NotSerialized)
> >     {
> >	Store (0x20, \_GPE.DPT1)
> >	Store (0x89, \_GPE.DPT2)
> >     }
> >}
> >
> >and DPT1 (and DPT2) is defined as:
> >
> >OperationRegion ( DG1, SystemIO, 0xb044, 0x04 )
> >Field ( DG1, ByteAcc, NoLock, Preserve ) {
> >     DPT1, 8, DPT2, 8
> >}
> 
> Just as a note for the sake of correctness, that Field only defines
> 2 bytes but the OpRegion is defined as 4 bytes (and doesn't need to
> be).

Yup, I saw that too and was wondering why it was defined that way.

It probably can use some more fixes :-)
> 
> >
> >But unfortunately when we do the writes they are done
> >as byte writes, not as 4-byte writes (long). Hence
> >any debug values are never show in QEMU.
> >
> >This fixes it so that we can see them in the log file.
> >
> >CC: ian.jackson@eu.citrix.com
> >Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >---
> >  hw/piix4acpi.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> >
> >diff --git a/hw/piix4acpi.c b/hw/piix4acpi.c
> >index bf916d9..ddbe8e0 100644
> >--- a/hw/piix4acpi.c
> >+++ b/hw/piix4acpi.c
> >@@ -284,7 +284,7 @@ static inline void clear_bit(uint8_t *map, int bit)
> >      map[bit / 8] &= ~(1 << (bit % 8));
> >  }
> >
> >-static void acpi_dbg_writel(void *opaque, uint32_t addr, uint32_t val)
> >+static void acpi_dbg_writeb(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);
> >@@ -776,7 +776,7 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
> >  #ifdef CONFIG_PASSTHROUGH
> >      php_devfn_init();
> >  #endif
> >-    register_ioport_write(ACPI_DBG_IO_ADDR, 4, 4, acpi_dbg_writel, d);
> >+    register_ioport_write(ACPI_DBG_IO_ADDR, 4, 1, acpi_dbg_writeb, d);
> >
> >      register_savevm("piix4acpi", 0, 2, piix4acpi_save, piix4acpi_load, d);
> >
> >
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work.

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("[PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work."):
> The ACPI AML code has little snippets where it uses two
> memory locations to stash debug information when doing PCI
> hotplug, such as:

Thanks.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

to qemu-xen-traditional master.

I think this ought to be backported in due course so I have put it on
my list.

Ian.

Re: [PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work.

[Konrad Rzeszutek Wilk]

On Tue, Nov 12, 2013 at 03:42:18PM +0000, Ian Jackson wrote:
> Konrad Rzeszutek Wilk writes ("[PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work."):
> > The ACPI AML code has little snippets where it uses two
> > memory locations to stash debug information when doing PCI
> > hotplug, such as:
> 
> Thanks.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Yeey!
> 
> to qemu-xen-traditional master.

It also applies to the qemu-xen but I forgot to CC Stefano. Doing it
here.

> 
> I think this ought to be backported in due course so I have put it on
> my list.

Much appreciated.
> 
> Ian.

Re: [PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work.

[Stefano Stabellini]

On Tue, 12 Nov 2013, Konrad Rzeszutek Wilk wrote:
> On Tue, Nov 12, 2013 at 03:42:18PM +0000, Ian Jackson wrote:
> > Konrad Rzeszutek Wilk writes ("[PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work."):
> > > The ACPI AML code has little snippets where it uses two
> > > memory locations to stash debug information when doing PCI
> > > hotplug, such as:
> > 
> > Thanks.
> > 
> > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Yeey!
> > 
> > to qemu-xen-traditional master.
> 
> It also applies to the qemu-xen but I forgot to CC Stefano. Doing it
> here.

Unfortunately the code in upstream qemu is very different in this area,
and it looks like ACPI_DBG_IO_ADDR is actually unused.

Re: [PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work.

[Konrad Rzeszutek Wilk]

On Wed, Nov 13, 2013 at 06:03:44PM +0000, Stefano Stabellini wrote:
> On Tue, 12 Nov 2013, Konrad Rzeszutek Wilk wrote:
> > On Tue, Nov 12, 2013 at 03:42:18PM +0000, Ian Jackson wrote:
> > > Konrad Rzeszutek Wilk writes ("[PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work."):
> > > > The ACPI AML code has little snippets where it uses two
> > > > memory locations to stash debug information when doing PCI
> > > > hotplug, such as:
> > > 
> > > Thanks.
> > > 
> > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > > Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > 
> > Yeey!
> > > 
> > > to qemu-xen-traditional master.
> > 
> > It also applies to the qemu-xen but I forgot to CC Stefano. Doing it
> > here.
> 
> Unfortunately the code in upstream qemu is very different in this area,
> and it looks like ACPI_DBG_IO_ADDR is actually unused.

Correct. Not for upstream qemu but for qemu-xen. You are the maintainer
of that tree too right?

Re: [PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work.

[Stefano Stabellini]

On Wed, 13 Nov 2013, Konrad Rzeszutek Wilk wrote:
> On Wed, Nov 13, 2013 at 06:03:44PM +0000, Stefano Stabellini wrote:
> > On Tue, 12 Nov 2013, Konrad Rzeszutek Wilk wrote:
> > > On Tue, Nov 12, 2013 at 03:42:18PM +0000, Ian Jackson wrote:
> > > > Konrad Rzeszutek Wilk writes ("[PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work."):
> > > > > The ACPI AML code has little snippets where it uses two
> > > > > memory locations to stash debug information when doing PCI
> > > > > hotplug, such as:
> > > > 
> > > > Thanks.
> > > > 
> > > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > > > Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > > 
> > > Yeey!
> > > > 
> > > > to qemu-xen-traditional master.
> > > 
> > > It also applies to the qemu-xen but I forgot to CC Stefano. Doing it
> > > here.
> > 
> > Unfortunately the code in upstream qemu is very different in this area,
> > and it looks like ACPI_DBG_IO_ADDR is actually unused.
> 
> Correct. Not for upstream qemu but for qemu-xen. You are the maintainer
> of that tree too right?

Right, the situation is the same in qemu-xen as in upstream QEMU.

Re: [PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work.

[Konrad Rzeszutek Wilk]

On Wed, Nov 13, 2013 at 06:43:16PM +0000, Stefano Stabellini wrote:
> On Wed, 13 Nov 2013, Konrad Rzeszutek Wilk wrote:
> > On Wed, Nov 13, 2013 at 06:03:44PM +0000, Stefano Stabellini wrote:
> > > On Tue, 12 Nov 2013, Konrad Rzeszutek Wilk wrote:
> > > > On Tue, Nov 12, 2013 at 03:42:18PM +0000, Ian Jackson wrote:
> > > > > Konrad Rzeszutek Wilk writes ("[PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work."):
> > > > > > The ACPI AML code has little snippets where it uses two
> > > > > > memory locations to stash debug information when doing PCI
> > > > > > hotplug, such as:
> > > > > 
> > > > > Thanks.
> > > > > 
> > > > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > > > > Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > > > 
> > > > Yeey!
> > > > > 
> > > > > to qemu-xen-traditional master.
> > > > 
> > > > It also applies to the qemu-xen but I forgot to CC Stefano. Doing it
> > > > here.
> > > 
> > > Unfortunately the code in upstream qemu is very different in this area,
> > > and it looks like ACPI_DBG_IO_ADDR is actually unused.
> > 
> > Correct. Not for upstream qemu but for qemu-xen. You are the maintainer
> > of that tree too right?
> 
> Right, the situation is the same in qemu-xen as in upstream QEMU.

I see that the qemu-xen has changed the name of the file, so its
acpi_piix4.c instead of piix4_acpi.c.

This seems to apply to my local xen-unstable tree:

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 059b64f..4ad9043 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -197,7 +197,7 @@ static void apm_ctrl_changed(uint32_t val, void *arg)
     }
 }
 
-static void acpi_dbg_writel(void *opaque, uint32_t addr, uint32_t val)
+static void acpi_dbg_writeb(void *opaque, uint32_t addr, uint32_t val)
 {
     PIIX4_DPRINTF("ACPI: DBG: 0x%08x\n", val);
 }
@@ -453,7 +453,7 @@ static int piix4_pm_initfn(PCIDevice *dev)
     /* APM */
     apm_init(&s->apm, apm_ctrl_changed, s);
 
-    register_ioport_write(ACPI_DBG_IO_ADDR, 4, 4, acpi_dbg_writel, s);
+    register_ioport_write(ACPI_DBG_IO_ADDR, 4, 1, acpi_dbg_writeb, s);
 
     if (s->kvm_enabled) {
         /* Mark SMM as already inited to prevent SMM from running.  KVM does not


Would you like me to repost it with the right file and the same
commit description?

Thank you.

Re: [PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work.

[Stefano Stabellini]

On Wed, 13 Nov 2013, Konrad Rzeszutek Wilk wrote:
> On Wed, Nov 13, 2013 at 06:43:16PM +0000, Stefano Stabellini wrote:
> > On Wed, 13 Nov 2013, Konrad Rzeszutek Wilk wrote:
> > > On Wed, Nov 13, 2013 at 06:03:44PM +0000, Stefano Stabellini wrote:
> > > > On Tue, 12 Nov 2013, Konrad Rzeszutek Wilk wrote:
> > > > > On Tue, Nov 12, 2013 at 03:42:18PM +0000, Ian Jackson wrote:
> > > > > > Konrad Rzeszutek Wilk writes ("[PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work."):
> > > > > > > The ACPI AML code has little snippets where it uses two
> > > > > > > memory locations to stash debug information when doing PCI
> > > > > > > hotplug, such as:
> > > > > > 
> > > > > > Thanks.
> > > > > > 
> > > > > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > > > > > Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > > > > 
> > > > > Yeey!
> > > > > > 
> > > > > > to qemu-xen-traditional master.
> > > > > 
> > > > > It also applies to the qemu-xen but I forgot to CC Stefano. Doing it
> > > > > here.
> > > > 
> > > > Unfortunately the code in upstream qemu is very different in this area,
> > > > and it looks like ACPI_DBG_IO_ADDR is actually unused.
> > > 
> > > Correct. Not for upstream qemu but for qemu-xen. You are the maintainer
> > > of that tree too right?
> > 
> > Right, the situation is the same in qemu-xen as in upstream QEMU.
> 
> I see that the qemu-xen has changed the name of the file, so its
> acpi_piix4.c instead of piix4_acpi.c.
> 
> This seems to apply to my local xen-unstable tree:
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 059b64f..4ad9043 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -197,7 +197,7 @@ static void apm_ctrl_changed(uint32_t val, void *arg)
>      }
>  }
>  
> -static void acpi_dbg_writel(void *opaque, uint32_t addr, uint32_t val)
> +static void acpi_dbg_writeb(void *opaque, uint32_t addr, uint32_t val)
>  {
>      PIIX4_DPRINTF("ACPI: DBG: 0x%08x\n", val);
>  }
> @@ -453,7 +453,7 @@ static int piix4_pm_initfn(PCIDevice *dev)
>      /* APM */
>      apm_init(&s->apm, apm_ctrl_changed, s);
>  
> -    register_ioport_write(ACPI_DBG_IO_ADDR, 4, 4, acpi_dbg_writel, s);
> +    register_ioport_write(ACPI_DBG_IO_ADDR, 4, 1, acpi_dbg_writeb, s);
>  
>      if (s->kvm_enabled) {
>          /* Mark SMM as already inited to prevent SMM from running.  KVM does not
> 
> 
> Would you like me to repost it with the right file and the same
> commit description?

I think we are not looking at the same repository: qemu-xen has been
blocked on failing OSSTests since the rebase. Anthony is trying to fix
the issue that has something to do with dirty bit tracking on EPT.

Meanwhile please use the staging repository:

git://xenbits.xensource.com/staging/qemu-upstream-unstable.git

Re: [PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work.

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work."):
> Konrad Rzeszutek Wilk writes ("[PATCH] hw/piix4acpi: Make writes to ACPI_DBG_IO_ADDR actually work."):
> > The ACPI AML code has little snippets where it uses two
> > memory locations to stash debug information when doing PCI
> > hotplug, such as:
> 
> Thanks.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> to qemu-xen-traditional master.
> 
> I think this ought to be backported in due course so I have put it on
> my list.

Now applied to qemu-xen-traditional for Xen 4.3, 4.2, 4.1.

(I know 4.1 is EOL but it was 10 seconds' work to git-cherry-pick and
run my push script.)

Ian.