On 11/13/20 4:50 PM, Jonathon Jongsma wrote:
On Fri, 13 Nov 2020 15:19:11 +0100 Shalini Chellathurai Saroja <shalini@linux.ibm.com> wrote:
On 11/12/20 6:01 PM, Jonathon Jongsma wrote:
diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index d10a79e3..45281363 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -439,6 +439,17 @@ <dd>AP Card identifier.</dd> </dl> </dd> + <dt><code>ap_queue</code></dt> + <dd>Describes the AP Queue on a S390 host. An AP Queue is + identified by it's ap-adapter and ap-domain id. "it's" should be "its". Is it worth a very brief description of what an AP queue actually is? Sure, I will provide a brief description on AP queue.
...
+ <define name='capapqueue'> + <attribute name='type'> + <value>ap_queue</value> + </attribute> + <element name='ap-adapter'> + <ref name='apAdapterRange'/> + </element> + <element name='ap-domain'> + <ref name='apDomainRange'/> + </element> + </define>
Let's use double quotes to keep the file consistent.
Sure:-)
+ <define name='address'> <element name='address'> <attribute name='domain'><ref name='hexuint'/></attribute> @@ -738,4 +751,16 @@ </choice> </define>
+ <define name="apDomainRange"> + <choice> + <data type="string"> + <param name="pattern">(0x)?[0-9a-fA-F]{1,4}</param> + </data> + <data type="int"> + <param name="minInclusive">0</param> + <param name="maxInclusive">255</param> Is 255 correct here? the hex pattern above implies that it is a 16 bit value, but here you're limiting it to 255. If it is a 16 bit value, can we just use the already-defined 'uint16' type from basictypes.rng?
Yes, it is. Current architecture limit is 256 (0-255) for domains.
The address schema of domains was created in linux on z, with a future architectural change in mind.
...
So, this is a bit confusing to me. The xml schema appears to be trying to prevent me from specifying a value above 255 if I enter it as an integer, yet it allows me to specify a value of 0xFFFF if I specify it as a hex value.
Side note: I can also enter plain integers beyond 255 because of the fact that the '0x' prefix is optional. 9999 will validate according to the schema because it matches the hex string pattern.
However I note that your parsing code does not enforce this 255 limit anywhere.
I will enforce the maximum limit of domain value as 255 in the parsing code. I will modify the apDomainRange definition to have the prefix '0x' as required instead of optional. Thank you for pointing it out:-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index b4eb4553..6bbff571 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1218,6 +1218,29 @@ udevProcessAPCard(struct udev_device *device, }
+static int +udevProcessAPQueue(struct udev_device *device, + virNodeDeviceDefPtr def) +{ + char *c; + virNodeDevCapDataPtr data = &def->caps->data; + In the previous patch, you added a comment explaining the format of the sysfs path. I found that helpful. Without knowing the format, it's a bit difficult to judge whether this code below is correct.
I will add the below comment
/* The sysfs path would be in the format /sys/bus/ap/devices/xx.yyyy, where xx is ap adapter id and yyyy is ap domain id. eg:/sys/bus/ap/devices/08.0001 */
-- Kind regards Shalini Chellathurai Saroja Linux on Z and Virtualization Development Vorsitzende des Aufsichtsrats: Gregor Pillen Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294