[Qemu-devel] [PATCH] pci: w1cmask[PCI_BRIDGE_CONTROL] initialized incorrectly

[Qemu-devel] [PATCH] pci: w1cmask[PCI_BRIDGE_CONTROL] initialized incorrectly

[Isaku Yamahata]

pci_init_wmask_bridge() incorrectly set w1cmask[PCI_BRIDGE_CONTROL].
This patch removes the line otherwise the assert(!(wmask & w1cmask)) in
pci_default_write_config() is hit.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index b8f5385..79a46e7 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -643,10 +643,6 @@ static void pci_init_wmask_bridge(PCIDevice *d)
                  PCI_BRIDGE_CTL_SEC_DISCARD |
                  PCI_BRIDGE_CTL_DISCARD_STATUS |
                  PCI_BRIDGE_CTL_DISCARD_SERR);
-    /* Below does not do anything as we never set this bit, put here for
-     * completeness. */
-    pci_set_word(d->w1cmask + PCI_BRIDGE_CONTROL,
-                 PCI_BRIDGE_CTL_DISCARD_STATUS);
 }
 
 static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
-- 
1.7.1.1

[Qemu-devel] Re: [PATCH] pci: w1cmask[PCI_BRIDGE_CONTROL] initialized incorrectly

[Michael S. Tsirkin]

On Wed, Jan 26, 2011 at 06:45:27PM +0900, Isaku Yamahata wrote:
> pci_init_wmask_bridge() incorrectly set w1cmask[PCI_BRIDGE_CONTROL].
> This patch removes the line otherwise the assert(!(wmask & w1cmask)) in
> pci_default_write_config() is hit.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

Maybe clear in wmask? This bit really should be w1c, should it not?

> ---
>  hw/pci.c |    4 ----
>  1 files changed, 0 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index b8f5385..79a46e7 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -643,10 +643,6 @@ static void pci_init_wmask_bridge(PCIDevice *d)
>                   PCI_BRIDGE_CTL_SEC_DISCARD |
>                   PCI_BRIDGE_CTL_DISCARD_STATUS |
>                   PCI_BRIDGE_CTL_DISCARD_SERR);
> -    /* Below does not do anything as we never set this bit, put here for
> -     * completeness. */
> -    pci_set_word(d->w1cmask + PCI_BRIDGE_CONTROL,
> -                 PCI_BRIDGE_CTL_DISCARD_STATUS);
>  }
>  
>  static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
> -- 
> 1.7.1.1

[Qemu-devel] Re: [PATCH] pci: w1cmask[PCI_BRIDGE_CONTROL] initialized incorrectly

[Isaku Yamahata]

The bit should be writable, not w1c.

3.2.5.18 bridge control register
bit 11 Discard Timer SERR# Enable

  When set to 1, this bit enables the bridge to assert SERR# on
  the primary interface when either the Primary Discard Timer or
  Secondary Discard Timer expires and a Delayed Transaction is
  discarded from a queue in the bridge. The default state of this
  bit must be 0 after reset.
  0 - do not assert SERR# on the primary interface as
  a result of the expiration of either the Primary
  Discard Timer or Secondary Discard Timer
  1 - assert SERR# on the primary interface if either
  the Primary Discard Timer or Secondary Discard
  Timer expires and a Delayed Transaction


On Wed, Jan 26, 2011 at 02:09:59PM +0200, Michael S. Tsirkin wrote:
> On Wed, Jan 26, 2011 at 06:45:27PM +0900, Isaku Yamahata wrote:
> > pci_init_wmask_bridge() incorrectly set w1cmask[PCI_BRIDGE_CONTROL].
> > This patch removes the line otherwise the assert(!(wmask & w1cmask)) in
> > pci_default_write_config() is hit.
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> Maybe clear in wmask? This bit really should be w1c, should it not?
> 
> > ---
> >  hw/pci.c |    4 ----
> >  1 files changed, 0 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index b8f5385..79a46e7 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -643,10 +643,6 @@ static void pci_init_wmask_bridge(PCIDevice *d)
> >                   PCI_BRIDGE_CTL_SEC_DISCARD |
> >                   PCI_BRIDGE_CTL_DISCARD_STATUS |
> >                   PCI_BRIDGE_CTL_DISCARD_SERR);
> > -    /* Below does not do anything as we never set this bit, put here for
> > -     * completeness. */
> > -    pci_set_word(d->w1cmask + PCI_BRIDGE_CONTROL,
> > -                 PCI_BRIDGE_CTL_DISCARD_STATUS);
> >  }
> >  
> >  static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
> > -- 
> > 1.7.1.1
> 

