Patchwork Re: [PATCH 4/8] pci: Replace used bitmap with capability byte map

login
register
mail settings
Submitter Michael S. Tsirkin
Date Nov. 12, 2010, 9:02 a.m.
Message ID <20101112090230.GF7631@redhat.com>
Download mbox | patch
Permalink /patch/70942/
State New
Headers show

Comments

Michael S. Tsirkin - Nov. 12, 2010, 9:02 a.m.
On Thu, Nov 11, 2010 at 11:07:15PM -0700, Alex Williamson wrote:
> On Fri, 2010-11-12 at 07:40 +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 11, 2010 at 07:55:43PM -0700, Alex Williamson wrote:
> > > Capabilities are allocated in bytes, so we can track both whether
> > > a byte is used and by what capability in the same structure.
> > > 
> > > Remove pci_reserve_capability() as there are no users.
> > > 
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > 
> > I actually wanted to remove the used array completely and ask
> > all users to add offsets directly.
> > Will this be needed then?
> 
> Can you give an example, I don't understand what you mean by asking
> users to add offsets directly.

Here's a dump of patch in progress.
Something like the below (untested).  After applying this we can remove
the whole used array allocator.

>  I think some kind of tracking what's
> where in config space needs to be done somewhere and the common PCI code
> seems like it'd be the place.

Why do we need it? config lets us scan the capability list
readily enough. I had this idea that we should pack
capabilities in config space, but now I think it's silly.

>  Thanks,
> 
> Alex

pci: remove config space allocator

pci supports allocating caps in config space so they are packed tightly.
This doesn't seem to be useful, especially since caps must stay at fixed
offsets to ensure backwards compatibility (e.g. for migration).

Remove this support, and make virtio-pci supply the offset to
the MSIX capability explicitly.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Alex Williamson - Nov. 12, 2010, 3:32 p.m.
On Fri, 2010-11-12 at 11:02 +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 11, 2010 at 11:07:15PM -0700, Alex Williamson wrote:
> > On Fri, 2010-11-12 at 07:40 +0200, Michael S. Tsirkin wrote:
> > > On Thu, Nov 11, 2010 at 07:55:43PM -0700, Alex Williamson wrote:
> > > > Capabilities are allocated in bytes, so we can track both whether
> > > > a byte is used and by what capability in the same structure.
> > > > 
> > > > Remove pci_reserve_capability() as there are no users.
> > > > 
> > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > 
> > > I actually wanted to remove the used array completely and ask
> > > all users to add offsets directly.
> > > Will this be needed then?
> > 
> > Can you give an example, I don't understand what you mean by asking
> > users to add offsets directly.
> 
> Here's a dump of patch in progress.
> Something like the below (untested).  After applying this we can remove
> the whole used array allocator.
> 
> >  I think some kind of tracking what's
> > where in config space needs to be done somewhere and the common PCI code
> > seems like it'd be the place.
> 
> Why do we need it? config lets us scan the capability list
> readily enough. I had this idea that we should pack
> capabilities in config space, but now I think it's silly.
> 
> >  Thanks,
> > 
> > Alex
> 
> pci: remove config space allocator
> 
> pci supports allocating caps in config space so they are packed tightly.
> This doesn't seem to be useful, especially since caps must stay at fixed
> offsets to ensure backwards compatibility (e.g. for migration).
> 
> Remove this support, and make virtio-pci supply the offset to
> the MSIX capability explicitly.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

So add_capability effectively becomes the add_capability_at_offset that
we have in the qemu-kvm tree.  I think that's fine, but doesn't
necessarily support removing the cap_map.  A direct config offset to
capability id map is pretty useful for device assignment, which only
uses add_capability_at_offset after these patches.  We might also want
to use it to check drivers to make sure they're not overlapping caps.
For instance in the below, we now have core virtio code forcing an
offset for the msix cap.  What if the driver already placed something at
that offset, or what if the driver wants to add capability foo, it
should at least error if it tries to place it over something else in the
list.

Alex

