[Qemu-devel] Re: [PATCH v2 2/9] pcie: helper functions for pcie extended capability.

[Isaku Yamahata]

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