-- 
yamahata

[Qemu-devel] Re: [PATCH] pci: w1cmask[PCI_BRIDGE_CONTROL] initialized incorrectly

[Michael S. Tsirkin]

On Wed, Jan 26, 2011 at 10:17:48PM +0900, Isaku Yamahata wrote:
> The bit should be writable, not w1c.
> 
> 3.2.5.18 bridge control register
> bit 11 Discard Timer SERR# Enable
> 
>   When set to 1, this bit enables the bridge to assert SERR# on
>   the primary interface when either the Primary Discard Timer or
>   Secondary Discard Timer expires and a Delayed Transaction is
>   discarded from a queue in the bridge. The default state of this
>   bit must be 0 after reset.
>   0 - do not assert SERR# on the primary interface as
>   a result of the expiration of either the Primary
>   Discard Timer or Secondary Discard Timer
>   1 - assert SERR# on the primary interface if either
>   the Primary Discard Timer or Secondary Discard
>   Timer expires and a Delayed Transaction

Yes but
#define  PCI_BRIDGE_CTL_DISCARD_STATUS 0x400   /* Discard timer status */

So this is bit 10?

BTW, how about moving these bits to linux and then to our pci_regs.h?

>S. Tsirkin wrote:
> > On Wed, Jan 26, 2011 at 06:45:27PM +0900, Isaku Yamahata wrote:
> > > pci_init_wmask_bridge() incorrectly set w1cmask[PCI_BRIDGE_CONTROL].
> > > This patch removes the line otherwise the assert(!(wmask & w1cmask)) in
> > > pci_default_write_config() is hit.
> > > 
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > 
> > Maybe clear in wmask? This bit really should be w1c, should it not?
> > 
> > > ---
> > >  hw/pci.c |    4 ----
> > >  1 files changed, 0 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/pci.c b/hw/pci.c
> > > index b8f5385..79a46e7 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -643,10 +643,6 @@ static void pci_init_wmask_bridge(PCIDevice *d)
> > >                   PCI_BRIDGE_CTL_SEC_DISCARD |
> > >                   PCI_BRIDGE_CTL_DISCARD_STATUS |
> > >                   PCI_BRIDGE_CTL_DISCARD_SERR);
> > > -    /* Below does not do anything as we never set this bit, put here for
> > > -     * completeness. */
> > > -    pci_set_word(d->w1cmask + PCI_BRIDGE_CONTROL,
> > > -                 PCI_BRIDGE_CTL_DISCARD_STATUS);
> > >  }
> > >  
> > >  static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
> > > -- 
> > > 1.7.1.1
> > 
> 
> -- 
> yamahata

[Qemu-devel] Re: [PATCH] pci: w1cmask[PCI_BRIDGE_CONTROL] initialized incorrectly

[Isaku Yamahata]

