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.

...


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
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294