Re: [PATCH libvirt v2 03/11] nodedev: detect AP queues

On Fri, 13 Nov 2020 15:19:11 +0100 Shalini Chellathurai Saroja <shalini(a)linux.ibm.com&gt; 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.

>> >>> 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 */ >