On Wed, Jan 26, 2011 at 03:46:01PM +0200, Michael S. Tsirkin wrote:
> On Wed, Jan 26, 2011 at 10:17:48PM +0900, Isaku Yamahata wrote:
> > The bit should be writable, not w1c.
> > 
> > 3.2.5.18 bridge control register
> > bit 11 Discard Timer SERR# Enable
> > 
> >   When set to 1, this bit enables the bridge to assert SERR# on
> >   the primary interface when either the Primary Discard Timer or
> >   Secondary Discard Timer expires and a Delayed Transaction is
> >   discarded from a queue in the bridge. The default state of this
> >   bit must be 0 after reset.
> >   0 - do not assert SERR# on the primary interface as
> >   a result of the expiration of either the Primary
> >   Discard Timer or Secondary Discard Timer
> >   1 - assert SERR# on the primary interface if either
> >   the Primary Discard Timer or Secondary Discard
> >   Timer expires and a Delayed Transaction
> 
> Yes but
> #define  PCI_BRIDGE_CTL_DISCARD_STATUS 0x400   /* Discard timer status */
> 
> So this is bit 10?

Oh, sorry. So wmask should be chenged.


> BTW, how about moving these bits to linux and then to our pci_regs.h?
> 
> >S. Tsirkin wrote:
> > > On Wed, Jan 26, 2011 at 06:45:27PM +0900, Isaku Yamahata wrote:
> > > > pci_init_wmask_bridge() incorrectly set w1cmask[PCI_BRIDGE_CONTROL].
> > > > This patch removes the line otherwise the assert(!(wmask & w1cmask)) in
> > > > pci_default_write_config() is hit.
> > > > 
> > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > 
> > > Maybe clear in wmask? This bit really should be w1c, should it not?
> > > 
> > > > ---
> > > >  hw/pci.c |    4 ----
> > > >  1 files changed, 0 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/hw/pci.c b/hw/pci.c
> > > > index b8f5385..79a46e7 100644
> > > > --- a/hw/pci.c
> > > > +++ b/hw/pci.c
> > > > @@ -643,10 +643,6 @@ static void pci_init_wmask_bridge(PCIDevice *d)
> > > >                   PCI_BRIDGE_CTL_SEC_DISCARD |
> > > >                   PCI_BRIDGE_CTL_DISCARD_STATUS |
> > > >                   PCI_BRIDGE_CTL_DISCARD_SERR);
> > > > -    /* Below does not do anything as we never set this bit, put here for
> > > > -     * completeness. */
> > > > -    pci_set_word(d->w1cmask + PCI_BRIDGE_CONTROL,
> > > > -                 PCI_BRIDGE_CTL_DISCARD_STATUS);
> > > >  }
> > > >  
> > > >  static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
> > > > -- 
> > > > 1.7.1.1
> > > 
> > 
> > -- 
> > yamahata
> 

-- 
yamahata

[Qemu-devel] Re: [PATCH] pci: w1cmask[PCI_BRIDGE_CONTROL] initialized incorrectly

[Michael S. Tsirkin]

On Wed, Jan 26, 2011 at 10:53:42PM +0900, Isaku Yamahata wrote:
> On Wed, Jan 26, 2011 at 03:46:01PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Jan 26, 2011 at 10:17:48PM +0900, Isaku Yamahata wrote:
> > > The bit should be writable, not w1c.
> > > 
> > > 3.2.5.18 bridge control register
> > > bit 11 Discard Timer SERR# Enable
> > > 
> > >   When set to 1, this bit enables the bridge to assert SERR# on
> > >   the primary interface when either the Primary Discard Timer or
> > >   Secondary Discard Timer expires and a Delayed Transaction is
> > >   discarded from a queue in the bridge. The default state of this
> > >   bit must be 0 after reset.
> > >   0 - do not assert SERR# on the primary interface as
> > >   a result of the expiration of either the Primary
> > >   Discard Timer or Secondary Discard Timer
> > >   1 - assert SERR# on the primary interface if either
> > >   the Primary Discard Timer or Secondary Discard
> > >   Timer expires and a Delayed Transaction
> > 
> > Yes but
> > #define  PCI_BRIDGE_CTL_DISCARD_STATUS 0x400   /* Discard timer status */
> > 
> > So this is bit 10?
> 
> Oh, sorry. So wmask should be chenged.

Yes. does the below work?

