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 <yamah...@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
