[for-4.0,v3,3/9] qapi: Define PCIe link speed and width properties

Create properties to be able to define speeds and widths for PCIe
links.  The only tricky bit here is that our get and set callbacks
translate from the fixed QAPI automagic enums to those we define
in PCI code to represent the actual register segment value.

Cc: Eric Blake <eblake@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Tested-by: Geoffrey McRae <geoff@hostfission.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/core/qdev-properties.c    |  178 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/qdev-properties.h |    8 ++
 qapi/common.json             |   42 ++++++++++
 3 files changed, 228 insertions(+)

Alex Williamson <alex.williamson@redhat.com> writes:

> Create properties to be able to define speeds and widths for PCIe
> links.  The only tricky bit here is that our get and set callbacks
> translate from the fixed QAPI automagic enums to those we define
> in PCI code to represent the actual register segment value.

QAPI can only generate enumerations with values 0, 1, 2, ...  You want
different enumeration values, namely the actual register values.  You
still want QAPI to get its standard mapping to and from strings.

This patch's solution is to define a non-QAPI enumeration type with the
values you want [PATCH 1/9], then map between the enumerations in the
PropertyInfo methods.  Works.

You could instead use the encoding chosen by QAPI for the properties,
and map it to the register values on use.  Differently ugly.  Might be
simpler.  Your choice to make.

We could extend QAPI to permit specification of the enumeration values.
Marc-André's work to permit conditionals makes the syntax flexible
enough to support that.  Of course, adding QAPI features is worthwhile
only if we get sufficient mileage out of them to result in an overall
improvement.  Even if we decided to do it right now, I'd recommend not
to wait for it, but instead plan to simplify after the feature lands.

> Cc: Eric Blake <eblake@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Tested-by: Geoffrey McRae <geoff@hostfission.com>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Hi Alex,

