* Re: [Qemu-devel] [PATCH] pseries: Limit PCI host bridge "index" value
2015-01-14 2:33 [Qemu-devel] [PATCH] pseries: Limit PCI host bridge "index" value David Gibson
@ 2015-01-14 12:10 ` Paolo Bonzini
2015-01-14 17:23 ` Michael Roth
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2015-01-14 12:10 UTC (permalink / raw)
To: David Gibson, aik, mdroth, agraf; +Cc: qemu-ppc, qemu-devel
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>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] pseries: Limit PCI host bridge "index" value
2015-01-14 2:33 [Qemu-devel] [PATCH] pseries: Limit PCI host bridge "index" value David Gibson
2015-01-14 12:10 ` Paolo Bonzini
@ 2015-01-14 17:23 ` Michael Roth
2015-01-15 1:38 ` David Gibson
2015-01-29 0:37 ` David Gibson
2015-01-29 13:12 ` Alexander Graf
3 siblings, 1 reply; 6+ messages in thread
From: Michael Roth @ 2015-01-14 17:23 UTC (permalink / raw)
To: David Gibson, aik, mdroth, agraf; +Cc: pbonzini, qemu-ppc, qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] pseries: Limit PCI host bridge "index" value
2015-01-14 17:23 ` Michael Roth
@ 2015-01-15 1:38 ` David Gibson
0 siblings, 0 replies; 6+ messages in thread
From: David Gibson @ 2015-01-15 1:38 UTC (permalink / raw)
To: Michael Roth; +Cc: aik, agraf, qemu-devel, qemu-ppc, mdroth, pbonzini
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] pseries: Limit PCI host bridge "index" value
2015-01-14 2:33 [Qemu-devel] [PATCH] pseries: Limit PCI host bridge "index" value David Gibson
2015-01-14 12:10 ` Paolo Bonzini
2015-01-14 17:23 ` Michael Roth
@ 2015-01-29 0:37 ` David Gibson
2015-01-29 13:12 ` Alexander Graf
3 siblings, 0 replies; 6+ messages in thread
From: David Gibson @ 2015-01-29 0:37 UTC (permalink / raw)
To: aik, mdroth, agraf; +Cc: pbonzini, qemu-ppc, qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] pseries: Limit PCI host bridge "index" value
2015-01-14 2:33 [Qemu-devel] [PATCH] pseries: Limit PCI host bridge "index" value David Gibson
` (2 preceding siblings ...)
2015-01-29 0:37 ` David Gibson
@ 2015-01-29 13:12 ` Alexander Graf
3 siblings, 0 replies; 6+ messages in thread
From: Alexander Graf @ 2015-01-29 13:12 UTC (permalink / raw)
To: David Gibson, aik, mdroth; +Cc: pbonzini, qemu-ppc, qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread