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

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Jun 10, 2009 at 11:55:40AM -0300, Glauber Costa wrote:
> 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?

I think so, yes.

> 
> 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" ?

Right. So I want to load an image and it has capability X at offset Y.
wmask has to match. I don't want to assume that we never change Y
for the device without breaking old images, so I clear wmask here
and set it up again after looking up capabilities that I loaded.

Maybe this explanation should go into the comment above?

-- 
MST