On 12/4/18 5:26 PM, Alex Williamson wrote:
> Create properties to be able to define speeds and widths for PCIe
> links.  The only tricky bit here is that our get and set callbacks
> translate from the fixed QAPI automagic enums to those we define
> in PCI code to represent the actual register segment value.
> 
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Tested-by: Geoffrey McRae <geoff@hostfission.com>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  hw/core/qdev-properties.c    |  178 ++++++++++++++++++++++++++++++++++++++++++
>  include/hw/qdev-properties.h |    8 ++
>  qapi/common.json             |   42 ++++++++++
>  3 files changed, 228 insertions(+)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 35072dec1ecf..f5ca5b821a79 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1327,3 +1327,181 @@ const PropertyInfo qdev_prop_off_auto_pcibar = {
>      .set = set_enum,
>      .set_default_value = set_default_value_enum,
>  };
> +
> +/* --- PCIELinkSpeed 2_5/5/8/16 -- */
> +
> +static void get_prop_pcielinkspeed(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    PCIExpLinkSpeed *p = qdev_get_prop_ptr(dev, prop);
> +    PCIELinkSpeed speed;
> +
> +    switch (*p) {
> +    case QEMU_PCI_EXP_LNK_2_5GT:
> +        speed = PCIE_LINK_SPEED_2_5;
> +        break;
> +    case QEMU_PCI_EXP_LNK_5GT:
> +        speed = PCIE_LINK_SPEED_5;
> +        break;
> +    case QEMU_PCI_EXP_LNK_8GT:
> +        speed = PCIE_LINK_SPEED_8;
> +        break;
> +    case QEMU_PCI_EXP_LNK_16GT:
> +        speed = PCIE_LINK_SPEED_16;
> +        break;
> +    default:
> +        /* Unreachable */
> +        abort();
nit: g_assert_not_reached() here and below.
> +    }
> +
> +    visit_type_enum(v, prop->name, (int *)&speed, prop->info->enum_table, errp);
> +}
> +
> +static void set_prop_pcielinkspeed(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    PCIExpLinkSpeed *p = qdev_get_prop_ptr(dev, prop);
> +    PCIELinkSpeed speed;
> +    Error *local_err = NULL;
> +
> +    if (dev->realized) {
> +        qdev_prop_set_after_realize(dev, name, errp);
> +        return;
> +    }
> +
> +    visit_type_enum(v, prop->name, (int *)&speed,
> +                    prop->info->enum_table, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    switch (speed) {
> +    case PCIE_LINK_SPEED_2_5:
> +        *p = QEMU_PCI_EXP_LNK_2_5GT;
> +        break;
> +    case PCIE_LINK_SPEED_5:
> +        *p = QEMU_PCI_EXP_LNK_5GT;
> +        break;
> +    case PCIE_LINK_SPEED_8:
> +        *p = QEMU_PCI_EXP_LNK_8GT;
> +        break;
> +    case PCIE_LINK_SPEED_16:
> +        *p = QEMU_PCI_EXP_LNK_16GT;
> +        break;
> +    default:
> +        /* Unreachable */
> +        abort();
> +    }
> +}
> +
> +const PropertyInfo qdev_prop_pcie_link_speed = {
> +    .name = "PCIELinkSpeed",
> +    .description = "2_5/5/8/16",
> +    .enum_table = &PCIELinkSpeed_lookup,
> +    .get = get_prop_pcielinkspeed,
> +    .set = set_prop_pcielinkspeed,
> +    .set_default_value = set_default_value_enum,
> +};
> +
> +/* --- PCIELinkWidth 1/2/4/8/12/16/32 -- */
> +
> +static void get_prop_pcielinkwidth(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    PCIExpLinkWidth *p = qdev_get_prop_ptr(dev, prop);
> +    PCIELinkWidth width;
> +
> +    switch (*p) {
> +    case QEMU_PCI_EXP_LNK_X1:
> +        width = PCIE_LINK_WIDTH_1;
> +        break;
> +    case QEMU_PCI_EXP_LNK_X2:
> +        width = PCIE_LINK_WIDTH_2;
> +        break;
> +    case QEMU_PCI_EXP_LNK_X4:
> +        width = PCIE_LINK_WIDTH_4;
> +        break;
> +    case QEMU_PCI_EXP_LNK_X8:
> +        width = PCIE_LINK_WIDTH_8;
> +        break;
> +    case QEMU_PCI_EXP_LNK_X12:
> +        width = PCIE_LINK_WIDTH_12;
> +        break;
> +    case QEMU_PCI_EXP_LNK_X16:
> +        width = PCIE_LINK_WIDTH_16;
> +        break;
> +    case QEMU_PCI_EXP_LNK_X32:
> +        width = PCIE_LINK_WIDTH_32;
> +        break;
> +    default:
> +        /* Unreachable */
> +        abort();
> +    }
> +
> +    visit_type_enum(v, prop->name, (int *)&width, prop->info->enum_table, errp);
> +}
> +
> +static void set_prop_pcielinkwidth(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    PCIExpLinkWidth *p = qdev_get_prop_ptr(dev, prop);
> +    PCIELinkWidth width;
> +    Error *local_err = NULL;
> +
> +    if (dev->realized) {
> +        qdev_prop_set_after_realize(dev, name, errp);
> +        return;
> +    }
> +
> +    visit_type_enum(v, prop->name, (int *)&width,
> +                    prop->info->enum_table, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    switch (width) {
> +    case PCIE_LINK_WIDTH_1:
> +        *p = QEMU_PCI_EXP_LNK_X1;
> +        break;
> +    case PCIE_LINK_WIDTH_2:
> +        *p = QEMU_PCI_EXP_LNK_X2;
> +        break;
> +    case PCIE_LINK_WIDTH_4:
> +        *p = QEMU_PCI_EXP_LNK_X4;
> +        break;
> +    case PCIE_LINK_WIDTH_8:
> +        *p = QEMU_PCI_EXP_LNK_X8;
> +        break;
> +    case PCIE_LINK_WIDTH_12:
> +        *p = QEMU_PCI_EXP_LNK_X12;
> +        break;
> +    case PCIE_LINK_WIDTH_16:
> +        *p = QEMU_PCI_EXP_LNK_X16;
> +        break;
> +    case PCIE_LINK_WIDTH_32:
> +        *p = QEMU_PCI_EXP_LNK_X32;
> +        break;
> +    default:
> +        /* Unreachable */
> +        abort();
> +    }
> +}
> +
> +const PropertyInfo qdev_prop_pcie_link_width = {
> +    .name = "PCIELinkWidth",
> +    .description = "1/2/4/8/12/16/32",
> +    .enum_table = &PCIELinkWidth_lookup,
> +    .get = get_prop_pcielinkwidth,
> +    .set = set_prop_pcielinkwidth,
> +    .set_default_value = set_default_value_enum,
> +};
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 4f60cc88f325..6a13a284c48c 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -36,6 +36,8 @@ extern const PropertyInfo qdev_prop_uuid;
>  extern const PropertyInfo qdev_prop_arraylen;
>  extern const PropertyInfo qdev_prop_link;
>  extern const PropertyInfo qdev_prop_off_auto_pcibar;
> +extern const PropertyInfo qdev_prop_pcie_link_speed;
> +extern const PropertyInfo qdev_prop_pcie_link_width;
>  
>  #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
>          .name      = (_name),                                    \
> @@ -217,6 +219,12 @@ extern const PropertyInfo qdev_prop_off_auto_pcibar;
>  #define DEFINE_PROP_OFF_AUTO_PCIBAR(_n, _s, _f, _d) \
>      DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_off_auto_pcibar, \
>                          OffAutoPCIBAR)
> +#define DEFINE_PROP_PCIE_LINK_SPEED(_n, _s, _f, _d) \
> +    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pcie_link_speed, \
> +                        PCIExpLinkSpeed)
> +#define DEFINE_PROP_PCIE_LINK_WIDTH(_n, _s, _f, _d) \
> +    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pcie_link_width, \
> +                        PCIExpLinkWidth)
>  
>  #define DEFINE_PROP_UUID(_name, _state, _field) {                  \
>          .name      = (_name),                                      \
> diff --git a/qapi/common.json b/qapi/common.json
> index 021174f04ea4..b6f3cca35c7e 100644
> --- a/qapi/common.json
> +++ b/qapi/common.json
> @@ -127,6 +127,48 @@
>  { 'enum': 'OffAutoPCIBAR',
>    'data': [ 'off', 'auto', 'bar0', 'bar1', 'bar2', 'bar3', 'bar4', 'bar5' ] }
>  
> +##
> +# @PCIELinkSpeed:
> +#
> +# An enumeration of PCIe link speeds in units of GT/s
> +#
> +# @2_5: 2.5GT/s
> +#
> +# @5: 5.0GT/s
> +#
> +# @8: 8.0GT/s
> +#
> +# @16: 16.0GT/s
> +#
> +# Since: 3.2
4.0 here and below

