diff mbox

[v4,04/15] pci: record which is written into pci configuration space

Message ID 7d5d92f508899391d060809e9a419c7556905379.1287371107.git.yamahata@valinux.co.jp
State New
Headers show

Commit Message

Isaku Yamahata Oct. 18, 2010, 3:17 a.m. UTC 
record which is written into pci configuration space.
introduce helper function to zero PCIDevice::written.
They will be used later.

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

Comments

Michael S. Tsirkin Oct. 18, 2010, 5:38 a.m. UTC | #1 
On Mon, Oct 18, 2010 at 12:17:45PM +0900, Isaku Yamahata wrote:
> record which is written into pci configuration space.
> introduce helper function to zero PCIDevice::written.
> They will be used later.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

This really exposes an internal variable.
I really dislike this, and I don't think it's needed
at all: just make the bit writeable.
Commented on appropriate patches.

> ---
>  hw/pci.c |   10 ++++++++++
>  hw/pci.h |    5 +++++
>  2 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 5954476..eca9324 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -627,6 +627,7 @@ static void pci_config_alloc(PCIDevice *pci_dev)
>      pci_dev->cmask = qemu_mallocz(config_size);
>      pci_dev->wmask = qemu_mallocz(config_size);
>      pci_dev->w1cmask = qemu_mallocz(config_size);
> +    pci_dev->written = qemu_mallocz(config_size);
>      pci_dev->used = qemu_mallocz(config_size);
>  }
>  
> @@ -636,6 +637,7 @@ static void pci_config_free(PCIDevice *pci_dev)
>      qemu_free(pci_dev->cmask);
>      qemu_free(pci_dev->wmask);
>      qemu_free(pci_dev->w1cmask);
> +    qemu_free(pci_dev->written);
>      qemu_free(pci_dev->used);
>  }
>  
> @@ -1002,6 +1004,8 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
>          assert(!(wmask & w1cmask));
>          d->config[addr + i] = (d->config[addr + i] & ~wmask) | (val & wmask);
>          d->config[addr + i] &= ~(val & w1cmask); /* W1C: Write 1 to Clear */
> +        d->written[addr + i] = val; /* record what is written for driver
> +                                       specific code */
>      }
>      if (ranges_overlap(addr, l, PCI_BASE_ADDRESS_0, 24) ||
>          ranges_overlap(addr, l, PCI_ROM_ADDRESS, 4) ||
> @@ -1013,6 +1017,12 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
>          pci_update_irq_disabled(d, was_irq_disabled);
>  }
>  
> +void pci_clear_written_write_config(PCIDevice *d,
> +                                    uint32_t addr, uint32_t val, int l)
> +{
> +    memset(d->written + addr, 0, l);
> +}
> +
>  /***********************************************************/
>  /* generic PCI irq support */
>  
> diff --git a/hw/pci.h b/hw/pci.h
> index eafa9f3..7097817 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -132,6 +132,9 @@ struct PCIDevice {
>      /* Used to implement RW1C(Write 1 to Clear) bytes */
>      uint8_t *w1cmask;
>  
> +    /* Used to record what value is written */
> +    uint8_t *written;
> +
>      /* Used to allocate config space for capabilities. */
>      uint8_t *used;
>  
> @@ -200,6 +203,8 @@ uint32_t pci_default_read_config(PCIDevice *d,
>                                   uint32_t address, int len);
>  void pci_default_write_config(PCIDevice *d,
>                                uint32_t address, uint32_t val, int len);
> +void pci_clear_written_write_config(PCIDevice *d,
> +                                    uint32_t addr, uint32_t val, int l);
>  void pci_device_save(PCIDevice *s, QEMUFile *f);
>  int pci_device_load(PCIDevice *s, QEMUFile *f);
>  
> -- 
> 1.7.1.1
Isaku Yamahata Oct. 18, 2010, 7:17 a.m. UTC | #2 
On Mon, Oct 18, 2010 at 07:38:53AM +0200, Michael S. Tsirkin wrote:
> On Mon, Oct 18, 2010 at 12:17:45PM +0900, Isaku Yamahata wrote:
> > record which is written into pci configuration space.
> > introduce helper function to zero PCIDevice::written.
> > They will be used later.
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> This really exposes an internal variable.
> I really dislike this, and I don't think it's needed
> at all: just make the bit writeable.
> Commented on appropriate patches.

I see. So You really want those bit writable.
Then how about introducing pci_{byte, word, long}_test_and_clear_mask()
helper functions?



> 
> > ---
> >  hw/pci.c |   10 ++++++++++
> >  hw/pci.h |    5 +++++
> >  2 files changed, 15 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 5954476..eca9324 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -627,6 +627,7 @@ static void pci_config_alloc(PCIDevice *pci_dev)
> >      pci_dev->cmask = qemu_mallocz(config_size);
> >      pci_dev->wmask = qemu_mallocz(config_size);
> >      pci_dev->w1cmask = qemu_mallocz(config_size);
> > +    pci_dev->written = qemu_mallocz(config_size);
> >      pci_dev->used = qemu_mallocz(config_size);
> >  }
> >  
> > @@ -636,6 +637,7 @@ static void pci_config_free(PCIDevice *pci_dev)
> >      qemu_free(pci_dev->cmask);
> >      qemu_free(pci_dev->wmask);
> >      qemu_free(pci_dev->w1cmask);
> > +    qemu_free(pci_dev->written);
> >      qemu_free(pci_dev->used);
> >  }
> >  
> > @@ -1002,6 +1004,8 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
> >          assert(!(wmask & w1cmask));
> >          d->config[addr + i] = (d->config[addr + i] & ~wmask) | (val & wmask);
> >          d->config[addr + i] &= ~(val & w1cmask); /* W1C: Write 1 to Clear */
> > +        d->written[addr + i] = val; /* record what is written for driver
> > +                                       specific code */
> >      }
> >      if (ranges_overlap(addr, l, PCI_BASE_ADDRESS_0, 24) ||
> >          ranges_overlap(addr, l, PCI_ROM_ADDRESS, 4) ||
> > @@ -1013,6 +1017,12 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
> >          pci_update_irq_disabled(d, was_irq_disabled);
> >  }
> >  
> > +void pci_clear_written_write_config(PCIDevice *d,
> > +                                    uint32_t addr, uint32_t val, int l)
> > +{
> > +    memset(d->written + addr, 0, l);
> > +}
> > +
> >  /***********************************************************/
> >  /* generic PCI irq support */
> >  
> > diff --git a/hw/pci.h b/hw/pci.h
> > index eafa9f3..7097817 100644
> > --- a/hw/pci.h
> > +++ b/hw/pci.h
> > @@ -132,6 +132,9 @@ struct PCIDevice {
> >      /* Used to implement RW1C(Write 1 to Clear) bytes */
> >      uint8_t *w1cmask;
> >  
> > +    /* Used to record what value is written */
> > +    uint8_t *written;
> > +
> >      /* Used to allocate config space for capabilities. */
> >      uint8_t *used;
> >  
> > @@ -200,6 +203,8 @@ uint32_t pci_default_read_config(PCIDevice *d,
> >                                   uint32_t address, int len);
> >  void pci_default_write_config(PCIDevice *d,
> >                                uint32_t address, uint32_t val, int len);
> > +void pci_clear_written_write_config(PCIDevice *d,
> > +                                    uint32_t addr, uint32_t val, int l);
> >  void pci_device_save(PCIDevice *s, QEMUFile *f);
> >  int pci_device_load(PCIDevice *s, QEMUFile *f);
> >  
> > -- 
> > 1.7.1.1
>
Michael S. Tsirkin Oct. 18, 2010, 7:32 a.m. UTC | #3 
On Mon, Oct 18, 2010 at 04:17:40PM +0900, Isaku Yamahata wrote:
> On Mon, Oct 18, 2010 at 07:38:53AM +0200, Michael S. Tsirkin wrote:
> > On Mon, Oct 18, 2010 at 12:17:45PM +0900, Isaku Yamahata wrote:
> > > record which is written into pci configuration space.
> > > introduce helper function to zero PCIDevice::written.
> > > They will be used later.
> > > 
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > 
> > This really exposes an internal variable.
> > I really dislike this, and I don't think it's needed
> > at all: just make the bit writeable.
> > Commented on appropriate patches.
> 
> I see. So You really want those bit writable.
> Then how about introducing pci_{byte, word, long}_test_and_clear_mask()
> helper functions?

okay, but then let's get rid of simple _clear/_set then: the side effect
or returning old value is harmless.  Also pls add a comment noting these
are not atomic.


> 
> 
> > 
> > > ---
> > >  hw/pci.c |   10 ++++++++++
> > >  hw/pci.h |    5 +++++
> > >  2 files changed, 15 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/hw/pci.c b/hw/pci.c
> > > index 5954476..eca9324 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -627,6 +627,7 @@ static void pci_config_alloc(PCIDevice *pci_dev)
> > >      pci_dev->cmask = qemu_mallocz(config_size);
> > >      pci_dev->wmask = qemu_mallocz(config_size);
> > >      pci_dev->w1cmask = qemu_mallocz(config_size);
> > > +    pci_dev->written = qemu_mallocz(config_size);
> > >      pci_dev->used = qemu_mallocz(config_size);
> > >  }
> > >  
> > > @@ -636,6 +637,7 @@ static void pci_config_free(PCIDevice *pci_dev)
> > >      qemu_free(pci_dev->cmask);
> > >      qemu_free(pci_dev->wmask);
> > >      qemu_free(pci_dev->w1cmask);
> > > +    qemu_free(pci_dev->written);
> > >      qemu_free(pci_dev->used);
> > >  }
> > >  
> > > @@ -1002,6 +1004,8 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
> > >          assert(!(wmask & w1cmask));
> > >          d->config[addr + i] = (d->config[addr + i] & ~wmask) | (val & wmask);
> > >          d->config[addr + i] &= ~(val & w1cmask); /* W1C: Write 1 to Clear */
> > > +        d->written[addr + i] = val; /* record what is written for driver
> > > +                                       specific code */
> > >      }
> > >      if (ranges_overlap(addr, l, PCI_BASE_ADDRESS_0, 24) ||
> > >          ranges_overlap(addr, l, PCI_ROM_ADDRESS, 4) ||
> > > @@ -1013,6 +1017,12 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
> > >          pci_update_irq_disabled(d, was_irq_disabled);
> > >  }
> > >  
> > > +void pci_clear_written_write_config(PCIDevice *d,
> > > +                                    uint32_t addr, uint32_t val, int l)
> > > +{
> > > +    memset(d->written + addr, 0, l);
> > > +}
> > > +
> > >  /***********************************************************/
> > >  /* generic PCI irq support */
> > >  
> > > diff --git a/hw/pci.h b/hw/pci.h
> > > index eafa9f3..7097817 100644
> > > --- a/hw/pci.h
> > > +++ b/hw/pci.h
> > > @@ -132,6 +132,9 @@ struct PCIDevice {
> > >      /* Used to implement RW1C(Write 1 to Clear) bytes */
> > >      uint8_t *w1cmask;
> > >  
> > > +    /* Used to record what value is written */
> > > +    uint8_t *written;
> > > +
> > >      /* Used to allocate config space for capabilities. */
> > >      uint8_t *used;
> > >  
> > > @@ -200,6 +203,8 @@ uint32_t pci_default_read_config(PCIDevice *d,
> > >                                   uint32_t address, int len);
> > >  void pci_default_write_config(PCIDevice *d,
> > >                                uint32_t address, uint32_t val, int len);
> > > +void pci_clear_written_write_config(PCIDevice *d,
> > > +                                    uint32_t addr, uint32_t val, int l);
> > >  void pci_device_save(PCIDevice *s, QEMUFile *f);
> > >  int pci_device_load(PCIDevice *s, QEMUFile *f);
> > >  
> > > -- 
> > > 1.7.1.1
> > 
> 
> -- 
> yamahata
diff mbox

Patch

diff --git a/hw/pci.c b/hw/pci.c
index 5954476..eca9324 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -627,6 +627,7 @@  static void pci_config_alloc(PCIDevice *pci_dev)
     pci_dev->cmask = qemu_mallocz(config_size);
     pci_dev->wmask = qemu_mallocz(config_size);
     pci_dev->w1cmask = qemu_mallocz(config_size);
+    pci_dev->written = qemu_mallocz(config_size);
     pci_dev->used = qemu_mallocz(config_size);
 }
 
