On Tue, Jul 07, 2015 at 11:29:11 -0400, John Ferlan wrote: > > > On 07/07/2015 08:19 AM, Peter Krempa wrote: > > On Mon, Jul 06, 2015 at 13:08:28 -0400, John Ferlan wrote: > >> v1 here: > >> http://www.redhat.com/archives/libvir-list/2015-June/msg00814.html > >> > >> Changes since v1: > >> - Add doc patch 1 to indicate that this feature may only be supported by > >> certain kernels > >> - Adjust former patch 1 to add call to qemuIsSharedHostdev from > >> qemuSetUnprivSGIO > >> - Insert patches 7 & 8 which essentially refactor qemuSetUnprivSGIO a bit. > >> There should be no functional difference > >> - Patch 9 is now a much slimmer former patch 6 > >> > >> The end result is that 'generically speaking' if any kernel supports > >> setting the unprivileged SGIO feature, then these patches provide > >> the capability to do so. > >> > >> Although as pointed out in the review of v1 only one specific downstream > >> kernel supports the feature, that doesn't mean other distros couldn't add > >> support in the same manner. So rather than just remove all traces from > >> libvirt completely, it seems it would be reasonable to keep the checks > >> in place and if a kernel then decides to add support this code exists > >> to assist. > > > > Well, I'm not going to insist that we revert the existing code since > > it's possible that the feature might actually make it into the upstream > > linux kernel eventually. > > > > Until it's upstream I don't think though we should add support (even if > > we document that it will not work) for stuff that is not upstream since > > the design of the upstream interface might then differ, which will make > > us carry two implementations. > > This series merely addresses adding back the hostdev changes using the > same processing as <disk> except for where the sgio bit is stored and > handled for hostdev. The unpriv_sgio for <disk> is already in libvirt as > of 1.0.2 and if I read between the lines correctly, that's not in the > upstream kernel either, but I cannot state that for sure.
Correct, none of the unpriv_sgio stuff is there, not even for <disk>. That's the part Jan was thinking of reverting from upstream. My stance is that we could wait for a while before purging that stuff to see whether the patches possibly make it upstream. If there's no such incentive, we should revert the code to the point where it would not change the behavior but the parts that actually check for the sysfs interface can be removed since the file never existed in upstream kernels. > > If unpriv_sgio doesn't exist upstream for <disk> and could cause us to > carry two implementations if done differently, then I don't see how > having hostdev have the same implementation as disk is problematic. Or > am I misreading what you wrote? My assumption being that whatever is > done upstream would use the same mechanism for disk and hostdev as it > does "now" for the downstream implementation. Well, if we accept this upstream it will decrease the motivation to get the kernel part merged upstream since it will work for the vendor that carries the downstream kernel patch and others will not care. If we want to make it useful, we should do it right. > > > > > Since there's already existing code that touches the kernel interface > > for unpriv_sgio, actually exposing the support will then require us to > > carry that part instead of changing it to the actual upstream impl. > > > > If a "hypothetical" downstream distro would add kernel patches to add the > > feature, they might as well as carry the downstream libvirt patches too. > > > > NACK series until upstream kernel support is present. (Some of the > > cleanup patches may be worth taking until the kernel issue gets settled > > though.) > > Right w/r/t taking some of these patches... This is something I pointed > out in the original series, but since this is a new series here's my > thoughts... > > Patch 1 changes to entirely remove the text about sgio in hostdev. Agreed, 1 is mostly fine. > > Patch 2 seems OK with a slight text adjustment on the commit message > > Patches 3-8 adjust the shared hostdev logic regardless of unpriv sgio Yes, I'll check them later whether they make sense if 9 and 10 is removed. > > Patches 9 & 10 could then become downstream only, at least for now. Much > smaller subset of changes than the original add/revert (and there's I > think 2 less bugs from what was there, plus the extra one you found in > patch 7 that would have been cleaned up by patch 9 - although I'll > contend that's an ordering thing - I did 9 first and then thought that 7 > & 8 would things more logical). Making all the stuff upstream, including the kernel change is the preferred way. > > Since there are ACK's for 2 and 8, I can assume to a degree you agree > with my thoughts through patch 8. There's no explicit ACK's on other > patches, although there is an explicit NACK to the whole series, so I'll > wait before doing anything with this to see your and others thoughts > regarding what is or should be applicable. I've already adjusted in my > local branch patch 2, 7, & 8. Peter
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
