[Qemu-devel] [PATCH master/stable-1.0] pci: fix corrupted pci conf index register by unaligned write

[Qemu-devel] [PATCH master/stable-1.0] pci: fix corrupted pci conf index register by unaligned write

[Avi Kivity]

Commit d0ed8076cbdc261 converted the PCI config access to the memory
API, but also inadvertantly changed it to accept unaligned writes,
and corrupt the index register in the process.  This causes a regression
booting NetBSD.

Fix by ignoring unaligned or non-dword writes.

https://bugs.launchpad.net/qemu/+bug/897771

Reported-by: Andreas Gustafsson <gson@gson.org>
Signed-off-by: Avi Kivity <avi@redhat.com>
---

 hw/pci_host.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/pci_host.c b/hw/pci_host.c
index 44c6c20..8041778 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -101,6 +101,9 @@ static void pci_host_config_write(void *opaque, target_phys_addr_t addr,
 
     PCI_DPRINTF("%s addr " TARGET_FMT_plx " len %d val %"PRIx64"\n",
                 __func__, addr, len, val);
+    if (addr != 0 || len != 4) {
+        return;
+    }
     s->config_reg = val;
 }
 
-- 
1.7.7.1

Re: [Qemu-devel] [PATCH master/stable-1.0] pci: fix corrupted pci conf index register by unaligned write

[Michael S. Tsirkin]

On Wed, Jan 04, 2012 at 04:28:42PM +0200, Avi Kivity wrote:
> Commit d0ed8076cbdc261 converted the PCI config access to the memory
> API, but also inadvertantly changed it to accept unaligned writes,
> and corrupt the index register in the process.  This causes a regression
> booting NetBSD.
> 
> Fix by ignoring unaligned or non-dword writes.
> 
> https://bugs.launchpad.net/qemu/+bug/897771
> 
> Reported-by: Andreas Gustafsson <gson@gson.org>
> Signed-off-by: Avi Kivity <avi@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
> 
>  hw/pci_host.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index 44c6c20..8041778 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -101,6 +101,9 @@ static void pci_host_config_write(void *opaque, target_phys_addr_t addr,
>  
>      PCI_DPRINTF("%s addr " TARGET_FMT_plx " len %d val %"PRIx64"\n",
>                  __func__, addr, len, val);
> +    if (addr != 0 || len != 4) {
> +        return;
> +    }
>      s->config_reg = val;
>  }
>  
> -- 
> 1.7.7.1

Re: [Qemu-devel] [PATCH master/stable-1.0] pci: fix corrupted pci conf index register by unaligned write

[Alexander Graf]

On 04.01.2012, at 15:47, Michael S. Tsirkin wrote:

> On Wed, Jan 04, 2012 at 04:28:42PM +0200, Avi Kivity wrote:
>> Commit d0ed8076cbdc261 converted the PCI config access to the memory
>> API, but also inadvertantly changed it to accept unaligned writes,
>> and corrupt the index register in the process.  This causes a regression
>> booting NetBSD.
>> 
>> Fix by ignoring unaligned or non-dword writes.
>> 
>> https://bugs.launchpad.net/qemu/+bug/897771
>> 
>> Reported-by: Andreas Gustafsson <gson@gson.org>
>> Signed-off-by: Avi Kivity <avi@redhat.com>
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

CC'ing qemu-stable.


Alex

> 
>> ---
>> 
>> hw/pci_host.c |    3 +++
>> 1 files changed, 3 insertions(+), 0 deletions(-)
>> 
>> diff --git a/hw/pci_host.c b/hw/pci_host.c
>> index 44c6c20..8041778 100644
>> --- a/hw/pci_host.c
>> +++ b/hw/pci_host.c
>> @@ -101,6 +101,9 @@ static void pci_host_config_write(void *opaque, target_phys_addr_t addr,
>> 
>>     PCI_DPRINTF("%s addr " TARGET_FMT_plx " len %d val %"PRIx64"\n",
>>                 __func__, addr, len, val);
>> +    if (addr != 0 || len != 4) {
>> +        return;
>> +    }
>>     s->config_reg = val;
>> }
>> 
>> -- 
>> 1.7.7.1
> 

