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@xxxxxxxxxxxxx> 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@xxxxxxxxxx> > > > > > > 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#ap-architectural-overview > > For the link, I would use > > https://www.kernel.org/doc/html/latest/s390/vfio-ap.html#ap-architectural-overview > > > Patch 6: > > https://github.com/torvalds/linux/blob/master/Documentation/s390/vfio-ap.rst#the-design > > 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
