On 04/09/2019 15:22, Andrew Jones wrote:

On Wed, Sep 04, 2019 at 02:55:15PM +0100, Steven Price wrote:

quoted

On 02/09/2019 13:52, Andrew Jones wrote:

quoted

On Fri, Aug 30, 2019 at 04:25:08PM +0100, Steven Price wrote:

quoted

On 30/08/2019 15:47, Andrew Jones wrote:

quoted

On Fri, Aug 30, 2019 at 09:42:46AM +0100, Steven Price wrote:

[...]

quoted

quoted

quoted

quoted

+    Return value: (int32)   : NOT_SUPPORTED (-1) or SUCCESS (0) if the relevant
+                              PV-time feature is supported by the hypervisor.
+
+PV_TIME_ST
+    Function ID:  (uint32)  : 0xC5000022
+    Return value: (int64)   : IPA of the stolen time data structure for this
+                              VCPU. On failure:
+                              NOT_SUPPORTED (-1)
+
+The IPA returned by PV_TIME_ST should be mapped by the guest as normal memory
+with inner and outer write back caching attributes, in the inner shareable
+domain. A total of 16 bytes from the IPA returned are guaranteed to be
+meaningfully filled by the hypervisor (see structure below).
+
+PV_TIME_ST returns the structure for the calling VCPU.
+
+Stolen Time
+-----------
+
+The structure pointed to by the PV_TIME_ST hypercall is as follows:
+
+  Field       | Byte Length | Byte Offset | Description
+  ----------- | ----------- | ----------- | --------------------------
+  Revision    |      4      |      0      | Must be 0 for version 0.1
+  Attributes  |      4      |      4      | Must be 0

The above fields don't appear to be exposed to userspace in anyway. How
will we handle migration from one KVM with one version of the structure
to another?

Interesting question. User space does have access to them now it is
providing the memory, but it's not exactly an easy method. In particular
user space has no (simple) way of probing the kernel's supported version.

I guess one solution would be to add an extra attribute on the VCPU
which would provide the revision information. The current kernel would
then reject any revision other than 0, but this could then be extended
to support other revision numbers in the future.

Although there's some logic in saying we could add the extra attribute
when(/if) there is a new version. Future kernels would then be expected
to use the current version unless user space explicitly set the new
attribute.

Do you feel this is something that needs to be addressed now, or can it
be deferred until another version is proposed?

Assuming we'll want userspace to have the option of choosing version=0,
and that we're fine with version=0 being the implicit choice, when nothing
is selected, then I guess it can be left as is for now. If, OTOH, we just
want migration to fail when attempting to migrate to another host with
an incompatible stolen-time structure (i.e. version=0 is not selectable
on hosts that implement later versions), then we should expose the version
in some way now. Perhaps a VCPU's "PV config" should be described in a
set of pseudo registers?

I wouldn't have thought making migration fail if/when the host upgrades
to a new version would be particularly helpful - we'd want to provide
backwards compatibility. In particular for the suspend/resume case (I
want to be able to save my VM to disk, upgrade the host kernel and then
resume the VM).

The only potential issue I see is the implicit "version=0 if not
specified". That seems solvable by rejecting setting the stolen time
base address if no version has been specified and the host kernel
doesn't support version=0.

I think that's the same failure I was trying avoid by failing the
migration instead. Maybe it's equivalent to fail at this vcpu-ioctl
time though?

Yes this is effectively the same failure. But since we require the
vcpu-ioctl to enable stolen time this gives an appropriate place to
fail. Indeed this is the failure if migrating from a host with these
patches to one running an existing kernel with no stolen time support.

Steve