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 <address@hidden>

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 <address@hidden>

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