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

[Jonathon Jongsma]

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.

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