[Qemu-devel] Re: [PATCH 1/8] pci: pci_default_cap_write_config ignores wmask

[Alex Williamson]

On Fri, 2010-11-12 at 07:22 +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 11, 2010 at 07:55:01PM -0700, Alex Williamson wrote:
> > Make use of wmask, just like the rest of config space.
> > 
> > Signed-off-by: Alex Williamson <address@hidden>
> > ---
> > 
> >  hw/pci.c |   19 ++++++++-----------
> >  1 files changed, 8 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 92aaa85..12c47ac 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -1175,13 +1175,14 @@ uint32_t pci_default_read_config(PCIDevice *d,
> >      return pci_read_config(d, address, len);
> >  }
> >  
> > -static void pci_write_config(PCIDevice *pci_dev,
> > -                             uint32_t address, uint32_t val, int len)
> > +static void pci_write_config(PCIDevice *d, uint32_t addr, uint32_t val, 
> > int l)
> >  {
> >      int i;
> > -    for (i = 0; i < len; i++) {
> > -        pci_dev->config[address + i] = val & 0xff;
> > -        val >>= 8;
> > +    uint32_t config_size = pci_config_size(d);
> > +
> > +    for (i = 0; i < l && addr + i < config_size; val >>= 8, ++i) {
> > +        uint8_t wmask = d->wmask[addr + i];
> > +        d->config[addr + i] = (d->config[addr + i] & ~wmask) | (val & 
> > wmask);
> >      }
> >  }
> >
> 
> 
> Let's not name an internal static helper pci_write_config.
> This is really update_config_by_mask or something like that.
> But see below: maybe we don't need it at all?

The function already exists, I just made it do what it seems like it
should have been doing already.

> > @@ -1207,18 +1208,14 @@ void pci_default_cap_write_config(PCIDevice 
> > *pci_dev,
> >  
> >  void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, 
> > int l)
> >  {
> > -    int i, was_irq_disabled = pci_irq_disabled(d);
> > -    uint32_t config_size = pci_config_size(d);
> > +    int was_irq_disabled = pci_irq_disabled(d);
> >  
> >      if (pci_access_cap_config(d, addr, l)) {
> >          d->cap.config_write(d, addr, val, l);
> >          return;
> >      }
> >  
> 
> I would like to also examine the need for _cap_
> functions. Why can assigned devices just do
> 
>       pci_default_write_config 
>       if (range_overlap(...msi)) {
>       }
>       if (range_overlap(...msix)) {
>       }
> and then we could remove all the _cap_ extensions
> altogether?

I think that somewhere we need to track what capabilities are at what
offset, config space isn't a performance path, but that look horribly
inefficient and gets worse with more capabilities.

Why don't we define capability id 0xff as normal config space (first 64
bytes), then add the capability id to read/write_config (this is what
vfio does).  Then the driver can split capability handling off from
their main functions if they want.  Anyway, I think such an improvement
could be added incrementally later.  Thanks,

Alex

> > -    for (i = 0; i < l && addr + i < config_size; val >>= 8, ++i) {
> > -        uint8_t wmask = d->wmask[addr + i];
> > -        d->config[addr + i] = (d->config[addr + i] & ~wmask) | (val & 
> > wmask);
> > -    }
> > +    pci_write_config(d, addr, val, l);
> >  
> >  #ifdef CONFIG_KVM_DEVICE_ASSIGNMENT
> >      if (kvm_enabled() && kvm_irqchip_in_kernel() &&