On Mon, Jun 18, 2018 at 05:08:32PM +0200, Halil Pasic wrote: > > > On 06/15/2018 05:37 PM, Michael S. Tsirkin wrote: > > On Fri, Jun 15, 2018 at 05:16:10PM +0200, Halil Pasic wrote: > > > > > > > > > On 06/15/2018 03:38 PM, Michael S. Tsirkin wrote: > > > > On Fri, Jun 15, 2018 at 02:42:58PM +0200, Halil Pasic wrote: > > > > > > > > > > > > > > > On 06/15/2018 02:19 PM, Michael S. Tsirkin wrote: > > > > > > On Fri, Jun 15, 2018 at 02:10:11PM +0200, Halil Pasic wrote: > > > > > > > > > > > > > > > > > > > > > On 06/11/2018 09:56 AM, Tiwei Bie wrote: > > > > > > > > Suggested-by: Michael S. Tsirkin <m...@redhat.com> > > > > > > > > Signed-off-by: Tiwei Bie <tiwei....@intel.com> > > > > > > > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/14 > > > > > > > > --- > > > > > > > > v2: > > > > > > > > - Refine the wording (Cornelia); > > > > > > > > > > > > > > > > v3: > > > > > > > > - Refine the wording (MST); > > > > > > > > > > > > > > > > content.tex | 7 +++++++ > > > > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > > > > > > > diff --git a/content.tex b/content.tex > > > > > > > > index f996fad..3c7d67d 100644 > > > > > > > > --- a/content.tex > > > > > > > > +++ b/content.tex > > > > > > > > @@ -125,6 +125,13 @@ which was not offered. The device SHOULD > > > > > > > > accept any valid subset > > > > > > > > of features the driver accepts, otherwise it MUST fail to > > > > > > > > set the > > > > > > > > FEATURES_OK \field{device status} bit when the driver > > > > > > > > writes it. > > > > > > > > +If a device has successfully negotiated a set of features > > > > > > > > +at least once (by accepting the FEATURES_OK \field{device > > > > > > > > +status} bit during device initialization), then it SHOULD > > > > > > > > +NOT fail re-negotiation of the same set of features after > > > > > > > > +a device or system reset. Failure to do so would interfere > > > > > > > > +with resuming from suspend and error recovery. > > > > > > > > + > > > > > > > > > > > > > > > > > > > > > Sorry people but I don't get it. I mean it is kind of reasonable > > > > > > > to assume that with a given device and a given driver (given, i.e. > > > > > > > nothing changes) the two will always negotiate the same features > > > > > > > (including the extremal case where the negotiation fails). > > > > > > > > > > > > > > Either the device or a driver rolling a dice to make feature > > > > > > > negotiation > > > > > > > more fun seems quite unreasonable. So I assume this is not what > > > > > > > we are > > > > > > > bothering to soft prohibit here. > > > > > > > > > > > > > > So the interesting scenario seems to be when stuff changes. When > > > > > > > migrating the implementation of the device could change. Or > > > > > > > something > > > > > > > changes regarding the resources used to provide the virtual > > > > > > > device. > > > > > > > > > > > > > > But then, if the device really can not support the set of features > > > > > > > it used to be able, I guess the SHOULD does not take effect (I > > > > > > > guess > > > > > > > that is the difference compared to MUST). > > > > > > > > > > > > > > Bottom line is: I tried to figure out what is this about, but I > > > > > > > failed. > > > > > > > I've read https://github.com/oasis-tcs/virtio-spec/issues/14 too > > > > > > > but > > > > > > > it did not click. I would appreciate some assistance. > > > > > > > > > > > > It's exactly what it says. Let's say you negotiated a feature and > > > > > > then > > > > > > device sets NEED_RESET. Driver must now reset the device and put it > > > > > > back in the same state it had before the reset, then resubmit > > > > > > requests that were available but never used. > > > > > > > > > > > > What if any of the features changed? Device suddenly > > > > > > needs to check for requests which do not match the > > > > > > features. > > > > > > > > > > > > Suspend is similar: guests tend to assume hardware does not change > > > > > > across suspend/resume, any changes tend to make resume fail. > > > > > > > > > > > > > > > > Thank you very much! But it still does not answer why would a device > > > > > want to do that (fail to negotiate a feature that it was able to > > > > > negotiate before). So I'm still in the dark about what are we trading > > > > > for what. > > > > > > > > It would be a mis-configured device. For example QEMU does not migrate > > > > the device features so if you misconfigure QEMU with different flags on > > > > source and destination (not a supported configuration), features might > > > > seem to change from guest POV. > > > > > > > > > > Do you mean set (or rather restrict) what QEMU calls the host_features? > > > > > > AFAIR there is no reset right after the migration. But yes if then there > > > is a reset and another migration. After a lots of thinking, it seems you > > > speak about the scenario I described in the answer to Tiwei Bie. But > > > there I also say that this statement you add here is not good enough for > > > that. Still puzzled. > > > > What would a good enough statement look like? > > > > > > > I did some reading and some thinking on the weekend. AFAIU the situation > is tricky. To mitigate that let me establish the terminology I'm going to > use. For vm lifecycle I'm going to use the definitions form libvirt as > defined by > https://libvirt.org/guide/html/Application_Development_Guide-Guest_Domains-Lifecycle.html. > > You explained, the motivation for this addition to the VIRTIO > specification is hibernate (aka suspend to disk). > > (1) AFAIU on hibernate the VM goes from 'running' to (most likely) > 'defined'. The first step of the resume from hibernate is to start the > VM. From the guest OS life-cycle perspective however we don't start a > completely new cycle (like the VM life-cycle does) with complete > re-initialization. After resuming form hibernate the system is expected > to be put in essentially the same state (but not exactly) as it was > before hibernate. > > (2) From VM (life-cycle) perspective we can not distinguish between a > 'shutdown' as a part of a hibernate and a 'plain shutdown'. > > (3) Any rule we come up for a device (e.g. the normative statement > proposed here) that regulates the effects of a 'system reset' that is a > part of the hibernate cycle equally affects the normal shutdown-start > cycle. > > (4) Any change in the negotiated feature set can affect the validity of > requests that have been constructed under different assumptions (i.e. > not only features going away, but also features appearing can be a > problem). > > (5) The Linux implementation already has reasonable handling for both > types of changes: the driver does not try to use the new features, and > fails cleanly if the old ones are not accepted. > > (6) Because of (3), prohibiting devices dropping support for a set of > features within a hibernate cycle is only possible if we prohibit such > changes in general. > > (7) If I read > https://www.kernel.org/doc/html/v4.14/driver-api/pm/devices.html > correctly the driver is expected to quiesce the device before going to > hibernate. AFAIU hibernating with requests in flight isn't a great idea. > > (8) If there are no in-flight requests in-flight (including on the > queues), then this whole feature changes might break requests story seems > irrelevant. > > (9) After a quick look the freeze in virtio (Linux implementation) does > not seem to do anything to prevent in-flight requests though. > > (10) From a VM management perspective a 'save' seems much preferable to > hibernate. A VM 'save' is migration like, so even if some components of > the system change between 'save' and 'restore' (e.g. QEMU up- or > donwngarde) we still have mechanisms in place that (hopefully) the guest > view of the system does not change. In this sense save/restore is > migration like. > > (11) The VIRTIO specification is a bit vague about how a reset is > supposed to be handled by the guest, but it certainly does not prohibit > the negotiated features from changing after reset. Here I will quote two > fragments that hint this is actually something foreseen by the VIRTIO > standard: > * 'During device initialization, the driver reads this and tells the > device the subset that it accepts. The only way to renegotiate is to > reset the device.' > * 'If the driver sets the FAILED bit, the driver MUST later reset the > device before attempting to re-initialize.' If re-initialize is in a > sense of '3.1.1 Driver Requirements: Device Initialization' then full > feature negotiation seems to be compulsory. Linux does not do this. But > since setting up queues seems to be a part of the 3.1.1 initialization > sequence (even if formulated somewhat vague), my best guess after reset > the driver is not supposed to perform 3.1.1 to the letter.
I think frankly if we want dynamic features we should work on a mechanism that allows changing them without a system reset. And I think the use-case that triggered this is the SRIOV feature, take a look at how that is handled across e.g. suspend/resume. > > (12) If I were to hibernate my PC and then, let's say replace my NIC with > a different model, the hardware does not change assumption would not hold > for a non-virtualized system either. I'm not sure this problem is ours to > solve. Precisely and since we can't solve it, we warn people not to create this kind of configuration unless they know exactly what they are doing. > My conclusion is the following. I think constraining feature changes > after system_reset is a bad idea. For 'normal' virtio reset some > clarifications would be welcome, but this one does not seem to be a very > good one. Regarding changing features, I think we are good enough with > what we have today (both standard and implementation). However if we want > to prohibit the features from changing after a reset in spite of my > arguments presented here, IMHO we need a driver normative statement too. > > Regards, > Halil Well the motion passed with 1 abstain and 5 in favor. Tiwei was the one who proposed it so as I already did this in the past, I'll wait a day or two for him to respond and let us know whether he'd like to drop the patch, but in absence of such a response I'll have to push the proposed wording. In that case you will need to put in a motion to revert, or make some other change on top. -- MST --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org