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

[Isaku Yamahata]

On Mon, May 25, 2009 at 10:09:47AM +0300, Michael S. Tsirkin wrote:
> On Wed, May 20, 2009 at 11:25:33AM +0900, Isaku Yamahata wrote:
> > diff --git a/hw/pci.h b/hw/pci.h
> > index 7f7842f..6bb1c5c 100644
> > --- a/hw/pci.h
> > +++ b/hw/pci.h
> > +struct PCIConfigReg {
> > +    uint8_t wmask;
> > +    uint8_t shift;
> 
> I am not sure I understand what shift is.
> Document fields in this structure?

I'll add a comment.
It represents offset in a register. (Here by register I mean
that 8/16/24/32 bits width register with a dedicated function
like command register with 16bits width, status register with
16bits width, bar0 register with 32bits width ... and so on)

For example, command register case is
offset in config        shift
0x04                    0
0x05                    8

It makes sense when its register width is bigger than 8 bits.
callbacks can be made one-to-one for register, not offset in
configuration space.

"shift" may be misleading. 
It should be "offset", "offset_in_reg" or something like that?


> > +    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 think that life will be much easier if you'll keep
> separate arrays of masks and an array of callbacks.
> Then two simple memset calls can initialize a range or registers
> to a sane default (readonly, no side effects for head,
> writeable, no side effects for the rest of config space),
> and you'll only need to set up callbacks and masks for when
> something special is needed.

At the moment I don't see any big difference.
Anyway I don't have strong opinion here, it's a implementation detail.


-- 
yamahata