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

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Sep 14, 2010 at 07:29:15PM +0900, Isaku Yamahata wrote:
> 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

Just add (Write 1 to Clear) and that's clear enough
I think, the table in the comment is not really helpful.

> +     * regiger      written val        result

regiger -> register :)

> +     * 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;

It will definitely work. Does this help PCIE code as well?
If yes it's probably worth it. If no we could
do it just for SERR in a simple way like this:

      for (i = 0; i < l && addr + i < config_size; val >>= 8, ++i) {

          .... (existing code) ...

          if (addr + i == PCI_COMMAND + 1 && (val & (PCI_COMMAND_SERR >> 8))) {
    	      d->config[addr + i] &= ~(PCI_COMMAND_SERR >> 8);
          }
      }


> > > +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, ...

Sure, it's good to split to smaller functions,
but all these can be static so this would not affect
the external API, it can still be clean and simple.

-- 
MST