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

[Michael S. Tsirkin]

On Mon, Jun 15, 2009 at 06:12:04PM +0900, Isaku Yamahata wrote:
> On Wed, Jun 10, 2009 at 06:48:06PM +0300, Michael S. Tsirkin wrote:
> > 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.
> 
> Introducing helper function sounds a good idea.

Should be a separate patch IMO.

> I suppose that reg_offset and related functions can be removed
> by helper functions.
> So new callback function type would be
> 
> typedef void (*pci_config_written_t)(struct PCIDevice *d,
>                                       uint32_t addr, uint32_t val, int len);
> 
> Since this is same as PCIConfigWriteFunc, pci_config_written_t would
> be removed with the next version.


-- 
MST