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

[Shalini Chellathurai Saroja]

On 11/13/20 4:50 PM, Jonathon Jongsma wrote:
> On Fri, 13 Nov 2020 15:19:11 +0100
> Shalini Chellathurai Saroja <shalini at 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