[Qemu-devel] [PATCH] pseries: Limit PCI host bridge "index" value

[Qemu-devel] [PATCH] pseries: Limit PCI host bridge "index" value

[David Gibson]

pseries guests can have large numbers of PCI host bridges.  To avoid the
user having to specify a number of different configuration values for every
one, the device supports an "index" property which is a shorthand setting
the various window and configuration addresses from a predefined sensible
set.

There are some problems with the details at present:
  * The "index" propery is signed, but negative values will create PCI
windows below where we expect, potentially colliding with other devices
  * No limit is imposed on the "index" property and large values can
translate to extremely large window addresses.  With PCI passthrough in
particular this can mean we exceed various mapping and physical address
limits causing the guest host bridge to not work in strange ways.

This patch addresses this, by making "index" unsigned, and imposing a
limit.  Currently the limit allows indices from 0..255 which is probably
enough host bridges for the time being.  It's fairly easy to extend if
we discover we need more.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c          | 8 +++++++-
 include/hw/pci-host/spapr.h | 4 +++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 21b95b3..6deeb19 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -501,6 +501,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
             return;
         }
 
+        if (sphb->index > SPAPR_PCI_MAX_INDEX) {
+            error_setg(errp, "\"index\" for PAPR PHB is too large (max %u)",
+                       SPAPR_PCI_MAX_INDEX);
+            return;
+        }
+
         sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index;
         sphb->dma_liobn = SPAPR_PCI_BASE_LIOBN + sphb->index;
 