> diff --git a/hw/msix.c b/hw/msix.c
> index f66d255..6fd3791 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -52,7 +52,7 @@ int msix_supported;
>   * Original bar size must be a power of 2 or 0.
>   * New bar size is returned. */
>  static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
> -                           unsigned bar_nr, unsigned bar_size)
> +                           unsigned offset, unsigned bar_nr, unsigned bar_size)
>  {
>      int config_offset;
>      uint8_t *config;
> @@ -75,7 +75,7 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
>  
>      pdev->msix_bar_size = new_size;
>      config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX,
> -                                       0, MSIX_CAP_LENGTH);
> +                                       offset, MSIX_CAP_LENGTH);
>      if (config_offset < 0)
>          return config_offset;
>      config = pdev->config + config_offset;
> @@ -237,7 +237,7 @@ static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
>  /* Initialize the MSI-X structures. Note: if MSI-X is supported, BAR size is
>   * modified, it should be retrieved with msix_bar_size. */
>  int msix_init(struct PCIDevice *dev, unsigned short nentries,
> -              unsigned bar_nr, unsigned bar_size)
> +              unsigned offset, unsigned bar_nr, unsigned bar_size)
>  {
>      int ret;
>      /* Nothing to do if MSI is not supported by interrupt controller */
> @@ -261,7 +261,7 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>      }
>  
>      dev->msix_entries_nr = nentries;
> -    ret = msix_add_config(dev, nentries, bar_nr, bar_size);
> +    ret = msix_add_config(dev, nentries, offset, bar_nr, bar_size);
>      if (ret)
>          goto err_config;
>  
> diff --git a/hw/msix.h b/hw/msix.h
> index a9f7993..b61e42e 100644
> --- a/hw/msix.h
> +++ b/hw/msix.h
> @@ -5,7 +5,7 @@
>  #include "pci.h"
>  
>  int msix_init(PCIDevice *pdev, unsigned short nentries,
> -              unsigned bar_nr, unsigned bar_size);
> +              unsigned offset, unsigned bar_nr, unsigned bar_size);
>  
>  void msix_write_config(PCIDevice *pci_dev, uint32_t address,
>                         uint32_t val, int len);
> diff --git a/hw/pci.c b/hw/pci.c
> index aed2d42..e411f12 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1737,12 +1737,7 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
>                         uint8_t offset, uint8_t size)
>  {
>      uint8_t *config;
> -    if (!offset) {
> -        offset = pci_find_space(pdev, size);
> -        if (!offset) {
> -            return -ENOSPC;
> -        }
> -    }
> +    assert(offset >= PCI_CONFIG_HEADER_SIZE);
>  
>      config = pdev->config + offset;
>      config[PCI_CAP_LIST_ID] = cap_id;
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 729917d..bcb0266 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -543,7 +543,8 @@ static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
>  
>      config[0x3d] = 1;
>  
> -    if (vdev->nvectors && !msix_init(&proxy->pci_dev, vdev->nvectors, 1, 0)) {
> +    if (vdev->nvectors && !msix_init(&proxy->pci_dev, vdev->nvectors,
> +                                     PCI_CONFIG_HEADER_SIZE, 1, 0)) {
>          pci_register_bar(&proxy->pci_dev, 1,
>                           msix_bar_size(&proxy->pci_dev),
>                           PCI_BASE_ADDRESS_SPACE_MEMORY,

Patch

diff --git a/hw/msix.c b/hw/msix.c
index f66d255..6fd3791 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -52,7 +52,7 @@  int msix_supported;
  * Original bar size must be a power of 2 or 0.
  * New bar size is returned. */
 static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
-                           unsigned bar_nr, unsigned bar_size)
+                           unsigned offset, unsigned bar_nr, unsigned bar_size)
 {
     int config_offset;
     uint8_t *config;
@@ -75,7 +75,7 @@  static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries,
 
     pdev->msix_bar_size = new_size;
     config_offset = pci_add_capability(pdev, PCI_CAP_ID_MSIX,
-                                       0, MSIX_CAP_LENGTH);
+                                       offset, MSIX_CAP_LENGTH);
     if (config_offset < 0)
         return config_offset;
     config = pdev->config + config_offset;
@@ -237,7 +237,7 @@  static void msix_mask_all(struct PCIDevice *dev, unsigned nentries)
 /* Initialize the MSI-X structures. Note: if MSI-X is supported, BAR size is
  * modified, it should be retrieved with msix_bar_size. */
 int msix_init(struct PCIDevice *dev, unsigned short nentries,
-              unsigned bar_nr, unsigned bar_size)
+              unsigned offset, unsigned bar_nr, unsigned bar_size)
 {
     int ret;
     /* Nothing to do if MSI is not supported by interrupt controller */
@@ -261,7 +261,7 @@  int msix_init(struct PCIDevice *dev, unsigned short nentries,
     }
 
     dev->msix_entries_nr = nentries;
-    ret = msix_add_config(dev, nentries, bar_nr, bar_size);
+    ret = msix_add_config(dev, nentries, offset, bar_nr, bar_size);
     if (ret)
         goto err_config;
 
diff --git a/hw/msix.h b/hw/msix.h
index a9f7993..b61e42e 100644
--- a/hw/msix.h
+++ b/hw/msix.h
@@ -5,7 +5,7 @@ 
 #include "pci.h"
 
 int msix_init(PCIDevice *pdev, unsigned short nentries,
-              unsigned bar_nr, unsigned bar_size);
+              unsigned offset, unsigned bar_nr, unsigned bar_size);
 
 void msix_write_config(PCIDevice *pci_dev, uint32_t address,
                        uint32_t val, int len);
diff --git a/hw/pci.c b/hw/pci.c
index aed2d42..e411f12 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1737,12 +1737,7 @@  int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
                        uint8_t offset, uint8_t size)
 {
     uint8_t *config;
-    if (!offset) {
-        offset = pci_find_space(pdev, size);
-        if (!offset) {
-            return -ENOSPC;
-        }
-    }
+    assert(offset >= PCI_CONFIG_HEADER_SIZE);
 
     config = pdev->config + offset;
     config[PCI_CAP_LIST_ID] = cap_id;
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 729917d..bcb0266 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -543,7 +543,8 @@  static void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev,
 
     config[0x3d] = 1;
 
-    if (vdev->nvectors && !msix_init(&proxy->pci_dev, vdev->nvectors, 1, 0)) {
+    if (vdev->nvectors && !msix_init(&proxy->pci_dev, vdev->nvectors,
+                                     PCI_CONFIG_HEADER_SIZE, 1, 0)) {
         pci_register_bar(&proxy->pci_dev, 1,
                          msix_bar_size(&proxy->pci_dev),
                          PCI_BASE_ADDRESS_SPACE_MEMORY,