Re: [Qemu-devel] [PATCH master/stable-1.0] pci: fix corrupted pci conf index register by unaligned write

[Stefan Weil]

Am 04.01.2012 15:47, schrieb Michael S. Tsirkin:
> On Wed, Jan 04, 2012 at 04:28:42PM +0200, Avi Kivity wrote:
>> Commit d0ed8076cbdc261 converted the PCI config access to the memory
>> API, but also inadvertantly changed it to accept unaligned writes,
>> and corrupt the index register in the process. This causes a regression
>> booting NetBSD.
>>
>> Fix by ignoring unaligned or non-dword writes.
>>
>> https://bugs.launchpad.net/qemu/+bug/897771
>>
>> Reported-by: Andreas Gustafsson <gson@gson.org>
>> Signed-off-by: Avi Kivity <avi@redhat.com>
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
>> ---
>>
>> hw/pci_host.c | 3 +++
>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/pci_host.c b/hw/pci_host.c
>> index 44c6c20..8041778 100644
>> --- a/hw/pci_host.c
>> +++ b/hw/pci_host.c
>> @@ -101,6 +101,9 @@ static void pci_host_config_write(void *opaque, 
>> target_phys_addr_t addr,
>>
>> PCI_DPRINTF("%s addr " TARGET_FMT_plx " len %d val %"PRIx64"\n",
>> __func__, addr, len, val);
>> + if (addr != 0 || len != 4) {
>> + return;
>> + }
>> s->config_reg = val;
>> }
>>
>> -- 
>> 1.7.7.1

Non dword writes are quite common. I get them with Linux kernels, too.
Do you really want to ignore them?

And the check for unaligned writes is, well, unusual :-)

Regards,
Stefan

Re: [Qemu-devel] [PATCH master/stable-1.0] pci: fix corrupted pci conf index register by unaligned write

[Michael S. Tsirkin]

On Thu, Jan 05, 2012 at 04:14:29PM +0100, Stefan Weil wrote:
> Am 04.01.2012 15:47, schrieb Michael S. Tsirkin:
> >On Wed, Jan 04, 2012 at 04:28:42PM +0200, Avi Kivity wrote:
> >>Commit d0ed8076cbdc261 converted the PCI config access to the memory
> >>API, but also inadvertantly changed it to accept unaligned writes,
> >>and corrupt the index register in the process. This causes a regression
> >>booting NetBSD.
> >>
> >>Fix by ignoring unaligned or non-dword writes.
> >>
> >>https://bugs.launchpad.net/qemu/+bug/897771
> >>
> >>Reported-by: Andreas Gustafsson <gson@gson.org>
> >>Signed-off-by: Avi Kivity <avi@redhat.com>
> >
> >Acked-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >>---
> >>
> >>hw/pci_host.c | 3 +++
> >>1 files changed, 3 insertions(+), 0 deletions(-)
> >>
> >>diff --git a/hw/pci_host.c b/hw/pci_host.c
> >>index 44c6c20..8041778 100644
> >>--- a/hw/pci_host.c
> >>+++ b/hw/pci_host.c
> >>@@ -101,6 +101,9 @@ static void pci_host_config_write(void
> >>*opaque, target_phys_addr_t addr,
> >>
> >>PCI_DPRINTF("%s addr " TARGET_FMT_plx " len %d val %"PRIx64"\n",
> >>__func__, addr, len, val);
> >>+ if (addr != 0 || len != 4) {
> >>+ return;
> >>+ }
> >>s->config_reg = val;
> >>}
> >>
> >>-- 
> >>1.7.7.1
> 
> Non dword writes are quite common. I get them with Linux kernels, too.
> Do you really want to ignore them?

Are you sure?
Note this is an io write at cf8. Not an unaligned config write.

> And the check for unaligned writes is, well, unusual :-)

This seems to be how memory API behaves ... right, Avi?
Maybe this should be documented somewhere.

> Regards,
> Stefan

Re: [Qemu-devel] [PATCH master/stable-1.0] pci: fix corrupted pci conf index register by unaligned write