@@ -636,6 +637,7 @@  static void pci_config_free(PCIDevice *pci_dev)
     qemu_free(pci_dev->cmask);
     qemu_free(pci_dev->wmask);
     qemu_free(pci_dev->w1cmask);
+    qemu_free(pci_dev->written);
     qemu_free(pci_dev->used);
 }
 
@@ -1002,6 +1004,8 @@  void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
         assert(!(wmask & w1cmask));
         d->config[addr + i] = (d->config[addr + i] & ~wmask) | (val & wmask);
         d->config[addr + i] &= ~(val & w1cmask); /* W1C: Write 1 to Clear */
+        d->written[addr + i] = val; /* record what is written for driver
+                                       specific code */
     }
     if (ranges_overlap(addr, l, PCI_BASE_ADDRESS_0, 24) ||
         ranges_overlap(addr, l, PCI_ROM_ADDRESS, 4) ||
@@ -1013,6 +1017,12 @@  void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
         pci_update_irq_disabled(d, was_irq_disabled);
 }
 
+void pci_clear_written_write_config(PCIDevice *d,
+                                    uint32_t addr, uint32_t val, int l)
+{
+    memset(d->written + addr, 0, l);
+}
+
 /***********************************************************/
 /* generic PCI irq support */
 
diff --git a/hw/pci.h b/hw/pci.h
index eafa9f3..7097817 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -132,6 +132,9 @@  struct PCIDevice {
     /* Used to implement RW1C(Write 1 to Clear) bytes */
     uint8_t *w1cmask;
 
+    /* Used to record what value is written */
+    uint8_t *written;
+
     /* Used to allocate config space for capabilities. */
     uint8_t *used;
 
@@ -200,6 +203,8 @@  uint32_t pci_default_read_config(PCIDevice *d,
                                  uint32_t address, int len);
 void pci_default_write_config(PCIDevice *d,
                               uint32_t address, uint32_t val, int len);
+void pci_clear_written_write_config(PCIDevice *d,
+                                    uint32_t addr, uint32_t val, int l);
 void pci_device_save(PCIDevice *s, QEMUFile *f);
 int pci_device_load(PCIDevice *s, QEMUFile *f);