[v4,05/15] pci/bridge: fix pci_bridge_reset()

lower 4bits of base/limit register is RO, and
should not be modified on reset.

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

On Mon, Oct 18, 2010 at 12:17:46PM +0900, Isaku Yamahata wrote:
> lower 4bits of base/limit register is RO, and
> should not be modified on reset.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci_bridge.c |   15 +++++++++------
>  1 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
> index 638e3b3..7229610 100644
> --- a/hw/pci_bridge.c
> +++ b/hw/pci_bridge.c
> @@ -161,12 +161,15 @@ void pci_bridge_reset_reg(PCIDevice *dev)
>      conf[PCI_SUBORDINATE_BUS] = 0;
>      conf[PCI_SEC_LATENCY_TIMER] = 0;
>  
> -    conf[PCI_IO_BASE] = 0;
> -    conf[PCI_IO_LIMIT] = 0;
> -    pci_set_word(conf + PCI_MEMORY_BASE, 0);
> -    pci_set_word(conf + PCI_MEMORY_LIMIT, 0);
> -    pci_set_word(conf + PCI_PREF_MEMORY_BASE, 0);
> -    pci_set_word(conf + PCI_PREF_MEMORY_LIMIT, 0);
> +    pci_clear_bit_byte(conf + PCI_IO_BASE, PCI_IO_RANGE_MASK & 0xff);
> +    pci_clear_bit_byte(conf + PCI_IO_LIMIT, PCI_IO_RANGE_MASK & 0xff);

No need for & 0xff and & 0xfffff here and below.

also, PCI spec also says that these registers' value
is undefined after reset, so no need to clear them?
If there's a reason, pls put it in comment.


> +    pci_clear_bit_word(conf + PCI_MEMORY_BASE, PCI_MEMORY_RANGE_MASK & 0xffff);
> +    pci_clear_bit_word(conf + PCI_MEMORY_LIMIT,
> +                       PCI_MEMORY_RANGE_MASK & 0xffff);
> +    pci_clear_bit_word(conf + PCI_PREF_MEMORY_BASE,
> +                       PCI_PREF_RANGE_MASK & 0xffff);
> +    pci_clear_bit_word(conf + PCI_PREF_MEMORY_LIMIT,
> +                       PCI_PREF_RANGE_MASK & 0xffff);
>      pci_set_word(conf + PCI_PREF_BASE_UPPER32, 0);
>      pci_set_word(conf + PCI_PREF_LIMIT_UPPER32, 0);
>  
> -- 
> 1.7.1.1

On Mon, Oct 18, 2010 at 04:10:17PM +0900, Isaku Yamahata wrote:
> On Mon, Oct 18, 2010 at 08:22:24AM +0200, Michael S. Tsirkin wrote:
> > On Mon, Oct 18, 2010 at 12:17:46PM +0900, Isaku Yamahata wrote:
> > > lower 4bits of base/limit register is RO, and
> > > should not be modified on reset.
> > > 
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > ---
> > >  hw/pci_bridge.c |   15 +++++++++------
> > >  1 files changed, 9 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
> > > index 638e3b3..7229610 100644
> > > --- a/hw/pci_bridge.c
> > > +++ b/hw/pci_bridge.c
> > > @@ -161,12 +161,15 @@ void pci_bridge_reset_reg(PCIDevice *dev)
> > >      conf[PCI_SUBORDINATE_BUS] = 0;
> > >      conf[PCI_SEC_LATENCY_TIMER] = 0;
> > >  
> > > -    conf[PCI_IO_BASE] = 0;
> > > -    conf[PCI_IO_LIMIT] = 0;
> > > -    pci_set_word(conf + PCI_MEMORY_BASE, 0);
> > > -    pci_set_word(conf + PCI_MEMORY_LIMIT, 0);
> > > -    pci_set_word(conf + PCI_PREF_MEMORY_BASE, 0);
> > > -    pci_set_word(conf + PCI_PREF_MEMORY_LIMIT, 0);
> > > +    pci_clear_bit_byte(conf + PCI_IO_BASE, PCI_IO_RANGE_MASK & 0xff);
> > > +    pci_clear_bit_byte(conf + PCI_IO_LIMIT, PCI_IO_RANGE_MASK & 0xff);
> > 
> > No need for & 0xff and & 0xfffff here and below.
> 
> gcc complains like this without them.
> hw/pci_bridge.c:165: error: large integer implicitly truncated to unsigned type

I see.

> 
> > also, PCI spec also says that these registers' value
> > is undefined after reset, so no need to clear them?
> > If there's a reason, pls put it in comment.
> 
> The spec says the lower bits are read-only and they have its meaning.

Yes. but my question is why even touch io base/io limit at all
in this function?
It looks like guest can not rely on these being 0 after reset.

> Anyway I'll add the reference as comment.
> 
> For example.
> 
> > 3.2.5.6 I/O Base Register and I/O Limit Register
> >
> > If the low four bits of the I/O Base and I/O Limit registers are 01h,
> > then the bridge supports 32-bit I/O address decoding, and the I/O Base
> > Upper 16 Bits and the I/O Limit Upper 16 Bits hold the upper 16 bits,
> > corresponding to AD[31::16], of the 32-bit I/O Base and I/O Limit
> > addresses respectively. In this case, system configuration software is
> > permitted to locate the I/O address range supported by the bridge
> > anywhere in the 4-GB I/O Space. Note that the 4-KB alignment and
> > granularity restrictions still apply when the bridge supports 32-bit
> > I/O addressing.
> > table 3-7

