Re: [Qemu-devel] Re: [PATCH 3/7] pci: pci_default_config_write() clean up.

[Isaku Yamahata]

On Tue, Jun 02, 2009 at 01:01:21PM +0300, Michael S. Tsirkin wrote:
> On Tue, Jun 02, 2009 at 03:42:46PM +0900, Isaku Yamahata wrote:
> > +struct PCIConfigReg {
> > +    uint8_t wmask;
> > +    /* offset of registers in bits for 2/4 bytes function register */
> > +    uint8_t reg_offset;
> 
> Sorry about being dense, but the comment still doesn't help me much.
> Can't we simply use the index in the array as offset?

No. I believe this is helpfull.
the next patch for hw/wdt_i6300esb.c is a good example.
With this, we can replace fragile address and len comparison
with one callback per one register function.
For that, the member which represents the position in function
is necessary.

> 
> > +    pci_config_written_t callback;
> > +};
> >  
> >  struct PCIDevice {
> >      DeviceState qdev;
> >      /* PCI config space */
> >      uint8_t config[PCI_CONFIG_SPACE_SIZE];
> > -
> > -    /* Used to implement R/W bytes */
> > -    uint8_t mask[PCI_CONFIG_SPACE_SIZE];
> > +    struct PCIConfigReg config_regs[PCI_CONFIG_SPACE_SIZE];
> 
> I still think separate mask/config/callback arrays
> are better - they are easier for devices to use. E.g. a single memset
> can make a range of register writeable, and a single function call
> does everything necessary to save a range the whole config space.
> 
> Add a callback array if you like, and be done with it.

Hmm, I don't think so. Maybe it's a matter of taste.
Looking at your MSI/MSI-X patches, our patch series conflict with
each other. That's bad. Let's resolve the conflicts.

- mask v.s. wmask
  I think mask is ambiguous because which bits it represents,
  writable bits or read only bits. 
  let's rename it to common name.
  wmask, w_mask, wr_mask, writable_mask or something like that.
  What name do you prefer?

- array v.s. struct
  I suppose this conflicts with you.
  So after renaming, I'll make wmask into uint8_t wmask[]
  (or whatever name we choose)

Then, we can avoid stepping on each other.
What do you think?


> 
> >  
> >      /* the following fields are read only */
> >      PCIBus *bus;
> > @@ -180,6 +261,21 @@ struct PCIDevice {
> >      int irq_state[4];
> >  };
> >  
> > +typedef void(*pci_conf_init_t)(struct PCIConfigReg*);
> > +
> > +void pci_conf_initb(struct PCIConfigReg *config_regs, uint32_t addr,
> > +                    pci_config_written_t callback, uint32_t wmask);
> > +void pci_conf_initw(struct PCIConfigReg *config_regs, uint32_t addr,
> > +                    pci_config_written_t callback, uint32_t wmask);
> > +void pci_conf_initl(struct PCIConfigReg *config_regs, uint32_t addr,
> > +                    pci_config_written_t callback, uint32_t wmask);
> 
> If we got rid of reg_offset, I think we won't need these.
> We'd just do dev->callback[REGISTER] = my_callback.
> 
> 

-- 
yamahata