commit 24ebb78e6ec19b39cd138d493a2859122110f9cb
Author: Michael S. Tsirkin <mst@redhat.com>
Date:   Wed Jan 26 15:55:07 2011 +0200

    pci: bridge control fixup
    
    PCI_BRIDGE_CTL_DISCARD_STATUS (bit 10 in bridge control register)
    is W1C so we should not make it writeable, otherwise the assert(!(wmask
    & w1cmask)) in pci_default_write_config() is hit
    
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
    Reported-by: Isaku Yamahata <yamahata@valinux.co.jp>

diff --git a/hw/pci.c b/hw/pci.c
index 044c4bd..712280a 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -641,7 +641,6 @@ static void pci_init_wmask_bridge(PCIDevice *d)
                  PCI_BRIDGE_CTL_FAST_BACK |
                  PCI_BRIDGE_CTL_DISCARD |
                  PCI_BRIDGE_CTL_SEC_DISCARD |
-                 PCI_BRIDGE_CTL_DISCARD_STATUS |
                  PCI_BRIDGE_CTL_DISCARD_SERR);
     /* Below does not do anything as we never set this bit, put here for
      * completeness. */

[Qemu-devel] Re: [PATCH] pci: w1cmask[PCI_BRIDGE_CONTROL] initialized incorrectly

[Isaku Yamahata]

On Wed, Jan 26, 2011 at 03:57:15PM +0200, Michael S. Tsirkin wrote:
> On Wed, Jan 26, 2011 at 10:53:42PM +0900, Isaku Yamahata wrote:
> > On Wed, Jan 26, 2011 at 03:46:01PM +0200, Michael S. Tsirkin wrote:
> > > On Wed, Jan 26, 2011 at 10:17:48PM +0900, Isaku Yamahata wrote:
> > > > The bit should be writable, not w1c.
> > > > 
> > > > 3.2.5.18 bridge control register
> > > > bit 11 Discard Timer SERR# Enable
> > > > 
> > > >   When set to 1, this bit enables the bridge to assert SERR# on
> > > >   the primary interface when either the Primary Discard Timer or
> > > >   Secondary Discard Timer expires and a Delayed Transaction is
> > > >   discarded from a queue in the bridge. The default state of this
> > > >   bit must be 0 after reset.
> > > >   0 - do not assert SERR# on the primary interface as
> > > >   a result of the expiration of either the Primary
> > > >   Discard Timer or Secondary Discard Timer
> > > >   1 - assert SERR# on the primary interface if either
> > > >   the Primary Discard Timer or Secondary Discard
> > > >   Timer expires and a Delayed Transaction
> > > 
> > > Yes but
> > > #define  PCI_BRIDGE_CTL_DISCARD_STATUS 0x400   /* Discard timer status */
> > > 
> > > So this is bit 10?
> > 
> > Oh, sorry. So wmask should be chenged.
> 
> Yes. does the below work?

Yep. Thank you.
Tested-by: Isaku Yamahata <yamahata@valinux.co.jp>

> 
> commit 24ebb78e6ec19b39cd138d493a2859122110f9cb
> Author: Michael S. Tsirkin <mst@redhat.com>
> Date:   Wed Jan 26 15:55:07 2011 +0200
> 
>     pci: bridge control fixup
>     
>     PCI_BRIDGE_CTL_DISCARD_STATUS (bit 10 in bridge control register)
>     is W1C so we should not make it writeable, otherwise the assert(!(wmask
>     & w1cmask)) in pci_default_write_config() is hit
>     
>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>     Reported-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 044c4bd..712280a 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -641,7 +641,6 @@ static void pci_init_wmask_bridge(PCIDevice *d)
>                   PCI_BRIDGE_CTL_FAST_BACK |
>                   PCI_BRIDGE_CTL_DISCARD |
>                   PCI_BRIDGE_CTL_SEC_DISCARD |
> -                 PCI_BRIDGE_CTL_DISCARD_STATUS |
>                   PCI_BRIDGE_CTL_DISCARD_SERR);
>      /* Below does not do anything as we never set this bit, put here for
>       * completeness. */
> 

-- 
yamahata