[Qemu-devel] Re: [PATCH] qemu/pci: optimize pci config handling

[Michael S. Tsirkin]

On Thu, Oct 08, 2009 at 09:08:07AM +0900, Isaku Yamahata wrote:
> On Wed, Oct 07, 2009 at 02:33:49PM +0200, Michael S. Tsirkin wrote:
> > There's no need to save all of config space before each config cycle:
> > just the 64 byte header is enough for our purposes.  This will become
> > more important as we add pci express support, which has 4K config space.
> > 
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > Cc: Isaku Yamahata <address@hidden>
> > ---
> > 
> > Isaku Yamahata, I think with this change, you can
> > increase the size of config space to 4K without
> > need for helper functions. Makes sense?
> 
> It is good that the patch makes the function header size
> independent(256 or 4K).
> However, your patch just makes the code special.
> I think helper functions should be introduced eventually.
> With helper function, the implementation detail/logic will be encapsulated
> cleanly within them.

I think really most devices should be fine with range checking
address and length as they do now. Unfortunately they open-code
them, I think helpers that encapsulate range checks and
byte covered by range checks would be very good.
Note they aren't pci specific at all.

How about:
        ranges_overlap(ofset1, len1, ofset2, len2)
and
        range_covers_byte(ofset1, len1, ofset2))
?

OTOH the code *inside* pci is trying to implement most of the spec within
pci_default_config_write, so that devices do not have to worry.
So it does not really need to be encapsulated from itself.
So it is special, and the reason saving state is a good idea there
is that there is a lot of state.

Maybe we are being too smart.  Maybe any write into PCI header should
just trigger bar update routine. Then we would have a single range
check, and no memcpy, and it would work for both bridge and simple
device.  To try and scope the problem, need to count the number of
config writes that a typical OS boot does. Care to do this?

> When I tried to introduce callback claiming ad-hoc range check is fragile,
> you did suggest such helper functions would help.
> My helper function is so generic that each pci device can use
> and it will surely simplify them.

Only if they happen to only care about what you programmed in.  For
example there are already two kinds with size and without one, that is a
sign that more is to come, when we care about bits being changed.
Another problem will happen when writes have side effects on config (e.g.
VPD).  Since init helper saves state, you have to be very careful to
call the "changed" callback before you do any other changes there.
There are also too many parameters, all of which are integers, so no type
checking and easy to misuse, hard to figure out what each parameter
does.

> With your patch, each pci device doesn't get any benefit.

So generally I think what everyone does is either
(pci itself):

memcpy(orig, dev->config + REGISTER);
.....
if (memcmp(orig, dev->config + REGISTER))
        value changed do something

Here we need two operations to see that something
has changed, but we do not need to worry about
address/length, and we know when value changed,
not just was written into.

or 
        if (ranges_overlap(addr, len, REGISTER, 2))
                value might have changed do something
        if (range_covers_byte(addr, len, REGISTER))
                value might have changed do something

Here there is one operation, on the other hand it only tells us register
was written into, and we have to worry about address and length.

It's actually pretty hard to simplify each of these further :)

> If your are opposing to range check claiming that we should stick
> to memcpy/memcmp and aren't opposing to helper functions,
> I'd like to rewrite the helper functions with memcpy/memcmp
> adding new member, orig, in PCIDevice.
> At least those helper functions should be generic enough such that
> they can be used by not only pci_default_write_config(), but also other
> pci devices. So whole configuration space would be copied.
> i.e. while 256 bytes or 4Kbytes.

I had difficulty defining what does "orig" mean.
Does it just mean "state before default config was called"?
If so it's better as local variable inside default config ...

> Although I don't see why range check is so bad as you claim,

It's not bad for a single register a typical device cares about.  pci
internally is different I think.


> since memcpy or range change isn't essential to me, I compromise.

I would suggest that we split the cleanup and the functional parts.
When there is code duplication in two places, it will be much easier to
see how we remove it by using helpers.



> thanks,
> 
> > 
> >  hw/pci.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 41e99a9..5986937 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -541,11 +541,11 @@ uint32_t pci_default_read_config(PCIDevice *d,
> >  
> >  void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, 
> > int l)
> >  {
> > -    uint8_t orig[PCI_CONFIG_SPACE_SIZE];
> > +    uint8_t orig[PCI_CONFIG_HEADER_SIZE];
> >      int i;
> >  
> >      /* not efficient, but simple */
> > -    memcpy(orig, d->config, PCI_CONFIG_SPACE_SIZE);
> > +    memcpy(orig, d->config, sizeof orig);
> >      for(i = 0; i < l && addr < PCI_CONFIG_SPACE_SIZE; val >>= 8, ++i, 
> > ++addr) {
> >          uint8_t wmask = d->wmask[addr];
> >          d->config[addr] = (d->config[addr] & ~wmask) | (val & wmask);
> 
> -- 
> yamahata