@@ -669,7 +675,7 @@ static void spapr_phb_reset(DeviceState *qdev)
 }
 
 static Property spapr_phb_properties[] = {
-    DEFINE_PROP_INT32("index", sPAPRPHBState, index, -1),
+    DEFINE_PROP_UINT32("index", sPAPRPHBState, index, -1),
     DEFINE_PROP_UINT64("buid", sPAPRPHBState, buid, -1),
     DEFINE_PROP_UINT32("liobn", sPAPRPHBState, dma_liobn, -1),
     DEFINE_PROP_UINT64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 4ea2a0d..876ecf0 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -64,7 +64,7 @@ typedef struct spapr_pci_msi_mig {
 struct sPAPRPHBState {
     PCIHostState parent_obj;
 
-    int32_t index;
+    uint32_t index;
     uint64_t buid;
     char *dtbusname;
 
@@ -94,6 +94,8 @@ struct sPAPRPHBVFIOState {
     int32_t iommugroupid;
 };
 
+#define SPAPR_PCI_MAX_INDEX          255
+
 #define SPAPR_PCI_BASE_BUID          0x800000020000000ULL
 
 #define SPAPR_PCI_WINDOW_BASE        0x10000000000ULL
-- 
2.1.0

Re: [Qemu-devel] [PATCH] pseries: Limit PCI host bridge "index" value

[Paolo Bonzini]

On 14/01/2015 03:33, David Gibson wrote:
> pseries guests can have large numbers of PCI host bridges.  To avoid the
> user having to specify a number of different configuration values for every
> one, the device supports an "index" property which is a shorthand setting
> the various window and configuration addresses from a predefined sensible
> set.
> 
> There are some problems with the details at present:
>   * The "index" propery is signed, but negative values will create PCI
> windows below where we expect, potentially colliding with other devices
>   * No limit is imposed on the "index" property and large values can
> translate to extremely large window addresses.  With PCI passthrough in
> particular this can mean we exceed various mapping and physical address
> limits causing the guest host bridge to not work in strange ways.
> 
> This patch addresses this, by making "index" unsigned, and imposing a
> limit.  Currently the limit allows indices from 0..255 which is probably
> enough host bridges for the time being.  It's fairly easy to extend if
> we discover we need more.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_pci.c          | 8 +++++++-
>  include/hw/pci-host/spapr.h | 4 +++-
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 21b95b3..6deeb19 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -501,6 +501,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>              return;
>          }
>  
> +        if (sphb->index > SPAPR_PCI_MAX_INDEX) {
> +            error_setg(errp, "\"index\" for PAPR PHB is too large (max %u)",
> +                       SPAPR_PCI_MAX_INDEX);
> +            return;
> +        }
> +
>          sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index;
>          sphb->dma_liobn = SPAPR_PCI_BASE_LIOBN + sphb->index;
>  
> @@ -669,7 +675,7 @@ static void spapr_phb_reset(DeviceState *qdev)
>  }
>  
>  static Property spapr_phb_properties[] = {
> -    DEFINE_PROP_INT32("index", sPAPRPHBState, index, -1),
> +    DEFINE_PROP_UINT32("index", sPAPRPHBState, index, -1),
>      DEFINE_PROP_UINT64("buid", sPAPRPHBState, buid, -1),
>      DEFINE_PROP_UINT32("liobn", sPAPRPHBState, dma_liobn, -1),
>      DEFINE_PROP_UINT64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 4ea2a0d..876ecf0 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -64,7 +64,7 @@ typedef struct spapr_pci_msi_mig {
>  struct sPAPRPHBState {
>      PCIHostState parent_obj;
>  
> -    int32_t index;
> +    uint32_t index;
>      uint64_t buid;
>      char *dtbusname;
>  
> @@ -94,6 +94,8 @@ struct sPAPRPHBVFIOState {
>      int32_t iommugroupid;
>  };
>  
> +#define SPAPR_PCI_MAX_INDEX          255
> +
>  #define SPAPR_PCI_BASE_BUID          0x800000020000000ULL
>  
>  #define SPAPR_PCI_WINDOW_BASE        0x10000000000ULL
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH] pseries: Limit PCI host bridge "index" value

[Michael Roth]

Quoting David Gibson (2015-01-13 20:33:39)
> pseries guests can have large numbers of PCI host bridges.  To avoid the
> user having to specify a number of different configuration values for every
> one, the device supports an "index" property which is a shorthand setting
> the various window and configuration addresses from a predefined sensible
> set.
> 
> There are some problems with the details at present:
>   * The "index" propery is signed, but negative values will create PCI
> windows below where we expect, potentially colliding with other devices
>   * No limit is imposed on the "index" property and large values can
> translate to extremely large window addresses.  With PCI passthrough in
> particular this can mean we exceed various mapping and physical address
> limits causing the guest host bridge to not work in strange ways.
> 
> This patch addresses this, by making "index" unsigned, and imposing a
> limit.  Currently the limit allows indices from 0..255 which is probably
> enough host bridges for the time being.  It's fairly easy to extend if
> we discover we need more.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

I think the limit makes sense, but since the check isn't triggered in cases
where 'index' isn't specified and '[io,mem]_win_[size,offset]' are set
explicitly, maybe it makes sense to sanity-check the final calculation for
those values as well? We could actually drop the index limit in that case
(if we decided we wanted to).

But I think it's okay to assume such users know what they're doing in the
meantime, so:

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  hw/ppc/spapr_pci.c          | 8 +++++++-
>  include/hw/pci-host/spapr.h | 4 +++-
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 21b95b3..6deeb19 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -501,6 +501,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>              return;
>          }
> 
> +        if (sphb->index > SPAPR_PCI_MAX_INDEX) {
> +            error_setg(errp, "\"index\" for PAPR PHB is too large (max %u)",
> +                       SPAPR_PCI_MAX_INDEX);
> +            return;
> +        }
> +
>          sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index;
>          sphb->dma_liobn = SPAPR_PCI_BASE_LIOBN + sphb->index;
> 
> @@ -669,7 +675,7 @@ static void spapr_phb_reset(DeviceState *qdev)
>  }
> 
>  static Property spapr_phb_properties[] = {
> -    DEFINE_PROP_INT32("index", sPAPRPHBState, index, -1),
> +    DEFINE_PROP_UINT32("index", sPAPRPHBState, index, -1),
>      DEFINE_PROP_UINT64("buid", sPAPRPHBState, buid, -1),
>      DEFINE_PROP_UINT32("liobn", sPAPRPHBState, dma_liobn, -1),
>      DEFINE_PROP_UINT64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 4ea2a0d..876ecf0 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -64,7 +64,7 @@ typedef struct spapr_pci_msi_mig {
>  struct sPAPRPHBState {
>      PCIHostState parent_obj;
> 
> -    int32_t index;
> +    uint32_t index;
>      uint64_t buid;
>      char *dtbusname;
> 
> @@ -94,6 +94,8 @@ struct sPAPRPHBVFIOState {
>      int32_t iommugroupid;
>  };
> 
> +#define SPAPR_PCI_MAX_INDEX          255
> +
>  #define SPAPR_PCI_BASE_BUID          0x800000020000000ULL
> 
>  #define SPAPR_PCI_WINDOW_BASE        0x10000000000ULL
> -- 
> 2.1.0

Re: [Qemu-devel] [PATCH] pseries: Limit PCI host bridge "index" value

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2099 bytes --]

On Wed, Jan 14, 2015 at 11:23:10AM -0600, Michael Roth wrote:
> Quoting David Gibson (2015-01-13 20:33:39)
> > pseries guests can have large numbers of PCI host bridges.  To avoid the
> > user having to specify a number of different configuration values for every
> > one, the device supports an "index" property which is a shorthand setting
> > the various window and configuration addresses from a predefined sensible
> > set.
> > 
> > There are some problems with the details at present:
> >   * The "index" propery is signed, but negative values will create PCI
> > windows below where we expect, potentially colliding with other devices
> >   * No limit is imposed on the "index" property and large values can
> > translate to extremely large window addresses.  With PCI passthrough in
> > particular this can mean we exceed various mapping and physical address
> > limits causing the guest host bridge to not work in strange ways.
> > 
> > This patch addresses this, by making "index" unsigned, and imposing a
> > limit.  Currently the limit allows indices from 0..255 which is probably
> > enough host bridges for the time being.  It's fairly easy to extend if
> > we discover we need more.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> I think the limit makes sense, but since the check isn't triggered in cases
> where 'index' isn't specified and '[io,mem]_win_[size,offset]' are set
> explicitly, maybe it makes sense to sanity-check the final calculation for
> those values as well? We could actually drop the index limit in that case
> (if we decided we wanted to).
> 
> But I think it's okay to assume such users know what they're doing in the
> meantime, so:

Yeah, that was my assumption, that if you're explicitly setting the
windows, then you know what you're doing.

> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] pseries: Limit PCI host bridge "index" value

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3348 bytes --]

Ping agraf,

Any word on merging this?

On Wed, Jan 14, 2015 at 01:33:39PM +1100, David Gibson wrote:
> pseries guests can have large numbers of PCI host bridges.  To avoid the
> user having to specify a number of different configuration values for every
> one, the device supports an "index" property which is a shorthand setting
> the various window and configuration addresses from a predefined sensible
> set.
> 
> There are some problems with the details at present:
>   * The "index" propery is signed, but negative values will create PCI
> windows below where we expect, potentially colliding with other devices
>   * No limit is imposed on the "index" property and large values can
> translate to extremely large window addresses.  With PCI passthrough in
> particular this can mean we exceed various mapping and physical address
> limits causing the guest host bridge to not work in strange ways.
> 
> This patch addresses this, by making "index" unsigned, and imposing a
> limit.  Currently the limit allows indices from 0..255 which is probably
> enough host bridges for the time being.  It's fairly easy to extend if
> we discover we need more.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_pci.c          | 8 +++++++-
>  include/hw/pci-host/spapr.h | 4 +++-
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 21b95b3..6deeb19 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -501,6 +501,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>              return;
>          }
>  
> +        if (sphb->index > SPAPR_PCI_MAX_INDEX) {
> +            error_setg(errp, "\"index\" for PAPR PHB is too large (max %u)",
> +                       SPAPR_PCI_MAX_INDEX);
> +            return;
> +        }
> +
>          sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index;
>          sphb->dma_liobn = SPAPR_PCI_BASE_LIOBN + sphb->index;
>  
> @@ -669,7 +675,7 @@ static void spapr_phb_reset(DeviceState *qdev)
>  }
>  
>  static Property spapr_phb_properties[] = {
> -    DEFINE_PROP_INT32("index", sPAPRPHBState, index, -1),
> +    DEFINE_PROP_UINT32("index", sPAPRPHBState, index, -1),
>      DEFINE_PROP_UINT64("buid", sPAPRPHBState, buid, -1),
>      DEFINE_PROP_UINT32("liobn", sPAPRPHBState, dma_liobn, -1),
>      DEFINE_PROP_UINT64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 4ea2a0d..876ecf0 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -64,7 +64,7 @@ typedef struct spapr_pci_msi_mig {
>  struct sPAPRPHBState {
>      PCIHostState parent_obj;
>  
> -    int32_t index;
> +    uint32_t index;
>      uint64_t buid;
>      char *dtbusname;
>  
> @@ -94,6 +94,8 @@ struct sPAPRPHBVFIOState {
>      int32_t iommugroupid;
>  };
>  
> +#define SPAPR_PCI_MAX_INDEX          255
> +
>  #define SPAPR_PCI_BASE_BUID          0x800000020000000ULL
>  
>  #define SPAPR_PCI_WINDOW_BASE        0x10000000000ULL

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] pseries: Limit PCI host bridge "index" value

[Alexander Graf]

On 14.01.15 03:33, David Gibson wrote:
> pseries guests can have large numbers of PCI host bridges.  To avoid the
> user having to specify a number of different configuration values for every
> one, the device supports an "index" property which is a shorthand setting
> the various window and configuration addresses from a predefined sensible
> set.
> 
> There are some problems with the details at present:
>   * The "index" propery is signed, but negative values will create PCI
> windows below where we expect, potentially colliding with other devices
>   * No limit is imposed on the "index" property and large values can
> translate to extremely large window addresses.  With PCI passthrough in
> particular this can mean we exceed various mapping and physical address
> limits causing the guest host bridge to not work in strange ways.
> 
> This patch addresses this, by making "index" unsigned, and imposing a
> limit.  Currently the limit allows indices from 0..255 which is probably
> enough host bridges for the time being.  It's fairly easy to extend if
> we discover we need more.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Thanks, applied to ppc-next.


Alex