On Wed, Sep 08, 2010 at 01:31:22PM +0300, Michael S. Tsirkin wrote:
> > +/*
> > + * RW1C: Write-1-to-clear
> > + * regiger written val result
> > + * 0 0 => 0
> > + * 1 0 => 1
> > + * 0 1 => 0
> > + * 1 1 => 0
> > + */
> > +static inline void pcie_w1c_long(PCIDevice *d, uint32_t pos, uint32_t mask,
> > + uint32_t addr, uint32_t val)
> > +{
> > + uint32_t written = pcie_written_val_long(addr, val, pos) & mask;
> > + uint32_t reg = pci_get_long(d->config + pos);
> > + reg &= ~written;
> > + pci_set_long(d->config + pos, reg);
> > +}
> > +
> > +static inline void pcie_w1c_word(PCIDevice *d, uint32_t pos, uint16_t mask,
> > + uint32_t addr, uint32_t val)
> > +{
> > + uint16_t written = pcie_written_val_word(addr, val, pos) & mask;
> > + uint16_t reg = pci_get_word(d->config + pos);
> > + reg &= ~written;
> > + pci_set_word(d->config + pos, reg);
> > +}
> > +
>
> So the SERR bit support IMO belongs in pci. And this means the W1C
> inline functions need to move there.
>
> pci.c implemented this in a simpler way, by shifting
> val by 8 bytes each time. Can we find a way to do it
> in a similar way? I'll try to think about it.
How about the following?
diff --git a/hw/pci.c b/hw/pci.c
index ceee291..6f1ce48 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -627,6 +627,7 @@ static void pci_config_alloc(PCIDevice *pci_dev)
pci_dev->config = qemu_mallocz(config_size);
pci_dev->cmask = qemu_mallocz(config_size);
pci_dev->wmask = qemu_mallocz(config_size);
+ pci_dev->w1cmask = qemu_mallocz(config_size);
pci_dev->used = qemu_mallocz(config_size);
}
@@ -635,6 +636,7 @@ static void pci_config_free(PCIDevice *pci_dev)
qemu_free(pci_dev->config);
qemu_free(pci_dev->cmask);
qemu_free(pci_dev->wmask);
+ qemu_free(pci_dev->w1cmask);
qemu_free(pci_dev->used);
}
@@ -997,7 +999,10 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr,
uint32_t val, int l)
for (i = 0; i < l && addr + i < config_size; val >>= 8, ++i) {
uint8_t wmask = d->wmask[addr + i];
+ uint8_t w1cmask = d->w1cmask[addr + i];
+ assert(!(wmask & w1cmask));
d->config[addr + i] = (d->config[addr + i] & ~wmask) | (val & wmask);
+ d->config[addr + i] &= ~(val & w1cmask);
}
if (ranges_overlap(addr, l, PCI_BASE_ADDRESS_0, 24) ||
ranges_overlap(addr, l, PCI_ROM_ADDRESS, 4) ||
diff --git a/hw/pci.h b/hw/pci.h
index e001f92..3509459 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -132,6 +132,15 @@ struct PCIDevice {
/* Used to implement R/W bytes */
uint8_t *wmask;
+ /* Used to implement RW1C bytes
+ * regiger written val result
+ * 0 0 => 0
+ * 1 0 => 1
+ * 0 1 => 0
+ * 1 1 => 0
+ */
+ uint8_t *w1cmask;
+
/* Used to allocate config space for capabilities. */
uint8_t *used;
> > +int pci_pcie_cap_init(PCIDevice *dev,
> > + uint8_t offset, uint8_t type, uint8_t port)
>
> I think we should have
> pcie_init
> that would init everything and be external,
> and this one should be static, and this one
> should be static.
Ah, please take a look at Figure 7-10 if possible.
Express capability structure is too complex to initialize
by single function.
So I introduce functions that applies to all devices,
functions that applies to root complex,
function that applies, ...
--
yamahata