[Avi Kivity]

On 01/08/2012 11:17 AM, Michael S. Tsirkin wrote:
> On Thu, Jan 05, 2012 at 04:14:29PM +0100, Stefan Weil wrote:
> > Am 04.01.2012 15:47, schrieb Michael S. Tsirkin:
> > >On Wed, Jan 04, 2012 at 04:28:42PM +0200, Avi Kivity wrote:
> > >>Commit d0ed8076cbdc261 converted the PCI config access to the memory
> > >>API, but also inadvertantly changed it to accept unaligned writes,
> > >>and corrupt the index register in the process. This causes a regression
> > >>booting NetBSD.
> > >>
> > >>Fix by ignoring unaligned or non-dword writes.
> > >>
> > >>https://bugs.launchpad.net/qemu/+bug/897771
> > >>
> > >>Reported-by: Andreas Gustafsson <gson@gson.org>
> > >>Signed-off-by: Avi Kivity <avi@redhat.com>
> > >
> > >Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > >>---
> > >>
> > >>hw/pci_host.c | 3 +++
> > >>1 files changed, 3 insertions(+), 0 deletions(-)
> > >>
> > >>diff --git a/hw/pci_host.c b/hw/pci_host.c
> > >>index 44c6c20..8041778 100644
> > >>--- a/hw/pci_host.c
> > >>+++ b/hw/pci_host.c
> > >>@@ -101,6 +101,9 @@ static void pci_host_config_write(void
> > >>*opaque, target_phys_addr_t addr,
> > >>
> > >>PCI_DPRINTF("%s addr " TARGET_FMT_plx " len %d val %"PRIx64"\n",
> > >>__func__, addr, len, val);
> > >>+ if (addr != 0 || len != 4) {
> > >>+ return;
> > >>+ }
> > >>s->config_reg = val;
> > >>}
> > >>
> > >>-- 
> > >>1.7.7.1
> > 
> > Non dword writes are quite common. I get them with Linux kernels, too.
> > Do you really want to ignore them?
>
> Are you sure?
> Note this is an io write at cf8. Not an unaligned config write.
>
> > And the check for unaligned writes is, well, unusual :-)

What's unusual?


> This seems to be how memory API behaves ... right, Avi?
> Maybe this should be documented somewhere.

Document what?

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH master/stable-1.0] pci: fix corrupted pci conf index register by unaligned write

[Michael S. Tsirkin]

On Sun, Jan 08, 2012 at 12:02:35PM +0200, Avi Kivity wrote:
> On 01/08/2012 11:17 AM, Michael S. Tsirkin wrote:
> > On Thu, Jan 05, 2012 at 04:14:29PM +0100, Stefan Weil wrote:
> > > Am 04.01.2012 15:47, schrieb Michael S. Tsirkin:
> > > >On Wed, Jan 04, 2012 at 04:28:42PM +0200, Avi Kivity wrote:
> > > >>Commit d0ed8076cbdc261 converted the PCI config access to the memory
> > > >>API, but also inadvertantly changed it to accept unaligned writes,
> > > >>and corrupt the index register in the process. This causes a regression
> > > >>booting NetBSD.
> > > >>
> > > >>Fix by ignoring unaligned or non-dword writes.
> > > >>
> > > >>https://bugs.launchpad.net/qemu/+bug/897771
> > > >>
> > > >>Reported-by: Andreas Gustafsson <gson@gson.org>
> > > >>Signed-off-by: Avi Kivity <avi@redhat.com>
> > > >
> > > >Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > > >
> > > >>---
> > > >>
> > > >>hw/pci_host.c | 3 +++
> > > >>1 files changed, 3 insertions(+), 0 deletions(-)
> > > >>
> > > >>diff --git a/hw/pci_host.c b/hw/pci_host.c
> > > >>index 44c6c20..8041778 100644
> > > >>--- a/hw/pci_host.c
> > > >>+++ b/hw/pci_host.c
> > > >>@@ -101,6 +101,9 @@ static void pci_host_config_write(void
> > > >>*opaque, target_phys_addr_t addr,
> > > >>
> > > >>PCI_DPRINTF("%s addr " TARGET_FMT_plx " len %d val %"PRIx64"\n",
> > > >>__func__, addr, len, val);
> > > >>+ if (addr != 0 || len != 4) {
> > > >>+ return;
> > > >>+ }
> > > >>s->config_reg = val;
> > > >>}
> > > >>
> > > >>-- 
> > > >>1.7.7.1
> > > 
> > > Non dword writes are quite common. I get them with Linux kernels, too.
> > > Do you really want to ignore them?
> >
> > Are you sure?
> > Note this is an io write at cf8. Not an unaligned config write.
> >
> > > And the check for unaligned writes is, well, unusual :-)
> 
> What's unusual?
> 
> 
> > This seems to be how memory API behaves ... right, Avi?
> > Maybe this should be documented somewhere.
> 
> Document what?

