Re: [Qemu-devel] [PATCH v5 15/16] spapr_pci: enable basic hotplug operations

[David Gibson]

On Tue, Feb 24, 2015 at 11:17:24PM -0600, Michael Roth wrote:
> Quoting David Gibson (2015-02-24 21:11:29)
> > On Mon, Feb 16, 2015 at 08:27:51AM -0600, Michael Roth wrote:
> > > This enables hotplug for PHB bridges. Upon hotplug we generate the
> > 
> > "PCI Host Bridge bridges" :-p
> > 
> > > OF-nodes required by PAPR specification and IEEE 1275-1994
> > > "PCI Bus Binding to Open Firmware" for the device.
> > > 
> > > We associate the corresponding FDT for these nodes with the DrcEntry
> > > corresponding to the slot, which will be fetched via
> > > ibm,configure-connector RTAS calls by the guest as described by PAPR
> > > specification. The FDT is cleaned up in the case of unplug.
> > > 
> > > Signed-off-by: Michael Roth <address@hidden>
> > > ---
> > >  hw/ppc/spapr_pci.c | 391 
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 371 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > > index 6a3d917..b9af1cd 100644
> > > --- a/hw/ppc/spapr_pci.c
> > > +++ b/hw/ppc/spapr_pci.c
> > > @@ -33,8 +33,11 @@
> > >  #include <libfdt.h>
> > >  #include "trace.h"
> > >  #include "qemu/error-report.h"
> > > +#include "qapi/qmp/qerror.h"
> > >  
> > >  #include "hw/pci/pci_bus.h"
> > > +#include "hw/ppc/spapr_drc.h"
> > > +#include "sysemu/device_tree.h"
> > >  
> > >  /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */
> > >  #define RTAS_QUERY_FN           0
> > > @@ -47,7 +50,13 @@
> > >  #define RTAS_TYPE_MSI           1
> > >  #define RTAS_TYPE_MSIX          2
> > >  
> > > -#include "hw/ppc/spapr_drc.h"
> > > +#define _FDT(exp) \
> > > +    do { \
> > > +        int ret = (exp);                                           \
> > > +        if (ret < 0) {                                             \
> > > +            return ret;                                            \
> > > +        }                                                          \
> > > +    } while (0)
> > >  
> > >  static sPAPRPHBState *find_phb(sPAPREnvironment *spapr, uint64_t buid)
> > >  {
> > > @@ -481,6 +490,359 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus 
> > > *bus, void *opaque, int devfn)
> > >      return &phb->iommu_as;
> > >  }
> > >  
> > > +/* Macros to operate with address in OF binding to PCI */
> > > +#define b_x(x, p, l)    (((x) & ((1<<(l))-1)) << (p))
> > > +#define b_n(x)          b_x((x), 31, 1) /* 0 if relocatable */
> > > +#define b_p(x)          b_x((x), 30, 1) /* 1 if prefetchable */
> > > +#define b_t(x)          b_x((x), 29, 1) /* 1 if the address is aliased */
> > > +#define b_ss(x)         b_x((x), 24, 2) /* the space code */
> > > +#define b_bbbbbbbb(x)   b_x((x), 16, 8) /* bus number */
> > > +#define b_ddddd(x)      b_x((x), 11, 5) /* device number */
> > > +#define b_fff(x)        b_x((x), 8, 3)  /* function number */
> > > +#define b_rrrrrrrr(x)   b_x((x), 0, 8)  /* register number */
> > > +/* for 'reg'/'assigned-addresses' OF properties */
> > > +#define RESOURCE_CELLS_SIZE 2
> > > +#define RESOURCE_CELLS_ADDRESS 3
> > > +
> > > +typedef struct ResourceFields {
> > > +    uint32_t phys_hi;
> > > +    uint32_t phys_mid;
> > > +    uint32_t phys_lo;
> > > +    uint32_t size_hi;
> > > +    uint32_t size_lo;
> > > +} ResourceFields;
> > 
> > Hrm, 5*32-bit ints, that probably needs a ((packed)) on at least some
> > platforms to be safe.
> 
> I seem to remember some rule (C99?) about padding only being used in cases
> where an n-byte field doesn't naturally fall on an n-byte boundary, but
> it doesn't hurt to be safe/explicit about what we're expecting.

So, the potential problem is end-of-structure padding rather than
between-field padding.  I know some platforms (ARMs I think) at least
used to round structures out to 8 bytes, even if all the members were
less than that.  Mind you at the time, I believe ((packed)) didn't
actually fix it, causing headaches with nested structures.

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

pgpuw1RuI1fXY.pgp
Description: PGP signature