Message ID | 1455790054-1952-4-git-send-email-leonid.bloch@ravellosystems.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Thu, Feb 18, 2016 at 12:07:24PM +0200, Leonid Bloch wrote: > From: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com> > A bit of text here to explain what's different from the existing APIs. > Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com> > Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com> > --- > hw/pci/pcie.c | 80 +++++++++++++++++++++++++++++++++++----------- > include/hw/pci/pcie.h | 4 +++ > include/hw/pci/pcie_regs.h | 5 +-- > 3 files changed, 69 insertions(+), 20 deletions(-) > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index 435a6cf..be3a318 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -42,26 +42,15 @@ > /*************************************************************************** > * pci express capability helper functions > */ > -int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port) > -{ > - int pos; > - uint8_t *exp_cap; > - > - assert(pci_is_express(dev)); > - > - pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, > - PCI_EXP_VER2_SIZEOF); > - if (pos < 0) { > - return pos; > - } > - dev->exp.exp_cap = pos; > - exp_cap = dev->config + pos; > > +static void > +_pcie_cap_v1_fill(uint8_t *exp_cap, uint8_t port, uint8_t type, uint8_t version) Names are rather confusing. There's init, there's fill, there's a prefix and a non prefix versions ... Please add some comments for each function, saying what it does. Also this one is really both for v1 and v2, correct? Don't use _ prefix in QEMU please. > +{ > /* capability register > - interrupt message number defaults to 0 */ > + interrupt message number defaults to 0 */ > pci_set_word(exp_cap + PCI_EXP_FLAGS, > ((type << PCI_EXP_FLAGS_TYPE_SHIFT) & PCI_EXP_FLAGS_TYPE) | > - PCI_EXP_FLAGS_VER2); > + version); > > /* device capability register > * table 7-12: > @@ -80,6 +69,23 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port) > > pci_set_word(exp_cap + PCI_EXP_LNKSTA, > PCI_EXP_LNK_MLW_1 | PCI_EXP_LNK_LS_25 |PCI_EXP_LNKSTA_DLLLA); > +} > + > +int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port) > +{ > + int pos; > + uint8_t *exp_cap; > + > + assert(pci_is_express(dev)); > + > + pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, PCI_EXP_VER2_SIZEOF); > + if (pos < 0) { > + return pos; > + } > + dev->exp.exp_cap = pos; > + exp_cap = dev->config + pos; > + > + _pcie_cap_v1_fill(exp_cap, port, type, PCI_EXP_FLAGS_VER2); > > pci_set_long(exp_cap + PCI_EXP_DEVCAP2, > PCI_EXP_DEVCAP2_EFF | PCI_EXP_DEVCAP2_EETLPP); > @@ -88,7 +94,28 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port) > return pos; > } > > -int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset) > +int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port) > +{ > + int pos; > + uint8_t *exp_cap; > + > + assert(pci_is_express(dev)); > + > + pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, > + PCI_EXP_VER1_SIZEOF); > + if (pos < 0) { > + return pos; > + } > + dev->exp.exp_cap = pos; > + exp_cap = dev->config + pos; > + > + _pcie_cap_v1_fill(exp_cap, port, type, PCI_EXP_FLAGS_VER1); > + > + return pos; > +} > + > +static int > +_pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset, uint8_t cap_size) > { > uint8_t type = PCI_EXP_TYPE_ENDPOINT; > > @@ -101,7 +128,19 @@ int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset) > type = PCI_EXP_TYPE_RC_END; > } > > - return pcie_cap_init(dev, offset, type, 0); > + return (cap_size == PCI_EXP_VER1_SIZEOF) > + ? pcie_cap_v1_init(dev, offset, type, 0) > + : pcie_cap_init(dev, offset, type, 0); > +} > + > +int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset) > +{ > + return _pcie_endpoint_cap_init(dev, offset, PCI_EXP_VER2_SIZEOF); > +} > + > +int pcie_endpoint_cap_v1_init(PCIDevice *dev, uint8_t offset) > +{ > + return _pcie_endpoint_cap_init(dev, offset, PCI_EXP_VER1_SIZEOF); > } > > void pcie_cap_exit(PCIDevice *dev) > @@ -109,6 +148,11 @@ void pcie_cap_exit(PCIDevice *dev) > pci_del_capability(dev, PCI_CAP_ID_EXP, PCI_EXP_VER2_SIZEOF); > } > > +void pcie_cap_v1_exit(PCIDevice *dev) > +{ > + pci_del_capability(dev, PCI_CAP_ID_EXP, PCI_EXP_VER1_SIZEOF); > +} > + > uint8_t pcie_cap_get_type(const PCIDevice *dev) > { > uint32_t pos = dev->exp.exp_cap; > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h > index b48a7a2..cbbf0c5 100644 > --- a/include/hw/pci/pcie.h > +++ b/include/hw/pci/pcie.h > @@ -80,8 +80,12 @@ struct PCIExpressDevice { > > /* PCI express capability helper functions */ > int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port); > +int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset, > + uint8_t type, uint8_t port); > int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset); > void pcie_cap_exit(PCIDevice *dev); > +int pcie_endpoint_cap_v1_init(PCIDevice *dev, uint8_t offset); > +void pcie_cap_v1_exit(PCIDevice *dev); > uint8_t pcie_cap_get_type(const PCIDevice *dev); > void pcie_cap_flags_set_vector(PCIDevice *dev, uint8_t vector); > uint8_t pcie_cap_flags_get_vector(PCIDevice *dev); > diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h > index 6a28b33..a95522a 100644 > --- a/include/hw/pci/pcie_regs.h > +++ b/include/hw/pci/pcie_regs.h > @@ -11,6 +11,7 @@ > > /* express capability */ > > +#define PCI_EXP_VER1_SIZEOF 0x14 /* express capability of ver. 1 */ > #define PCI_EXP_VER2_SIZEOF 0x3c /* express capability of ver. 2 */ > #define PCI_EXT_CAP_VER_SHIFT 16 > #define PCI_EXT_CAP_NEXT_SHIFT 20 > @@ -26,11 +27,11 @@ > (((x) + PCI_EXT_CAP_ALIGN - 1) & ~(PCI_EXT_CAP_ALIGN - 1)) > > /* PCI_EXP_FLAGS */ > -#define PCI_EXP_FLAGS_VER2 2 /* for now, supports only ver. 2 */ > +#define PCI_EXP_FLAGS_VER1 1 > +#define PCI_EXP_FLAGS_VER2 2 > #define PCI_EXP_FLAGS_IRQ_SHIFT ctz32(PCI_EXP_FLAGS_IRQ) > #define PCI_EXP_FLAGS_TYPE_SHIFT ctz32(PCI_EXP_FLAGS_TYPE) > > - > /* PCI_EXP_LINK{CAP, STA} */ > /* link speed */ > #define PCI_EXP_LNK_LS_25 1 > -- > 2.5.0
On Thu, Feb 18, 2016 at 12:35 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Thu, Feb 18, 2016 at 12:07:24PM +0200, Leonid Bloch wrote: >> From: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com> >> > > A bit of text here to explain what's different > from the existing APIs. > >> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com> >> Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com> >> --- >> hw/pci/pcie.c | 80 +++++++++++++++++++++++++++++++++++----------- >> include/hw/pci/pcie.h | 4 +++ >> include/hw/pci/pcie_regs.h | 5 +-- >> 3 files changed, 69 insertions(+), 20 deletions(-) >> >> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c >> index 435a6cf..be3a318 100644 >> --- a/hw/pci/pcie.c >> +++ b/hw/pci/pcie.c >> @@ -42,26 +42,15 @@ >> /*************************************************************************** >> * pci express capability helper functions >> */ >> -int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port) >> -{ >> - int pos; >> - uint8_t *exp_cap; >> - >> - assert(pci_is_express(dev)); >> - >> - pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, >> - PCI_EXP_VER2_SIZEOF); >> - if (pos < 0) { >> - return pos; >> - } >> - dev->exp.exp_cap = pos; >> - exp_cap = dev->config + pos; >> >> +static void >> +_pcie_cap_v1_fill(uint8_t *exp_cap, uint8_t port, uint8_t type, uint8_t version) > > Names are rather confusing. There's init, there's fill, there's > a prefix and a non prefix versions ... > Please add some comments for each function, saying what it does. Thanks, the naming was changed in v2. Some comments were added as well. > > Also this one is really both for v1 and v2, correct? This one fills the v1 values, and the values common with v1 in v2. > > Don't use _ prefix in QEMU please. Thanks, got rid of leading underscores in this patch, and in the others as well. > >> +{ >> /* capability register >> - interrupt message number defaults to 0 */ >> + interrupt message number defaults to 0 */ >> pci_set_word(exp_cap + PCI_EXP_FLAGS, >> ((type << PCI_EXP_FLAGS_TYPE_SHIFT) & PCI_EXP_FLAGS_TYPE) | >> - PCI_EXP_FLAGS_VER2); >> + version); >> >> /* device capability register >> * table 7-12: >> @@ -80,6 +69,23 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port) >> >> pci_set_word(exp_cap + PCI_EXP_LNKSTA, >> PCI_EXP_LNK_MLW_1 | PCI_EXP_LNK_LS_25 |PCI_EXP_LNKSTA_DLLLA); >> +} >> + >> +int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port) >> +{ >> + int pos; >> + uint8_t *exp_cap; >> + >> + assert(pci_is_express(dev)); >> + >> + pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, PCI_EXP_VER2_SIZEOF); >> + if (pos < 0) { >> + return pos; >> + } >> + dev->exp.exp_cap = pos; >> + exp_cap = dev->config + pos; >> + >> + _pcie_cap_v1_fill(exp_cap, port, type, PCI_EXP_FLAGS_VER2); >> >> pci_set_long(exp_cap + PCI_EXP_DEVCAP2, >> PCI_EXP_DEVCAP2_EFF | PCI_EXP_DEVCAP2_EETLPP); >> @@ -88,7 +94,28 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port) >> return pos; >> } >> >> -int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset) >> +int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port) >> +{ >> + int pos; >> + uint8_t *exp_cap; >> + >> + assert(pci_is_express(dev)); >> + >> + pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, >> + PCI_EXP_VER1_SIZEOF); >> + if (pos < 0) { >> + return pos; >> + } >> + dev->exp.exp_cap = pos; >> + exp_cap = dev->config + pos; >> + >> + _pcie_cap_v1_fill(exp_cap, port, type, PCI_EXP_FLAGS_VER1); >> + >> + return pos; >> +} >> + >> +static int >> +_pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset, uint8_t cap_size) >> { >> uint8_t type = PCI_EXP_TYPE_ENDPOINT; >> >> @@ -101,7 +128,19 @@ int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset) >> type = PCI_EXP_TYPE_RC_END; >> } >> >> - return pcie_cap_init(dev, offset, type, 0); >> + return (cap_size == PCI_EXP_VER1_SIZEOF) >> + ? pcie_cap_v1_init(dev, offset, type, 0) >> + : pcie_cap_init(dev, offset, type, 0); >> +} >> + >> +int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset) >> +{ >> + return _pcie_endpoint_cap_init(dev, offset, PCI_EXP_VER2_SIZEOF); >> +} >> + >> +int pcie_endpoint_cap_v1_init(PCIDevice *dev, uint8_t offset) >> +{ >> + return _pcie_endpoint_cap_init(dev, offset, PCI_EXP_VER1_SIZEOF); >> } >> >> void pcie_cap_exit(PCIDevice *dev) >> @@ -109,6 +148,11 @@ void pcie_cap_exit(PCIDevice *dev) >> pci_del_capability(dev, PCI_CAP_ID_EXP, PCI_EXP_VER2_SIZEOF); >> } >> >> +void pcie_cap_v1_exit(PCIDevice *dev) >> +{ >> + pci_del_capability(dev, PCI_CAP_ID_EXP, PCI_EXP_VER1_SIZEOF); >> +} >> + >> uint8_t pcie_cap_get_type(const PCIDevice *dev) >> { >> uint32_t pos = dev->exp.exp_cap; >> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h >> index b48a7a2..cbbf0c5 100644 >> --- a/include/hw/pci/pcie.h >> +++ b/include/hw/pci/pcie.h >> @@ -80,8 +80,12 @@ struct PCIExpressDevice { >> >> /* PCI express capability helper functions */ >> int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port); >> +int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset, >> + uint8_t type, uint8_t port); >> int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset); >> void pcie_cap_exit(PCIDevice *dev); >> +int pcie_endpoint_cap_v1_init(PCIDevice *dev, uint8_t offset); >> +void pcie_cap_v1_exit(PCIDevice *dev); >> uint8_t pcie_cap_get_type(const PCIDevice *dev); >> void pcie_cap_flags_set_vector(PCIDevice *dev, uint8_t vector); >> uint8_t pcie_cap_flags_get_vector(PCIDevice *dev); >> diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h >> index 6a28b33..a95522a 100644 >> --- a/include/hw/pci/pcie_regs.h >> +++ b/include/hw/pci/pcie_regs.h >> @@ -11,6 +11,7 @@ >> >> /* express capability */ >> >> +#define PCI_EXP_VER1_SIZEOF 0x14 /* express capability of ver. 1 */ >> #define PCI_EXP_VER2_SIZEOF 0x3c /* express capability of ver. 2 */ >> #define PCI_EXT_CAP_VER_SHIFT 16 >> #define PCI_EXT_CAP_NEXT_SHIFT 20 >> @@ -26,11 +27,11 @@ >> (((x) + PCI_EXT_CAP_ALIGN - 1) & ~(PCI_EXT_CAP_ALIGN - 1)) >> >> /* PCI_EXP_FLAGS */ >> -#define PCI_EXP_FLAGS_VER2 2 /* for now, supports only ver. 2 */ >> +#define PCI_EXP_FLAGS_VER1 1 >> +#define PCI_EXP_FLAGS_VER2 2 >> #define PCI_EXP_FLAGS_IRQ_SHIFT ctz32(PCI_EXP_FLAGS_IRQ) >> #define PCI_EXP_FLAGS_TYPE_SHIFT ctz32(PCI_EXP_FLAGS_TYPE) >> >> - >> /* PCI_EXP_LINK{CAP, STA} */ >> /* link speed */ >> #define PCI_EXP_LNK_LS_25 1 >> -- >> 2.5.0
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index 435a6cf..be3a318 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -42,26 +42,15 @@ /*************************************************************************** * pci express capability helper functions */ -int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port) -{ - int pos; - uint8_t *exp_cap; - - assert(pci_is_express(dev)); - - pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, - PCI_EXP_VER2_SIZEOF); - if (pos < 0) { - return pos; - } - dev->exp.exp_cap = pos; - exp_cap = dev->config + pos; +static void +_pcie_cap_v1_fill(uint8_t *exp_cap, uint8_t port, uint8_t type, uint8_t version) +{ /* capability register - interrupt message number defaults to 0 */ + interrupt message number defaults to 0 */ pci_set_word(exp_cap + PCI_EXP_FLAGS, ((type << PCI_EXP_FLAGS_TYPE_SHIFT) & PCI_EXP_FLAGS_TYPE) | - PCI_EXP_FLAGS_VER2); + version); /* device capability register * table 7-12: @@ -80,6 +69,23 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port) pci_set_word(exp_cap + PCI_EXP_LNKSTA, PCI_EXP_LNK_MLW_1 | PCI_EXP_LNK_LS_25 |PCI_EXP_LNKSTA_DLLLA); +} + +int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port) +{ + int pos; + uint8_t *exp_cap; + + assert(pci_is_express(dev)); + + pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, PCI_EXP_VER2_SIZEOF); + if (pos < 0) { + return pos; + } + dev->exp.exp_cap = pos; + exp_cap = dev->config + pos; + + _pcie_cap_v1_fill(exp_cap, port, type, PCI_EXP_FLAGS_VER2); pci_set_long(exp_cap + PCI_EXP_DEVCAP2, PCI_EXP_DEVCAP2_EFF | PCI_EXP_DEVCAP2_EETLPP); @@ -88,7 +94,28 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port) return pos; } -int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset) +int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port) +{ + int pos; + uint8_t *exp_cap; + + assert(pci_is_express(dev)); + + pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset, + PCI_EXP_VER1_SIZEOF); + if (pos < 0) { + return pos; + } + dev->exp.exp_cap = pos; + exp_cap = dev->config + pos; + + _pcie_cap_v1_fill(exp_cap, port, type, PCI_EXP_FLAGS_VER1); + + return pos; +} + +static int +_pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset, uint8_t cap_size) { uint8_t type = PCI_EXP_TYPE_ENDPOINT; @@ -101,7 +128,19 @@ int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset) type = PCI_EXP_TYPE_RC_END; } - return pcie_cap_init(dev, offset, type, 0); + return (cap_size == PCI_EXP_VER1_SIZEOF) + ? pcie_cap_v1_init(dev, offset, type, 0) + : pcie_cap_init(dev, offset, type, 0); +} + +int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset) +{ + return _pcie_endpoint_cap_init(dev, offset, PCI_EXP_VER2_SIZEOF); +} + +int pcie_endpoint_cap_v1_init(PCIDevice *dev, uint8_t offset) +{ + return _pcie_endpoint_cap_init(dev, offset, PCI_EXP_VER1_SIZEOF); } void pcie_cap_exit(PCIDevice *dev) @@ -109,6 +148,11 @@ void pcie_cap_exit(PCIDevice *dev) pci_del_capability(dev, PCI_CAP_ID_EXP, PCI_EXP_VER2_SIZEOF); } +void pcie_cap_v1_exit(PCIDevice *dev) +{ + pci_del_capability(dev, PCI_CAP_ID_EXP, PCI_EXP_VER1_SIZEOF); +} + uint8_t pcie_cap_get_type(const PCIDevice *dev) { uint32_t pos = dev->exp.exp_cap; diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h index b48a7a2..cbbf0c5 100644 --- a/include/hw/pci/pcie.h +++ b/include/hw/pci/pcie.h @@ -80,8 +80,12 @@ struct PCIExpressDevice { /* PCI express capability helper functions */ int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type, uint8_t port); +int pcie_cap_v1_init(PCIDevice *dev, uint8_t offset, + uint8_t type, uint8_t port); int pcie_endpoint_cap_init(PCIDevice *dev, uint8_t offset); void pcie_cap_exit(PCIDevice *dev); +int pcie_endpoint_cap_v1_init(PCIDevice *dev, uint8_t offset); +void pcie_cap_v1_exit(PCIDevice *dev); uint8_t pcie_cap_get_type(const PCIDevice *dev); void pcie_cap_flags_set_vector(PCIDevice *dev, uint8_t vector); uint8_t pcie_cap_flags_get_vector(PCIDevice *dev); diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h index 6a28b33..a95522a 100644 --- a/include/hw/pci/pcie_regs.h +++ b/include/hw/pci/pcie_regs.h @@ -11,6 +11,7 @@ /* express capability */ +#define PCI_EXP_VER1_SIZEOF 0x14 /* express capability of ver. 1 */ #define PCI_EXP_VER2_SIZEOF 0x3c /* express capability of ver. 2 */ #define PCI_EXT_CAP_VER_SHIFT 16 #define PCI_EXT_CAP_NEXT_SHIFT 20 @@ -26,11 +27,11 @@ (((x) + PCI_EXT_CAP_ALIGN - 1) & ~(PCI_EXT_CAP_ALIGN - 1)) /* PCI_EXP_FLAGS */ -#define PCI_EXP_FLAGS_VER2 2 /* for now, supports only ver. 2 */ +#define PCI_EXP_FLAGS_VER1 1 +#define PCI_EXP_FLAGS_VER2 2 #define PCI_EXP_FLAGS_IRQ_SHIFT ctz32(PCI_EXP_FLAGS_IRQ) #define PCI_EXP_FLAGS_TYPE_SHIFT ctz32(PCI_EXP_FLAGS_TYPE) - /* PCI_EXP_LINK{CAP, STA} */ /* link speed */ #define PCI_EXP_LNK_LS_25 1