That address passed to callbacks is in fact an offset
from start of the region.

> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH master/stable-1.0] pci: fix corrupted pci conf index register by unaligned write

[Avi Kivity]

On 01/08/2012 12:12 PM, Michael S. Tsirkin wrote:
> > >
> > > > And the check for unaligned writes is, well, unusual :-)
> > 
> > What's unusual?
> > 
> > 
> > > This seems to be how memory API behaves ... right, Avi?
> > > Maybe this should be documented somewhere.
> > 
> > Document what?
>
> That address passed to callbacks is in fact an offset
> from start of the region.
>

It's documented already (and it's not new - since 8da3ff18097, Dec 2008).

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH master/stable-1.0] pci: fix corrupted pci conf index register by unaligned write

[Michael S. Tsirkin]

On Sun, Jan 08, 2012 at 12:48:05PM +0200, Avi Kivity wrote:
> On 01/08/2012 12:12 PM, Michael S. Tsirkin wrote:
> > > >
> > > > > And the check for unaligned writes is, well, unusual :-)
> > > 
> > > What's unusual?
> > > 
> > > 
> > > > This seems to be how memory API behaves ... right, Avi?
> > > > Maybe this should be documented somewhere.
> > > 
> > > Document what?
> >
> > That address passed to callbacks is in fact an offset
> > from start of the region.
> >
> 
> It's documented already (and it's not new - since 8da3ff18097, Dec 2008).

True, memory.h explicitly says:
@addr is relative to @mr

But now that's mentioned, I have a question:
we could also specify min access size and disable
unaligned access in ops.valid structure.
That would trigger machine checks on access errors
instead of ignoring them as we do now.
Anyone knows offhand what does real hardware do?

> -- 
> error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH master/stable-1.0] pci: fix corrupted pci conf index register by unaligned write

[Avi Kivity]

On 01/08/2012 02:45 PM, Michael S. Tsirkin wrote:
> On Sun, Jan 08, 2012 at 12:48:05PM +0200, Avi Kivity wrote:
> > On 01/08/2012 12:12 PM, Michael S. Tsirkin wrote:
> > > > >
> > > > > > And the check for unaligned writes is, well, unusual :-)
> > > > 
> > > > What's unusual?
> > > > 
> > > > 
> > > > > This seems to be how memory API behaves ... right, Avi?
> > > > > Maybe this should be documented somewhere.
> > > > 
> > > > Document what?
> > >
> > > That address passed to callbacks is in fact an offset
> > > from start of the region.
> > >
> > 
> > It's documented already (and it's not new - since 8da3ff18097, Dec 2008).
>
> True, memory.h explicitly says:
> @addr is relative to @mr
>
> But now that's mentioned, I have a question:
> we could also specify min access size and disable
> unaligned access in ops.valid structure.
> That would trigger machine checks on access errors
> instead of ignoring them as we do now.
> Anyone knows offhand what does real hardware do?

It depends on the actual hardware.  qemu has some #ifdefs deep down that
control this.

Ultimately this should be local to the bus rather than global; for
example a bus can intercept unaligned writes and invoke some bus
specific behaviour even though the cpu allows those accesses.

-- 
error compiling committee.c: too many arguments to function