I don't think this is required but up to you.

> > 
> > 
> > > +    pci_clear_bit_word(conf + PCI_MEMORY_BASE, PCI_MEMORY_RANGE_MASK & 0xffff);
> > > +    pci_clear_bit_word(conf + PCI_MEMORY_LIMIT,
> > > +                       PCI_MEMORY_RANGE_MASK & 0xffff);
> > > +    pci_clear_bit_word(conf + PCI_PREF_MEMORY_BASE,
> > > +                       PCI_PREF_RANGE_MASK & 0xffff);
> > > +    pci_clear_bit_word(conf + PCI_PREF_MEMORY_LIMIT,
> > > +                       PCI_PREF_RANGE_MASK & 0xffff);
> > >      pci_set_word(conf + PCI_PREF_BASE_UPPER32, 0);
> > >      pci_set_word(conf + PCI_PREF_LIMIT_UPPER32, 0);
> > >  
> > > -- 
> > > 1.7.1.1
> > 
> 
> -- 
> yamahata

On Mon, Oct 18, 2010 at 08:22:24AM +0200, Michael S. Tsirkin wrote:
> On Mon, Oct 18, 2010 at 12:17:46PM +0900, Isaku Yamahata wrote:
> > lower 4bits of base/limit register is RO, and
> > should not be modified on reset.
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > ---
> >  hw/pci_bridge.c |   15 +++++++++------
> >  1 files changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
> > index 638e3b3..7229610 100644
> > --- a/hw/pci_bridge.c
> > +++ b/hw/pci_bridge.c
> > @@ -161,12 +161,15 @@ void pci_bridge_reset_reg(PCIDevice *dev)
> >      conf[PCI_SUBORDINATE_BUS] = 0;
> >      conf[PCI_SEC_LATENCY_TIMER] = 0;
> >  
> > -    conf[PCI_IO_BASE] = 0;
> > -    conf[PCI_IO_LIMIT] = 0;
> > -    pci_set_word(conf + PCI_MEMORY_BASE, 0);
> > -    pci_set_word(conf + PCI_MEMORY_LIMIT, 0);
> > -    pci_set_word(conf + PCI_PREF_MEMORY_BASE, 0);
> > -    pci_set_word(conf + PCI_PREF_MEMORY_LIMIT, 0);
> > +    pci_clear_bit_byte(conf + PCI_IO_BASE, PCI_IO_RANGE_MASK & 0xff);
> > +    pci_clear_bit_byte(conf + PCI_IO_LIMIT, PCI_IO_RANGE_MASK & 0xff);
> 
> No need for & 0xff and & 0xfffff here and below.

gcc complains like this without them.
hw/pci_bridge.c:165: error: large integer implicitly truncated to unsigned type


> also, PCI spec also says that these registers' value
> is undefined after reset, so no need to clear them?
> If there's a reason, pls put it in comment.

The spec says the lower bits are read-only and they have its meaning.
Anyway I'll add the reference as comment.

For example.

> 3.2.5.6 I/O Base Register and I/O Limit Register
>
> If the low four bits of the I/O Base and I/O Limit registers are 01h,
> then the bridge supports 32-bit I/O address decoding, and the I/O Base
> Upper 16 Bits and the I/O Limit Upper 16 Bits hold the upper 16 bits,
> corresponding to AD[31::16], of the 32-bit I/O Base and I/O Limit
> addresses respectively. In this case, system configuration software is
> permitted to locate the I/O address range supported by the bridge
> anywhere in the 4-GB I/O Space. Note that the 4-KB alignment and
> granularity restrictions still apply when the bridge supports 32-bit
> I/O addressing.
> table 3-7



> 
> 
> > +    pci_clear_bit_word(conf + PCI_MEMORY_BASE, PCI_MEMORY_RANGE_MASK & 0xffff);
> > +    pci_clear_bit_word(conf + PCI_MEMORY_LIMIT,
> > +                       PCI_MEMORY_RANGE_MASK & 0xffff);
> > +    pci_clear_bit_word(conf + PCI_PREF_MEMORY_BASE,
> > +                       PCI_PREF_RANGE_MASK & 0xffff);
> > +    pci_clear_bit_word(conf + PCI_PREF_MEMORY_LIMIT,
> > +                       PCI_PREF_RANGE_MASK & 0xffff);
> >      pci_set_word(conf + PCI_PREF_BASE_UPPER32, 0);
> >      pci_set_word(conf + PCI_PREF_LIMIT_UPPER32, 0);
> >  
> > -- 
> > 1.7.1.1
>

On Mon, Oct 18, 2010 at 09:08:09AM +0200, Michael S. Tsirkin wrote:

> > The spec says the lower bits are read-only and they have its meaning.
> 
> Yes. but my question is why even touch io base/io limit at all
> in this function?
> It looks like guest can not rely on these being 0 after reset.

Oh, now I'm seeing your point.
Then, I'll drop those lines and don't touch those registers
on reset at all.