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

[Shalini Chellathurai Saroja]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20201113/70cb1503/attachment-0001.htm>