Patchwork [38/61] pci: fix pci_default_write_config()

login
register
mail settings
Submitter Isaku Yamahata
Date Sept. 30, 2009, 10:18 a.m.
Message ID <1254305917-14784-39-git-send-email-yamahata@valinux.co.jp>
Download mbox | patch
Permalink /patch/34563/
State Superseded
Headers show

Comments

Isaku Yamahata - Sept. 30, 2009, 10:18 a.m.
When updated ROM expantion address of header type 0,
it missed to update mappings.
By using helper functions this patch also avoids memcpy() and memcmp().

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |   11 +++++------
 hw/pci.h |    9 ++++++++-
 2 files changed, 13 insertions(+), 7 deletions(-)
Michael S. Tsirkin - Sept. 30, 2009, 10:44 a.m.
On Wed, Sep 30, 2009 at 07:18:14PM +0900, Isaku Yamahata wrote:
> When updated ROM expantion address of header type 0,
> it missed to update mappings.

Can the bugfix be separated from proposed optimization please?
It should be a one-liner.

> By using helper functions this patch also avoids memcpy() and memcmp().

I expect this is all slow-path done during boot, so an extra memcpy
operation can not hurt.

I actually think keeping the original copy around is a good thing,
we probably should put it in PCI structure, so that devices can easily
check whether some value was changed.

For example, in your patch pci_update_mappings is called even if none of
the values are changed, or if memory is disabled, and
you also don't seem to update mappings when memory is enabled/disabled.

> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.c |   11 +++++------
>  hw/pci.h |    9 ++++++++-
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 757fe7b..2a59667 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -627,20 +627,19 @@ uint32_t pci_default_read_config(PCIDevice *d,
>  
>  void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
>  {
> -    uint8_t orig[PCI_CONFIG_SPACE_SIZE];
>      int i;
>      uint32_t config_size = pcie_config_size(d);
>  
> -    /* not efficient, but simple */
> -    memcpy(orig, d->config, PCI_CONFIG_SPACE_SIZE);
>      for(i = 0; i < l && addr < config_size; val >>= 8, ++i, ++addr) {
>          uint8_t wmask = d->wmask[addr];
>          d->config[addr] = (d->config[addr] & ~wmask) | (val & wmask);
>      }
> -    if (memcmp(orig + PCI_BASE_ADDRESS_0, d->config + PCI_BASE_ADDRESS_0, 24)
> -        || ((orig[PCI_COMMAND] ^ d->config[PCI_COMMAND])
> -            & (PCI_COMMAND_MEMORY | PCI_COMMAND_IO)))
> +
> +    if (pci_config_changed(addr, l,
> +                           PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_5 + 4) ||

How can pci_config_changed know whether some value was
actually changed without looking at original and new value?
IMO it's better to keep the original array around,
and use simple memcmp.

> +        pci_config_changed_with_size(addr, l, PCI_ROM_ADDRESS, 4)) {
>          pci_update_mappings(d);
> +    }
>  }
>  
>  static PCIBus *pci_find_bus_from(PCIBus *from, int bus_num)
> diff --git a/hw/pci.h b/hw/pci.h
> index 26c15c5..3327905 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -121,7 +121,12 @@ static inline int pci_bar_is_64bit(const PCIIORegion *r)
>  #define  PCI_HEADER_TYPE_BRIDGE		1
>  #define  PCI_HEADER_TYPE_CARDBUS	2
>  #define  PCI_HEADER_TYPE_MULTI_FUNCTION 0x80
> -#define PCI_BASE_ADDRESS_0	0x10	/* 32 bits */
> +#define PCI_BASE_ADDRESS_0      0x10    /* 32 bits */
> +#define PCI_BASE_ADDRESS_1      0x14    /* 32 bits [htype 0,1 only] */
> +#define PCI_BASE_ADDRESS_2      0x18    /* 32 bits [htype 0 only] */
> +#define PCI_BASE_ADDRESS_3      0x1c    /* 32 bits */
> +#define PCI_BASE_ADDRESS_4      0x20    /* 32 bits */
> +#define PCI_BASE_ADDRESS_5      0x24    /* 32 bits */
>  #define PCI_PRIMARY_BUS		0x18	/* Primary bus number */
>  #define PCI_SECONDARY_BUS	0x19	/* Secondary bus number */
>  #define PCI_SEC_STATUS		0x1e	/* Secondary status register, only bit 14 used */
> @@ -141,7 +146,9 @@ static inline int pci_bar_is_64bit(const PCIIORegion *r)
>  #define PCI_SUBVENDOR_ID        0x2c    /* obsolete, use PCI_SUBSYSTEM_VENDOR_ID */
>  #define PCI_SUBDEVICE_ID        0x2e    /* obsolete, use PCI_SUBSYSTEM_ID */
>  
> +#define PCI_ROM_ADDRESS         0x30    /* Bits 31..11 are address, 10..1 reserved */
>  #define  PCI_ROM_ADDRESS_ENABLE 0x01
> +#define PCI_ROM_ADDRESS_MASK    (~0x7ffUL)
>  
>  /* Bits in the PCI Status Register (PCI 2.3 spec) */
>  #define PCI_STATUS_RESERVED1	0x007
> -- 
> 1.6.0.2
Isaku Yamahata - Sept. 30, 2009, 11:09 a.m.
On Wed, Sep 30, 2009 at 12:44:46PM +0200, Michael S. Tsirkin wrote:
> On Wed, Sep 30, 2009 at 07:18:14PM +0900, Isaku Yamahata wrote:
> > When updated ROM expantion address of header type 0,
> > it missed to update mappings.
> 
> Can the bugfix be separated from proposed optimization please?
> It should be a one-liner.

Will do.


> > By using helper functions this patch also avoids memcpy() and memcmp().
> 
> I expect this is all slow-path done during boot, so an extra memcpy
> operation can not hurt.
> I actually think keeping the original copy around is a good thing,
> we probably should put it in PCI structure, so that devices can easily
> check whether some value was changed.

Yes, I agree that it's slow path.
In fact I did it having PCI express support in mind.
PCI express has 4K bytes config space instead of 256 bytes.
I suppose 4K is a bit large for stack/memcpy/memcmp to detect
at most 4 bytes change.
So Optimization to remember 4 bytes (+ alignment?) and
compare is what you want.


> For example, in your patch pci_update_mappings is called even if none of
> the values are changed, or if memory is disabled, and
> you also don't seem to update mappings when memory is enabled/disabled.

It's slow path, isn't it?
And pci_update_mapppings() takes care of such cases.


> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > ---
> >  hw/pci.c |   11 +++++------
> >  hw/pci.h |    9 ++++++++-
> >  2 files changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 757fe7b..2a59667 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -627,20 +627,19 @@ uint32_t pci_default_read_config(PCIDevice *d,
> >  
> >  void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
> >  {
> > -    uint8_t orig[PCI_CONFIG_SPACE_SIZE];
> >      int i;
> >      uint32_t config_size = pcie_config_size(d);
> >  
> > -    /* not efficient, but simple */
> > -    memcpy(orig, d->config, PCI_CONFIG_SPACE_SIZE);
> >      for(i = 0; i < l && addr < config_size; val >>= 8, ++i, ++addr) {
> >          uint8_t wmask = d->wmask[addr];
> >          d->config[addr] = (d->config[addr] & ~wmask) | (val & wmask);
> >      }
> > -    if (memcmp(orig + PCI_BASE_ADDRESS_0, d->config + PCI_BASE_ADDRESS_0, 24)
> > -        || ((orig[PCI_COMMAND] ^ d->config[PCI_COMMAND])
> > -            & (PCI_COMMAND_MEMORY | PCI_COMMAND_IO)))
> > +
> > +    if (pci_config_changed(addr, l,
> > +                           PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_5 + 4) ||
> 
> How can pci_config_changed know whether some value was
> actually changed without looking at original and new value?
> IMO it's better to keep the original array around,
> and use simple memcmp.
> 
> > +        pci_config_changed_with_size(addr, l, PCI_ROM_ADDRESS, 4)) {
> >          pci_update_mappings(d);
> > +    }
> >  }
> >  
> >  static PCIBus *pci_find_bus_from(PCIBus *from, int bus_num)
> > diff --git a/hw/pci.h b/hw/pci.h
> > index 26c15c5..3327905 100644
> > --- a/hw/pci.h
> > +++ b/hw/pci.h
> > @@ -121,7 +121,12 @@ static inline int pci_bar_is_64bit(const PCIIORegion *r)
> >  #define  PCI_HEADER_TYPE_BRIDGE		1
> >  #define  PCI_HEADER_TYPE_CARDBUS	2
> >  #define  PCI_HEADER_TYPE_MULTI_FUNCTION 0x80
> > -#define PCI_BASE_ADDRESS_0	0x10	/* 32 bits */
> > +#define PCI_BASE_ADDRESS_0      0x10    /* 32 bits */
> > +#define PCI_BASE_ADDRESS_1      0x14    /* 32 bits [htype 0,1 only] */
> > +#define PCI_BASE_ADDRESS_2      0x18    /* 32 bits [htype 0 only] */
> > +#define PCI_BASE_ADDRESS_3      0x1c    /* 32 bits */
> > +#define PCI_BASE_ADDRESS_4      0x20    /* 32 bits */
> > +#define PCI_BASE_ADDRESS_5      0x24    /* 32 bits */
> >  #define PCI_PRIMARY_BUS		0x18	/* Primary bus number */
> >  #define PCI_SECONDARY_BUS	0x19	/* Secondary bus number */
> >  #define PCI_SEC_STATUS		0x1e	/* Secondary status register, only bit 14 used */
> > @@ -141,7 +146,9 @@ static inline int pci_bar_is_64bit(const PCIIORegion *r)
> >  #define PCI_SUBVENDOR_ID        0x2c    /* obsolete, use PCI_SUBSYSTEM_VENDOR_ID */
> >  #define PCI_SUBDEVICE_ID        0x2e    /* obsolete, use PCI_SUBSYSTEM_ID */
> >  
> > +#define PCI_ROM_ADDRESS         0x30    /* Bits 31..11 are address, 10..1 reserved */
> >  #define  PCI_ROM_ADDRESS_ENABLE 0x01
> > +#define PCI_ROM_ADDRESS_MASK    (~0x7ffUL)
> >  
> >  /* Bits in the PCI Status Register (PCI 2.3 spec) */
> >  #define PCI_STATUS_RESERVED1	0x007
>
Michael S. Tsirkin - Sept. 30, 2009, 12:50 p.m.
On Wed, Sep 30, 2009 at 08:09:42PM +0900, Isaku Yamahata wrote:
> On Wed, Sep 30, 2009 at 12:44:46PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Sep 30, 2009 at 07:18:14PM +0900, Isaku Yamahata wrote:
> > > When updated ROM expantion address of header type 0,
> > > it missed to update mappings.
> > 
> > Can the bugfix be separated from proposed optimization please?
> > It should be a one-liner.
> 
> Will do.
> 
> 
> > > By using helper functions this patch also avoids memcpy() and memcmp().
> > 
> > I expect this is all slow-path done during boot, so an extra memcpy
> > operation can not hurt.
> > I actually think keeping the original copy around is a good thing,
> > we probably should put it in PCI structure, so that devices can easily
> > check whether some value was changed.
> 
> Yes, I agree that it's slow path.
> In fact I did it having PCI express support in mind.
> PCI express has 4K bytes config space instead of 256 bytes.
> I suppose 4K is a bit large for stack/memcpy/memcmp to detect
> at most 4 bytes change.
> So Optimization to remember 4 bytes (+ alignment?) and
> compare is what you want.

I don't know if the concern is real. Isn't this premature optimization?
Assuming it is not - the way I would do this, is keep the copy of the
array inside PCI device structure (no stack), and only memcpy the
relevant bytes to keep them in sync. It should be as simple as:
	memcpy(pdev->origin + addr, pdev->config + addr, len)
memcpy is done with a small length, so it is cheap.

> 
> > For example, in your patch pci_update_mappings is called even if none of
> > the values are changed, or if memory is disabled, and
> > you also don't seem to update mappings when memory is enabled/disabled.
> 
> It's slow path, isn't it?

Yes, but you seem add code to optimize it.  FWIW, with a lot of devices,
extra scan will be much slower than a 4K memcpy which might be a single
asm instruction.

> And pci_update_mapppings() takes care of such cases.

Yes but you don't seem to call it e.g. on command write.

> 
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > ---
> > >  hw/pci.c |   11 +++++------
> > >  hw/pci.h |    9 ++++++++-
> > >  2 files changed, 13 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/hw/pci.c b/hw/pci.c
> > > index 757fe7b..2a59667 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -627,20 +627,19 @@ uint32_t pci_default_read_config(PCIDevice *d,
> > >  
> > >  void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
> > >  {
> > > -    uint8_t orig[PCI_CONFIG_SPACE_SIZE];
> > >      int i;
> > >      uint32_t config_size = pcie_config_size(d);
> > >  
> > > -    /* not efficient, but simple */
> > > -    memcpy(orig, d->config, PCI_CONFIG_SPACE_SIZE);
> > >      for(i = 0; i < l && addr < config_size; val >>= 8, ++i, ++addr) {
> > >          uint8_t wmask = d->wmask[addr];
> > >          d->config[addr] = (d->config[addr] & ~wmask) | (val & wmask);
> > >      }
> > > -    if (memcmp(orig + PCI_BASE_ADDRESS_0, d->config + PCI_BASE_ADDRESS_0, 24)
> > > -        || ((orig[PCI_COMMAND] ^ d->config[PCI_COMMAND])
> > > -            & (PCI_COMMAND_MEMORY | PCI_COMMAND_IO)))
> > > +
> > > +    if (pci_config_changed(addr, l,
> > > +                           PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_5 + 4) ||
> > 
> > How can pci_config_changed know whether some value was
> > actually changed without looking at original and new value?
> > IMO it's better to keep the original array around,
> > and use simple memcmp.
> > 
> > > +        pci_config_changed_with_size(addr, l, PCI_ROM_ADDRESS, 4)) {
> > >          pci_update_mappings(d);
> > > +    }
> > >  }
> > >  
> > >  static PCIBus *pci_find_bus_from(PCIBus *from, int bus_num)
> > > diff --git a/hw/pci.h b/hw/pci.h
> > > index 26c15c5..3327905 100644
> > > --- a/hw/pci.h
> > > +++ b/hw/pci.h
> > > @@ -121,7 +121,12 @@ static inline int pci_bar_is_64bit(const PCIIORegion *r)
> > >  #define  PCI_HEADER_TYPE_BRIDGE		1
> > >  #define  PCI_HEADER_TYPE_CARDBUS	2
> > >  #define  PCI_HEADER_TYPE_MULTI_FUNCTION 0x80
> > > -#define PCI_BASE_ADDRESS_0	0x10	/* 32 bits */
> > > +#define PCI_BASE_ADDRESS_0      0x10    /* 32 bits */
> > > +#define PCI_BASE_ADDRESS_1      0x14    /* 32 bits [htype 0,1 only] */
> > > +#define PCI_BASE_ADDRESS_2      0x18    /* 32 bits [htype 0 only] */
> > > +#define PCI_BASE_ADDRESS_3      0x1c    /* 32 bits */
> > > +#define PCI_BASE_ADDRESS_4      0x20    /* 32 bits */
> > > +#define PCI_BASE_ADDRESS_5      0x24    /* 32 bits */
> > >  #define PCI_PRIMARY_BUS		0x18	/* Primary bus number */
> > >  #define PCI_SECONDARY_BUS	0x19	/* Secondary bus number */
> > >  #define PCI_SEC_STATUS		0x1e	/* Secondary status register, only bit 14 used */
> > > @@ -141,7 +146,9 @@ static inline int pci_bar_is_64bit(const PCIIORegion *r)
> > >  #define PCI_SUBVENDOR_ID        0x2c    /* obsolete, use PCI_SUBSYSTEM_VENDOR_ID */
> > >  #define PCI_SUBDEVICE_ID        0x2e    /* obsolete, use PCI_SUBSYSTEM_ID */
> > >  
> > > +#define PCI_ROM_ADDRESS         0x30    /* Bits 31..11 are address, 10..1 reserved */
> > >  #define  PCI_ROM_ADDRESS_ENABLE 0x01
> > > +#define PCI_ROM_ADDRESS_MASK    (~0x7ffUL)
> > >  
> > >  /* Bits in the PCI Status Register (PCI 2.3 spec) */
> > >  #define PCI_STATUS_RESERVED1	0x007
> > 
> 
> -- 
> yamahata

Patch

diff --git a/hw/pci.c b/hw/pci.c
index 757fe7b..2a59667 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -627,20 +627,19 @@  uint32_t pci_default_read_config(PCIDevice *d,
 
 void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
 {
-    uint8_t orig[PCI_CONFIG_SPACE_SIZE];
     int i;
     uint32_t config_size = pcie_config_size(d);
 
-    /* not efficient, but simple */
-    memcpy(orig, d->config, PCI_CONFIG_SPACE_SIZE);
     for(i = 0; i < l && addr < config_size; val >>= 8, ++i, ++addr) {
         uint8_t wmask = d->wmask[addr];
         d->config[addr] = (d->config[addr] & ~wmask) | (val & wmask);
     }
-    if (memcmp(orig + PCI_BASE_ADDRESS_0, d->config + PCI_BASE_ADDRESS_0, 24)
-        || ((orig[PCI_COMMAND] ^ d->config[PCI_COMMAND])
-            & (PCI_COMMAND_MEMORY | PCI_COMMAND_IO)))
+
+    if (pci_config_changed(addr, l,
+                           PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_5 + 4) ||
+        pci_config_changed_with_size(addr, l, PCI_ROM_ADDRESS, 4)) {
         pci_update_mappings(d);
+    }
 }
 
 static PCIBus *pci_find_bus_from(PCIBus *from, int bus_num)
diff --git a/hw/pci.h b/hw/pci.h
index 26c15c5..3327905 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -121,7 +121,12 @@  static inline int pci_bar_is_64bit(const PCIIORegion *r)
 #define  PCI_HEADER_TYPE_BRIDGE		1
 #define  PCI_HEADER_TYPE_CARDBUS	2
 #define  PCI_HEADER_TYPE_MULTI_FUNCTION 0x80
-#define PCI_BASE_ADDRESS_0	0x10	/* 32 bits */
+#define PCI_BASE_ADDRESS_0      0x10    /* 32 bits */
+#define PCI_BASE_ADDRESS_1      0x14    /* 32 bits [htype 0,1 only] */
+#define PCI_BASE_ADDRESS_2      0x18    /* 32 bits [htype 0 only] */
+#define PCI_BASE_ADDRESS_3      0x1c    /* 32 bits */
+#define PCI_BASE_ADDRESS_4      0x20    /* 32 bits */
+#define PCI_BASE_ADDRESS_5      0x24    /* 32 bits */
 #define PCI_PRIMARY_BUS		0x18	/* Primary bus number */
 #define PCI_SECONDARY_BUS	0x19	/* Secondary bus number */
 #define PCI_SEC_STATUS		0x1e	/* Secondary status register, only bit 14 used */
@@ -141,7 +146,9 @@  static inline int pci_bar_is_64bit(const PCIIORegion *r)
 #define PCI_SUBVENDOR_ID        0x2c    /* obsolete, use PCI_SUBSYSTEM_VENDOR_ID */
 #define PCI_SUBDEVICE_ID        0x2e    /* obsolete, use PCI_SUBSYSTEM_ID */
 
+#define PCI_ROM_ADDRESS         0x30    /* Bits 31..11 are address, 10..1 reserved */
 #define  PCI_ROM_ADDRESS_ENABLE 0x01
+#define PCI_ROM_ADDRESS_MASK    (~0x7ffUL)
 
 /* Bits in the PCI Status Register (PCI 2.3 spec) */
 #define PCI_STATUS_RESERVED1	0x007