On Fri, Dec 04, 2020 at 05:30:04PM +0100, Cornelia Huck wrote:
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>
Sounds good, I'll tweak the commit messages before merging. Regards, Erik