On Fri, 4 Dec 2020 16:31:56 +0100 Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
On 12/4/20 11:24 AM, Erik Skultety wrote:
On Thu, Dec 03, 2020 at 06:59:32PM +0100, Shalini Chellathurai Saroja wrote:
Add support for AP card devices, AP queues and AP matrix devices in libvirt node device driver. --- v4: - Added virNodeDevAPAdapterParseXML function to extract the adapter parsing logic. - Modified according to review comments. - New patch to mention support for AP devices in NEWS.rst.
Reviewed-by: Erik Skultety <eskultet@redhat.com>
I had 2 nitpicks which I can fix before merging, but I'd like to give other people a couple more days to express their "final" opinions and if there are no more comments, then sometime next week I'll merge this. There's one more little thing...Boris linked the s390 AP facility kernel documentation which really helped me during the review, so I think we should link it somewhere too - usually we're not so keen on doing that because 3rd party documentation URLs tend to die or migrate, but in this case it linked directly to the github repo (I think even the generated HTML on kernel.org would be just fine), but I don't know what the right place for this actually is as it describes the whole facility which we modelled in 3 capabilities. We could put it into the NEWS file, but then again, not sure how often anyone developing libvirt reads the NEWS file.
Regards, Erik
Erik, thanks for your review. How about we put for developers the links into the device originating commit messages:
<comment from the side line>
Patch 1+3: https://github.com/torvalds/linux/blob/master/Documentation/s390/vfio-ap.rst...
For the link, I would use https://www.kernel.org/doc/html/latest/s390/vfio-ap.html#ap-architectural-ov...
Patch 6: https://github.com/torvalds/linux/blob/master/Documentation/s390/vfio-ap.rst...
and https://www.kernel.org/doc/html/latest/s390/vfio-ap.html#the-design </comment from the side line>
Another alternative could be to add the links into docs/formatnode.html.in but that might be too much information for the normal libvirt users not interested in these details.