Thanks

Eric
> +##
> +{ 'enum': 'PCIELinkSpeed',
> +  'data': [ '2_5', '5', '8', '16' ] }
> +
> +##
> +# @PCIELinkWidth:
> +#
> +# An enumeration of PCIe link width
> +#
> +# @1: x1
> +#
> +# @2: x2
> +#
> +# @4: x4
> +#
> +# @8: x8
> +#
> +# @12: x12
> +#
> +# @16: x16
> +#
> +# @32: x32
> +#
> +# Since: 3.2
> +##
> +{ 'enum': 'PCIELinkWidth',
> +  'data': [ '1', '2', '4', '8', '12', '16', '32' ] }
> +
>  ##
>  # @SysEmuTarget:
>  #
> 
>

Auger Eric <eric.auger@redhat.com> writes:

> Hi Alex,
>
> On 12/4/18 5:26 PM, Alex Williamson wrote:
>> Create properties to be able to define speeds and widths for PCIe
>> links.  The only tricky bit here is that our get and set callbacks
>> translate from the fixed QAPI automagic enums to those we define
>> in PCI code to represent the actual register segment value.
>> 
>> Cc: Eric Blake <eblake@redhat.com>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Tested-by: Geoffrey McRae <geoff@hostfission.com>
>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>> ---
>>  hw/core/qdev-properties.c    |  178 ++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/qdev-properties.h |    8 ++
>>  qapi/common.json             |   42 ++++++++++
>>  3 files changed, 228 insertions(+)
>> 
>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>> index 35072dec1ecf..f5ca5b821a79 100644
>> --- a/hw/core/qdev-properties.c
>> +++ b/hw/core/qdev-properties.c
>> @@ -1327,3 +1327,181 @@ const PropertyInfo qdev_prop_off_auto_pcibar = {
>>      .set = set_enum,
>>      .set_default_value = set_default_value_enum,
>>  };
>> +
>> +/* --- PCIELinkSpeed 2_5/5/8/16 -- */
>> +
>> +static void get_prop_pcielinkspeed(Object *obj, Visitor *v, const char *name,
>> +                                   void *opaque, Error **errp)
>> +{
>> +    DeviceState *dev = DEVICE(obj);
>> +    Property *prop = opaque;
>> +    PCIExpLinkSpeed *p = qdev_get_prop_ptr(dev, prop);
>> +    PCIELinkSpeed speed;
>> +
>> +    switch (*p) {
>> +    case QEMU_PCI_EXP_LNK_2_5GT:
>> +        speed = PCIE_LINK_SPEED_2_5;
>> +        break;
>> +    case QEMU_PCI_EXP_LNK_5GT:
>> +        speed = PCIE_LINK_SPEED_5;
>> +        break;
>> +    case QEMU_PCI_EXP_LNK_8GT:
>> +        speed = PCIE_LINK_SPEED_8;
>> +        break;
>> +    case QEMU_PCI_EXP_LNK_16GT:
>> +        speed = PCIE_LINK_SPEED_16;
>> +        break;
>> +    default:
>> +        /* Unreachable */
>> +        abort();
> nit: g_assert_not_reached() here and below.

In my opinion, g_assert_not_reached() & friends are an overly ornate
reinvention of an old and perfectly adequate wheel.

A long time ago for reasons since forgotten, the maintainers in charge
back then demanded abort() instead of assert(0).  Either is fine with
me.

I tolerate g_assert_not_reached() in files that already use g_assert().
This one doesn't.

In any case, I'd drop the comment.

Note that I'm not this file's maintainer.

[...]

Hi Markus,

On 12/5/18 3:16 PM, Markus Armbruster wrote:
> Auger Eric <eric.auger@redhat.com> writes:
> 
>> Hi Alex,
>>
>> On 12/4/18 5:26 PM, Alex Williamson wrote:
>>> Create properties to be able to define speeds and widths for PCIe
>>> links.  The only tricky bit here is that our get and set callbacks
>>> translate from the fixed QAPI automagic enums to those we define
>>> in PCI code to represent the actual register segment value.
>>>
>>> Cc: Eric Blake <eblake@redhat.com>
>>> Cc: Markus Armbruster <armbru@redhat.com>
>>> Tested-by: Geoffrey McRae <geoff@hostfission.com>
>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>> ---
>>>  hw/core/qdev-properties.c    |  178 ++++++++++++++++++++++++++++++++++++++++++
>>>  include/hw/qdev-properties.h |    8 ++
>>>  qapi/common.json             |   42 ++++++++++
>>>  3 files changed, 228 insertions(+)
>>>
>>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>>> index 35072dec1ecf..f5ca5b821a79 100644
>>> --- a/hw/core/qdev-properties.c
>>> +++ b/hw/core/qdev-properties.c
>>> @@ -1327,3 +1327,181 @@ const PropertyInfo qdev_prop_off_auto_pcibar = {
>>>      .set = set_enum,
>>>      .set_default_value = set_default_value_enum,
>>>  };
>>> +
>>> +/* --- PCIELinkSpeed 2_5/5/8/16 -- */
>>> +
>>> +static void get_prop_pcielinkspeed(Object *obj, Visitor *v, const char *name,
>>> +                                   void *opaque, Error **errp)
>>> +{
>>> +    DeviceState *dev = DEVICE(obj);
>>> +    Property *prop = opaque;
>>> +    PCIExpLinkSpeed *p = qdev_get_prop_ptr(dev, prop);
>>> +    PCIELinkSpeed speed;
>>> +
>>> +    switch (*p) {
>>> +    case QEMU_PCI_EXP_LNK_2_5GT:
>>> +        speed = PCIE_LINK_SPEED_2_5;
>>> +        break;
>>> +    case QEMU_PCI_EXP_LNK_5GT:
>>> +        speed = PCIE_LINK_SPEED_5;
>>> +        break;
>>> +    case QEMU_PCI_EXP_LNK_8GT:
>>> +        speed = PCIE_LINK_SPEED_8;
>>> +        break;
>>> +    case QEMU_PCI_EXP_LNK_16GT:
>>> +        speed = PCIE_LINK_SPEED_16;
>>> +        break;
>>> +    default:
>>> +        /* Unreachable */
>>> +        abort();
>> nit: g_assert_not_reached() here and below.
> 
> In my opinion, g_assert_not_reached() & friends are an overly ornate
> reinvention of an old and perfectly adequate wheel.
> 
> A long time ago for reasons since forgotten, the maintainers in charge
> back then demanded abort() instead of assert(0).  Either is fine with
> me.
> 
> I tolerate g_assert_not_reached() in files that already use g_assert().
> This one doesn't.
> 
> In any case, I'd drop the comment.

OK I did not know. In the past I was encouraged to use it.

Thanks

Eric
> 
> Note that I'm not this file's maintainer.
> 
> [...]
>

On Wed, 05 Dec 2018 10:09:38 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Alex Williamson <alex.williamson@redhat.com> writes:
> 
> > Create properties to be able to define speeds and widths for PCIe
> > links.  The only tricky bit here is that our get and set callbacks
> > translate from the fixed QAPI automagic enums to those we define
> > in PCI code to represent the actual register segment value.  
> 
> QAPI can only generate enumerations with values 0, 1, 2, ...  You want
> different enumeration values, namely the actual register values.  You
> still want QAPI to get its standard mapping to and from strings.
> 
> This patch's solution is to define a non-QAPI enumeration type with the
> values you want [PATCH 1/9], then map between the enumerations in the
> PropertyInfo methods.  Works.
> 
> You could instead use the encoding chosen by QAPI for the properties,
> and map it to the register values on use.  Differently ugly.  Might be
> simpler.  Your choice to make.

As we discussed offline, I personally don't like using the magic macros
that QAPI generates outside of QAPI code; I can't git grep for them and
I need to remember to build the tree before using cscope in order to
find them.  Therefore I prefer to keep the translation to standard
macros within the QAPI code to make things more obvious, thus the
approach chosen.  I appreciate the feedback and alternative ideas.

> We could extend QAPI to permit specification of the enumeration values.
> Marc-André's work to permit conditionals makes the syntax flexible
> enough to support that.  Of course, adding QAPI features is worthwhile
> only if we get sufficient mileage out of them to result in an overall
> improvement.  Even if we decided to do it right now, I'd recommend not
> to wait for it, but instead plan to simplify after the feature lands.

Good to know.

> > Cc: Eric Blake <eblake@redhat.com>
> > Cc: Markus Armbruster <armbru@redhat.com>
> > Tested-by: Geoffrey McRae <geoff@hostfission.com>
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>  
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks!

Alex

Auger Eric <eric.auger@redhat.com> writes:

> Hi Markus,
>
> On 12/5/18 3:16 PM, Markus Armbruster wrote:
>> Auger Eric <eric.auger@redhat.com> writes:
[...]
>>> nit: g_assert_not_reached() here and below.
>> 
>> In my opinion, g_assert_not_reached() & friends are an overly ornate
>> reinvention of an old and perfectly adequate wheel.
>> 
>> A long time ago for reasons since forgotten, the maintainers in charge
>> back then demanded abort() instead of assert(0).  Either is fine with
>> me.
>> 
>> I tolerate g_assert_not_reached() in files that already use g_assert().
>> This one doesn't.
>> 
>> In any case, I'd drop the comment.
>
> OK I did not know. In the past I was encouraged to use it.

Some maintainers like ornate ;)

> Thanks
>
> Eric
>> 
>> Note that I'm not this file's maintainer.
>> 
>> [...]

On Wed, 05 Dec 2018 15:16:27 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Auger Eric <eric.auger@redhat.com> writes:
> 
> > Hi Alex,
> >
> > On 12/4/18 5:26 PM, Alex Williamson wrote:  
> >> Create properties to be able to define speeds and widths for PCIe
> >> links.  The only tricky bit here is that our get and set callbacks
> >> translate from the fixed QAPI automagic enums to those we define
> >> in PCI code to represent the actual register segment value.
> >> 
> >> Cc: Eric Blake <eblake@redhat.com>
> >> Cc: Markus Armbruster <armbru@redhat.com>
> >> Tested-by: Geoffrey McRae <geoff@hostfission.com>
> >> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >> ---
> >>  hw/core/qdev-properties.c    |  178 ++++++++++++++++++++++++++++++++++++++++++
> >>  include/hw/qdev-properties.h |    8 ++
> >>  qapi/common.json             |   42 ++++++++++
> >>  3 files changed, 228 insertions(+)
> >> 
> >> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> >> index 35072dec1ecf..f5ca5b821a79 100644
> >> --- a/hw/core/qdev-properties.c
> >> +++ b/hw/core/qdev-properties.c
> >> @@ -1327,3 +1327,181 @@ const PropertyInfo qdev_prop_off_auto_pcibar = {
> >>      .set = set_enum,
> >>      .set_default_value = set_default_value_enum,
> >>  };
> >> +
> >> +/* --- PCIELinkSpeed 2_5/5/8/16 -- */
> >> +
> >> +static void get_prop_pcielinkspeed(Object *obj, Visitor *v, const char *name,
> >> +                                   void *opaque, Error **errp)
> >> +{
> >> +    DeviceState *dev = DEVICE(obj);
> >> +    Property *prop = opaque;
> >> +    PCIExpLinkSpeed *p = qdev_get_prop_ptr(dev, prop);
> >> +    PCIELinkSpeed speed;
> >> +
> >> +    switch (*p) {
> >> +    case QEMU_PCI_EXP_LNK_2_5GT:
> >> +        speed = PCIE_LINK_SPEED_2_5;
> >> +        break;
> >> +    case QEMU_PCI_EXP_LNK_5GT:
> >> +        speed = PCIE_LINK_SPEED_5;
> >> +        break;
> >> +    case QEMU_PCI_EXP_LNK_8GT:
> >> +        speed = PCIE_LINK_SPEED_8;
> >> +        break;
> >> +    case QEMU_PCI_EXP_LNK_16GT:
> >> +        speed = PCIE_LINK_SPEED_16;
> >> +        break;
> >> +    default:
> >> +        /* Unreachable */
> >> +        abort();  
> > nit: g_assert_not_reached() here and below.  
> 
> In my opinion, g_assert_not_reached() & friends are an overly ornate
> reinvention of an old and perfectly adequate wheel.
> 
> A long time ago for reasons since forgotten, the maintainers in charge
> back then demanded abort() instead of assert(0).  Either is fine with
> me.
> 
> I tolerate g_assert_not_reached() in files that already use g_assert().
> This one doesn't.
> 
> In any case, I'd drop the comment.

I added the comment because as a casual QAPI contributor it's otherwise
not obvious that bogus user input can't reach that case.  Comments are
free.
 
> Note that I'm not this file's maintainer.

get_maintainer.pl says there is no maintainer here and references:

"Michael S. Tsirkin" <mst@redhat.com> (commit_signer:3/4=75%)
"Marc-André Lureau" <marcandre.lureau@redhat.com> (commit_signer:2/4=50%)
Markus Armbruster <armbru@redhat.com> (commit_signer:1/4=25%)
"Philippe Mathieu-Daudé" <f4bug@amsat.org> (commit_signer:1/4=25%)
Paolo Bonzini <pbonzini@redhat.com> (commit_signer:1/4=25%)

CC'ing those folks.  Unless anyone expresses a strong opinion or trend
towards using g_assert_not_reached(), I'll stick with Markus' style to
only use it in files where g_assert() is already present... or at least
claim that's why I didn't use it ;)  Thanks,

Alex

On Wed, 5 Dec 2018 13:42:25 +0100
Auger Eric <eric.auger@redhat.com> wrote:

> > --- a/qapi/common.json
> > +++ b/qapi/common.json
> > @@ -127,6 +127,48 @@
> >  { 'enum': 'OffAutoPCIBAR',
> >    'data': [ 'off', 'auto', 'bar0', 'bar1', 'bar2', 'bar3', 'bar4', 'bar5' ] }
> >  
> > +##
> > +# @PCIELinkSpeed:
> > +#
> > +# An enumeration of PCIe link speeds in units of GT/s
> > +#
> > +# @2_5: 2.5GT/s
> > +#
> > +# @5: 5.0GT/s
> > +#
> > +# @8: 8.0GT/s
> > +#
> > +# @16: 16.0GT/s
> > +#
> > +# Since: 3.2  
> 4.0 here and below

Fixed in the next spin.  Thanks,

Alex

> > +##
> > +{ 'enum': 'PCIELinkSpeed',
> > +  'data': [ '2_5', '5', '8', '16' ] }
> > +
> > +##
> > +# @PCIELinkWidth:
> > +#
> > +# An enumeration of PCIe link width
> > +#
> > +# @1: x1
> > +#
> > +# @2: x2
> > +#
> > +# @4: x4
> > +#
> > +# @8: x8
> > +#
> > +# @12: x12
> > +#
> > +# @16: x16
> > +#
> > +# @32: x32
> > +#
> > +# Since: 3.2
> > +##
> > +{ 'enum': 'PCIELinkWidth',
> > +  'data': [ '1', '2', '4', '8', '12', '16', '32' ] }
> > +
> >  ##
> >  # @SysEmuTarget:
> >  #
> > 
> >