Re: [Qemu-devel] [PATCHv3 03/13] qemu: add routines to manage PCI capabilities

[Glauber Costa <glommer@redhat.com>]

On Wed, Jun 10, 2009 at 12:54:15PM +0300, Michael S. Tsirkin wrote:
> On Tue, Jun 09, 2009 at 02:11:14PM -0300, Glauber Costa wrote:
> > On Fri, Jun 05, 2009 at 01:23:15PM +0300, Michael S. Tsirkin wrote:
> > > Add routines to manage PCI capability list. First user will be MSI-X.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  hw/pci.c |   98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
> > >  hw/pci.h |   18 +++++++++++-
> > >  2 files changed, 106 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/hw/pci.c b/hw/pci.c
> > > index 361d741..ed011b5 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -130,12 +130,13 @@ void pci_device_save(PCIDevice *s, QEMUFile *f)
> > >      int version = s->cap_present ? 3 : 2;
> > >      int i;
> > >  
> > > -    qemu_put_be32(f, version); /* PCI device version */
> > > +    /* PCI device version and capabilities */
> > > +    qemu_put_be32(f, version);
> > > +    if (version >= 3)
> > > +        qemu_put_be32(f, s->cap_present);
> > >      qemu_put_buffer(f, s->config, 256);
> > >      for (i = 0; i < 4; i++)
> > >          qemu_put_be32(f, s->irq_state[i]);
> > > -    if (version >= 3)
> > > -        qemu_put_be32(f, s->cap_present);
> > >  }
> > What is it doing here?
> > You should just do it right in the first patch, instead of doing in
> > one way there, and fixing here.
> > 
> > >  
> > >  int pci_device_load(PCIDevice *s, QEMUFile *f)
> > > @@ -146,12 +147,6 @@ int pci_device_load(PCIDevice *s, QEMUFile *f)
> > >      version_id = qemu_get_be32(f);
> > >      if (version_id > 3)
> > >          return -EINVAL;
> > > -    qemu_get_buffer(f, s->config, 256);
> > > -    pci_update_mappings(s);
> > > -
> > > -    if (version_id >= 2)
> > > -        for (i = 0; i < 4; i ++)
> > > -            s->irq_state[i] = qemu_get_be32(f);
> > >      if (version_id >= 3)
> > >          s->cap_present = qemu_get_be32(f);
> > >      else
> > ditto.
> > > @@ -160,6 +155,18 @@ int pci_device_load(PCIDevice *s, QEMUFile *f)
> > >      if (s->cap_present & ~s->cap_supported)
> > >          return -EINVAL;
> > >  
> > > +    qemu_get_buffer(f, s->config, 256);
> > > +    pci_update_mappings(s);
> > > +
> > > +    if (version_id >= 2)
> > > +        for (i = 0; i < 4; i ++)
> > > +            s->irq_state[i] = qemu_get_be32(f);
> > > +    /* Clear wmask and used bits for capabilities.
> > > +       Must be restored separately, since capabilities can
> > > +       be placed anywhere in config space. */
> > > +    memset(s->used, 0, PCI_CONFIG_SPACE_SIZE);
> > > +    for (i = PCI_CONFIG_HEADER_SIZE; i < PCI_CONFIG_SPACE_SIZE; ++i)
> > > +        s->wmask[i] = 0xff;
> > >      return 0;
> > >  }
> > Sorry, I don't exactly understand it. Although it can be anywhere, what do we actually
> > lose by keeping it at the same place in config space?
> 
> We lose the ability to let user control the capabilities exposed
> by the device.
> 
> And generally, I dislike arbitrary limitations. The PCI spec says the
> capability can be anywhere, implementing a linked list of caps is simple
> enough to not invent abritrary restrictions.
yes, but this is migration time, right?

caps can be anywhere, but we don't expect it to change during machine execution
lifetime.

Or I am just confused by the name "pci_device_load" ?