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

[Michael S. Tsirkin]

On Wed, Jun 03, 2009 at 11:31:08AM +0900, Isaku Yamahata wrote:
> 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.

So maybe this is going too far into a table-driven direction then.
Tables are good for common case, exceptions are better handled
by regular functional design.

I agree addr/len comparisons are fragile, but can't we simply implement
functions to encapsulate them? Along the lines of:

static inline int offset_in_range(int offset, int address, int len)
{
        return address <= offset && address + len > offset;
}

static inline int ranges_match(int addr1, int len1,
                               int addr2, int len2)
{
        return offset_affected(addr1, addr2, len2) ||
               offset_affected(addr2, addr1, len1);
}

Switching address and len comparison to use this would be a good cleanup
IMO.

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

-- 
MST