Re: [Qemu-devel] [for-4.0 PATCH v4 4/9] qapi: Define PCIe link speed and width properties
On Tue, 11 Dec 2018 09:48:37 +0100
Markus Armbruster wrote:
> Eric Blake writes:
>
> > On 12/7/18 10:41 AM, 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
> >> Tested-by: Geoffrey McRae
> >> Reviewed-by: Markus Armbruster
> >> Signed-off-by: Alex Williamson
> >> ---
> >> 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;
> >
> > C does not guarantee what width 'speed' has,...
>
> Correct. The enumeration constants have type int, but the enumeration
> type's compatible integer type is implementation-defined, see C99
> 6.7.2.2.
>
> >> +
> >> +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();
> >> +}
> >> +
> >> +visit_type_enum(v, prop->name, (int *), prop->info->enum_table,
> >> errp);
> >
> > ...making this cast to (int*) not be very kosher. I _think_ we're okay
> > here, but I'd be a LOT happier if you just used 'int speed' to avoid
> > the cast.
>
> Good catch!
>
> "The GNU Compiler Collection" section C Implementation-Defined Behavior
> / Structures, Unions, Enumerations, and Bit-Fields documents:
>
>* 'The integer type compatible with each enumerated type (C90
> 6.5.2.2, C99 and C11 6.7.2.2).'
>
> Normally, the type is 'unsigned int' if there are no negative
> values in the enumeration, otherwise 'int'. If '-fshort-enums' is
> specified, then if there are negative values it is the first of
> 'signed char', 'short' and 'int' that can represent all the values,
> otherwise it is the first of 'unsigned char', 'unsigned short' and
> 'unsigned int' that can represent all the values.
>
> On some targets, '-fshort-enums' is the default; this is determined
> by the ABI.
>
> We don't pass -fshort-enums, because we can well do without opening that
> can of worms.
>
> That leaves the dependence on the ABI. As far as I know, the ABIs we
> care about don't have short enums.
>
> Still, it's unclean without a real need.
Ok, I'll respin this patch to the following, the rest of the series is
unaffected. Since Markus has commented in agreement, I'll leave his
review. I'll give it a bit more time before I repost the whole series
again. Thanks,
Alex
commit f72d8fbfa3d081a9d0e83a9549f5bd4b0165cc92
Author: Alex Williamson
Date: Tue Dec 4 08:44:20 2018 -0700
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
Tested-by: Geoffrey McRae
Reviewed-by: Markus Armbruster
Signed-off-by: Alex Williamson
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 35072dec1ecf..1dee38000111 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1327,3 +1327,179 @@ 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
Re: [Qemu-devel] [for-4.0 PATCH v4 4/9] qapi: Define PCIe link speed and width properties
Eric Blake writes:
> On 12/7/18 10:41 AM, 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
>> Tested-by: Geoffrey McRae
>> Reviewed-by: Markus Armbruster
>> Signed-off-by: Alex Williamson
>> ---
>> 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;
>
> C does not guarantee what width 'speed' has,...
Correct. The enumeration constants have type int, but the enumeration
type's compatible integer type is implementation-defined, see C99
6.7.2.2.
>> +
>> +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();
>> +}
>> +
>> +visit_type_enum(v, prop->name, (int *), prop->info->enum_table,
>> errp);
>
> ...making this cast to (int*) not be very kosher. I _think_ we're okay
> here, but I'd be a LOT happier if you just used 'int speed' to avoid
> the cast.
Good catch!
"The GNU Compiler Collection" section C Implementation-Defined Behavior
/ Structures, Unions, Enumerations, and Bit-Fields documents:
* 'The integer type compatible with each enumerated type (C90
6.5.2.2, C99 and C11 6.7.2.2).'
Normally, the type is 'unsigned int' if there are no negative
values in the enumeration, otherwise 'int'. If '-fshort-enums' is
specified, then if there are negative values it is the first of
'signed char', 'short' and 'int' that can represent all the values,
otherwise it is the first of 'unsigned char', 'unsigned short' and
'unsigned int' that can represent all the values.
On some targets, '-fshort-enums' is the default; this is determined
by the ABI.
We don't pass -fshort-enums, because we can well do without opening that
can of worms.
That leaves the dependence on the ABI. As far as I know, the ABIs we
care about don't have short enums.
Still, it's unclean without a real need.
>
>> +}
>> +
>> +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 *),
>
> And again.
>
>> +prop->info->enum_table, _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();
>> +}
>> +}
>> +
>
>> +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) {
>
>> +visit_type_enum(v, prop->name, (int *), prop->info->enum_table,
>> errp);
>> +}
>> +
>> +static void
Re: [Qemu-devel] [for-4.0 PATCH v4 4/9] qapi: Define PCIe link speed and width properties
On 12/7/18 10:41 AM, 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
Tested-by: Geoffrey McRae
Reviewed-by: Markus Armbruster
Signed-off-by: Alex Williamson
---
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;
C does not guarantee what width 'speed' has,...
+
+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();
+}
+
+visit_type_enum(v, prop->name, (int *), prop->info->enum_table,
errp);
...making this cast to (int*) not be very kosher. I _think_ we're okay
here, but I'd be a LOT happier if you just used 'int speed' to avoid the
cast.
+}
+
+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 *),
And again.
+prop->info->enum_table, _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();
+}
+}
+
+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) {
+visit_type_enum(v, prop->name, (int *), 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 *),
And two more spots.
+++ 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: 4.0
+##
+{ 'enum': 'PCIELinkSpeed',
+ 'data': [ '2_5', '5', '8', '16' ] }
QAPI enums are special-cased to allow starting with digits, thanks to
QKeyCode. I don't know if we are trying to avoid adding yet more of
those, but the fact that you didn't have to whitelist them means that we
are at least not forcibly limiting the use of leading digits in new enums.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[Qemu-devel] [for-4.0 PATCH v4 4/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
Tested-by: Geoffrey McRae
Reviewed-by: Markus Armbruster
Signed-off-by: Alex Williamson
---
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();
+}
+
+visit_type_enum(v, prop->name, (int *), 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 *),
+prop->info->enum_table, _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 = _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 *), 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 *),
+prop->info->enum_table, _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:
+
