Message ID | 7d5d92f508899391d060809e9a419c7556905379.1287371107.git.yamahata@valinux.co.jp |
---|---|
State | New |
Headers | show |
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
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 >
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 --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);
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(-)