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@xxxxxxxxxx> > > > > --- > > > > 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 _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/virtualization
