From: Paul Mackerras <hidden> Date: 2011-12-20 10:21:42
Under pHyp, recent kernels use the dispatch trace log (DTL) to measure
stolen time. The DTL is a ring buffer containing 48-byte entries,
where the hypervisor creates an entry each time a virtual cpu is
dispatched. The entries contain a couple of fields that the kernel
interprets as stolen time, measured in timebase ticks.
Although this is not an ideal interface, it is one that our guest
kernels already support. So this series of patches adds code to
Book3S HV KVM to measure stolen time and report it to the guest via a
dispatch trace log.
Stolen time is measured per virtual core (set of 4 vcpus, on POWER7)
as being all the time when no vcpu thread is executing inside
kvmppc_run_core(), or when a vcpu thread is running the virtual core
but is preempted.
The first patch fixes some potential races with the registration and
unregistration of the DTL and the other per-virtual-processor areas,
since the guest can (un)register a per-virtual-processor area for one
vcpu in a call to the H_REGISTER_VPA hypercall on another vcpu, and
hence potentially while KVM is using a previously-registered area.
The second patch adds the machinery for measuring stolen time and for
creating DTL entries.
Paul.
From: Paul Mackerras <hidden> Date: 2011-12-20 10:22:57
The PAPR API allows three sorts of per-virtual-processor areas to be
registered (VPA, SLB shadow buffer, and dispatch trace log), and
furthermore, these can be registered and unregistered for another
virtual CPU. Currently we just update the vcpu fields pointing to
these areas at the time of registration or unregistration. If this
is done on another vcpu, there is the possibility that the target vcpu
is using those fields at the time and could end up using a bogus
pointer and corrupting memory.
This fixes the race by making the target cpu itself do the update, so
we can be sure that the update happens at a time when the fields aren't
being used. These are updated from a set of 'next_*' fields, which
are protected by a spinlock. (We could have just taken the spinlock
when using the vpa, slb_shadow or dtl fields, but that would mean
taking the spinlock on every guest entry and exit.)
The code in do_h_register_vpa now takes the spinlock and updates the
'next_*' fields. There is also a set of '*_pending' flags to indicate
that an update is pending.
This also changes 'struct dtl' (which was undefined) to 'struct dtl_entry',
which is what the rest of the kernel uses.
Signed-off-by: Paul Mackerras <redacted>
---
arch/powerpc/include/asm/kvm_host.h | 15 +++-
arch/powerpc/kvm/book3s_hv.c | 167 +++++++++++++++++++++++++----------
2 files changed, 131 insertions(+), 51 deletions(-)
From: Paul Mackerras <hidden> Date: 2011-12-20 10:37:24
This adds code to measure "stolen" time per virtual core in units of
timebase ticks, and to report the stolen time to the guest using the
dispatch trace log (DTL). The guest can register an area of memory
for the DTL for a given vcpu. The DTL is a ring buffer where KVM
fills in one entry every time it enters the guest for that vcpu.
Stolen time is measured as time when the virtual core is not running,
either because the vcore is not runnable (e.g. some of its vcpus are
executing elsewhere in the kernel or in userspace), or when the vcpu
thread that is running the vcore is preempted. This includes time
when all the vcpus are idle (i.e. have executed the H_CEDE hypercall),
which is OK because the guest accounts stolen time while idle as idle
time.
Each vcpu keeps a record of how much stolen time has been reported to
the guest for that vcpu so far. When we are about to enter the guest,
we create a new DTL entry (if the guest vcpu has a DTL) and report the
difference between total stolen time for the vcore and stolen time
reported so far for the vcpu as the "enqueue to dispatch" time in the
DTL entry.
Signed-off-by: Paul Mackerras <redacted>
---
arch/powerpc/include/asm/kvm_host.h | 4 +++
arch/powerpc/kvm/book3s_hv.c | 43 ++++++++++++++++++++++++++++++++++-
2 files changed, 46 insertions(+), 1 deletions(-)
From: Alexander Graf <hidden> Date: 2012-01-16 13:04:37
On 20.12.2011, at 11:22, Paul Mackerras wrote:
The PAPR API allows three sorts of per-virtual-processor areas to be
registered (VPA, SLB shadow buffer, and dispatch trace log), and
furthermore, these can be registered and unregistered for another
virtual CPU. Currently we just update the vcpu fields pointing to
these areas at the time of registration or unregistration. If this
is done on another vcpu, there is the possibility that the target vcpu
is using those fields at the time and could end up using a bogus
pointer and corrupting memory.
=20
This fixes the race by making the target cpu itself do the update, so
we can be sure that the update happens at a time when the fields =
aren't
being used. These are updated from a set of 'next_*' fields, which
are protected by a spinlock. (We could have just taken the spinlock
when using the vpa, slb_shadow or dtl fields, but that would mean
taking the spinlock on every guest entry and exit.)
=20
The code in do_h_register_vpa now takes the spinlock and updates the
'next_*' fields. There is also a set of '*_pending' flags to indicate
that an update is pending.
=20
This also changes 'struct dtl' (which was undefined) to 'struct =
dtl_entry',
which is what the rest of the kernel uses.
=20
Signed-off-by: Paul Mackerras <redacted>
---
arch/powerpc/include/asm/kvm_host.h | 15 +++-
arch/powerpc/kvm/book3s_hv.c | 167 =
This could probably use a new variable name. Also, what do 0 and 4 mean? =
Constant defines would be nice here.
quoted hunk
return H_PARAMETER;
+ free_va =3D va =3D NULL;
+ len =3D 0;
if (flags < 4) {
if (vpa & 0x7f)
return H_PARAMETER;
@@ -165,65 +167,122 @@ static unsigned long do_h_register_vpa(struct =
kvm_vcpu *vcpu,
[pasted from real source]
va =3D kvmppc_pin_guest_page(kvm, vpa, &nb);
Here you're pinning the page, setting va to that temporarily available =
address.
[...]
len =3D *(unsigned short *)(va + 4);
This is a condition on (flags <=3D 1). We bail out on flags =3D=3D 0 a =
few lines up. Just move this whole thing into the respective function =
handlers.
else
len =3D *(unsigned int *)(va + 4);
va + 4 isn't really descriptive. Is this a defined struct? Why not =
actually define one which you can just read data from? Or at least make =
this a define too. Reading random numbers in code is barely readable.
+ free_va =3D va;
Now free_va is the temporarily available address.
if (len > nb)
goto out_unpin;
- switch (flags) {
- case 1: /* register VPA */
- if (len < 640)
- goto out_unpin;
- if (tvcpu->arch.vpa)
- kvmppc_unpin_guest_page(kvm, =
vcpu->arch.vpa);
- tvcpu->arch.vpa =3D va;
- init_vpa(vcpu, va);
- break;
- case 2: /* register DTL */
- if (len < 48)
- goto out_unpin;
- len -=3D len % 48;
- if (tvcpu->arch.dtl)
- kvmppc_unpin_guest_page(kvm, =
Now you're setting next_vpa to this temporarily available address? But =
next_vpa will be used after va is getting free'd, no? Or is that why you =
have free_va?
Wouldn't it be easier to just map it every time we actually use it and =
only shove the GPA around? We could basically save ourselves a lot of =
the logic here.
Alex
From: Alexander Graf <hidden> Date: 2012-01-16 13:11:52
On 20.12.2011, at 11:37, Paul Mackerras wrote:
This adds code to measure "stolen" time per virtual core in units of
timebase ticks, and to report the stolen time to the guest using the
dispatch trace log (DTL). The guest can register an area of memory
for the DTL for a given vcpu. The DTL is a ring buffer where KVM
fills in one entry every time it enters the guest for that vcpu.
Stolen time is measured as time when the virtual core is not running,
either because the vcore is not runnable (e.g. some of its vcpus are
executing elsewhere in the kernel or in userspace), or when the vcpu
thread that is running the vcore is preempted. This includes time
when all the vcpus are idle (i.e. have executed the H_CEDE hypercall),
which is OK because the guest accounts stolen time while idle as idle
time.
Each vcpu keeps a record of how much stolen time has been reported to
the guest for that vcpu so far. When we are about to enter the guest,
we create a new DTL entry (if the guest vcpu has a DTL) and report the
difference between total stolen time for the vcore and stolen time
reported so far for the vcpu as the "enqueue to dispatch" time in the
DTL entry.
Signed-off-by: Paul Mackerras <redacted>
This patch makes sense and looks good to me :)
Alex
From: Paul Mackerras <hidden> Date: 2012-01-17 05:56:54
On Mon, Jan 16, 2012 at 02:04:29PM +0100, Alexander Graf wrote:
On 20.12.2011, at 11:22, Paul Mackerras wrote:
quoted
@@ -152,6 +152,8 @@ static unsigned long do_h_register_vpa(struct kvm_vcpu *vcpu,
flags &= 7;
if (flags == 0 || flags == 4)
This could probably use a new variable name. Also, what do 0 and 4 mean? Constant defines would be nice here.
Those constants are defined in PAPR as being a subfunction code
indicating what sort of area and whether it is to be registered or
unregistered. I'll make up some names for them.
[pasted from real source]
quoted
va = kvmppc_pin_guest_page(kvm, vpa, &nb);
Here you're pinning the page, setting va to that temporarily available address.
Well, it's not just temporarily available, it's available until we
unpin it, since we increment the page count, which inhibits migration.
quoted
len = *(unsigned int *)(va + 4);
va + 4 isn't really descriptive. Is this a defined struct? Why not actually define one which you can just read data from? Or at least make this a define too. Reading random numbers in code is barely readable.
It's not really a struct, at least not one that is used for anything
else. PAPR defines that the length of the buffer has to be placed in
the second 32-bit word at registration time.
Now you're setting next_vpa to this temporarily available address? But next_vpa will be used after va is getting free'd, no? Or is that why you have free_va?
Yes; here we are freeing any previously-set value of next_vpa. The
idea of free_va is that it is initially set to va so that we correctly
unpin va if any error occurs. But if there is no error, va gets put
into next_vpa and we free anything that was previously in next_vpa
instead.
Wouldn't it be easier to just map it every time we actually use it and only shove the GPA around? We could basically save ourselves a lot of the logic here.
There are fields in the VPA that we really want to be able to access
from real mode, for instance the fields that indicate whether we need
to save the FPR and/or VR values. As far as the DTL is concerned, we
could in fact use copy_to_user to access it, so it doesn't strictly
need to be pinned. We don't currently use the slb_shadow buffer, but
if we did we would need to access it from real mode, since we would be
reading it in order to set up guest SLB entries.
The other thing is that the VPA registration/unregistration is only
done a few times in the life of the guest, whereas we use the VPAs
constantly while the guest is running. So it is more efficient to do
more of the work at registration time to make it quicker to access the
VPAs.
I'll send revised patches. There's a small change I want to make to
patch 2 to avoid putting a very large stolen time value in the first
entry that gets put in after the DTL is registered, which can happen
currently if the DTL gets registered some time after the guest started
running.
Paul.
=20
This could probably use a new variable name. Also, what do 0 and 4 mean? C=
onstant defines would be nice here.
=20
Those constants are defined in PAPR as being a subfunction code
indicating what sort of area and whether it is to be registered or
unregistered. I'll make up some names for them.
=20
quoted
[pasted from real source]
quoted
va =3D kvmppc_pin_guest_page(kvm, vpa, &nb);
=20
Here you're pinning the page, setting va to that temporarily available ad=
dress.
=20
Well, it's not just temporarily available, it's available until we
unpin it, since we increment the page count, which inhibits migration.
=20
quoted
quoted
len =3D *(unsigned int *)(va + 4);
=20
va + 4 isn't really descriptive. Is this a defined struct? Why not actual=
ly define one which you can just read data from? Or at least make this a def=
ine too. Reading random numbers in code is barely readable.
=20
It's not really a struct, at least not one that is used for anything
else. PAPR defines that the length of the buffer has to be placed in
the second 32-bit word at registration time.
=20
quoted
=20
quoted
+ free_va =3D va;
=20
Now free_va is the temporarily available address.
=20
Now you're setting next_vpa to this temporarily available address? But ne=
xt_vpa will be used after va is getting free'd, no? Or is that why you have f=
ree_va?
=20
Yes; here we are freeing any previously-set value of next_vpa. The
idea of free_va is that it is initially set to va so that we correctly
unpin va if any error occurs. But if there is no error, va gets put
into next_vpa and we free anything that was previously in next_vpa
instead.
=20
quoted
=20
Wouldn't it be easier to just map it every time we actually use it and on=
ly shove the GPA around? We could basically save ourselves a lot of the logi=
c here.
=20
There are fields in the VPA that we really want to be able to access
from real mode, for instance the fields that indicate whether we need
to save the FPR and/or VR values. As far as the DTL is concerned, we
could in fact use copy_to_user to access it, so it doesn't strictly
need to be pinned. We don't currently use the slb_shadow buffer, but
if we did we would need to access it from real mode, since we would be
reading it in order to set up guest SLB entries.
=20
The other thing is that the VPA registration/unregistration is only
done a few times in the life of the guest, whereas we use the VPAs
constantly while the guest is running. So it is more efficient to do
more of the work at registration time to make it quicker to access the
VPAs.
The thing I was getting at was not the map during the lifetime, but the map d=
uring registration. Currently we have:
1) Set VPA to x
2) Assign feature y to VPA
3) Use VPA
1 and 2 are the slow path, 3 occurs more frequently. So we want 3 to be fast=
. 1 and 2 don't matter that much wrt performance.
You are currently mapping the VPA at /, which gets you into this map/unmap m=
ess trying to free the previous mapping. If you moved the map to step 2 and o=
nly stored the GPA at step 1, all map+unmap operations except for final unma=
ps would be in one spot, so you wouldn't need to construct this big complex s=
tate machine.
I hope that makes it more clear :)
Alex
=20
I'll send revised patches. There's a small change I want to make to
patch 2 to avoid putting a very large stolen time value in the first
entry that gets put in after the DTL is registered, which can happen
currently if the DTL gets registered some time after the guest started
running.
=20
Paul.
From: Paul Mackerras <hidden> Date: 2012-01-17 11:31:41
On Tue, Jan 17, 2012 at 10:27:26AM +0100, Alexander Graf wrote:
The thing I was getting at was not the map during the lifetime, but
the map during registration. Currently we have:
1) Set VPA to x
2) Assign feature y to VPA
3) Use VPA
1 and 2 are the slow path, 3 occurs more frequently. So we want 3 to
be fast. 1 and 2 don't matter that much wrt performance.
You are currently mapping the VPA at /, which gets you into this
map/unmap mess trying to free the previous mapping. If you moved the
map to step 2 and only stored the GPA at step 1, all map+unmap
operations except for final unmaps would be in one spot, so you
wouldn't need to construct this big complex state machine.
That might simplify things - I'll try it and see. The worry with
doing the map/pin at 2 is that if anything goes wrong we no longer
have the opportunity to return an error for the H_REGISTER_VPA call,
so I'll have to at least do some checking in 1, leading to possibly
more code overall.
Paul.
From: Alexander Graf <hidden> Date: 2012-01-17 12:19:24
On 17.01.2012, at 12:31, Paul Mackerras wrote:
On Tue, Jan 17, 2012 at 10:27:26AM +0100, Alexander Graf wrote:
=20
quoted
The thing I was getting at was not the map during the lifetime, but
the map during registration. Currently we have:
=20
1) Set VPA to x
2) Assign feature y to VPA
3) Use VPA
=20
1 and 2 are the slow path, 3 occurs more frequently. So we want 3 to
be fast. 1 and 2 don't matter that much wrt performance.
=20
You are currently mapping the VPA at /, which gets you into this
map/unmap mess trying to free the previous mapping. If you moved the
map to step 2 and only stored the GPA at step 1, all map+unmap
operations except for final unmaps would be in one spot, so you
wouldn't need to construct this big complex state machine.
=20
That might simplify things - I'll try it and see. The worry with
doing the map/pin at 2 is that if anything goes wrong we no longer
have the opportunity to return an error for the H_REGISTER_VPA call,
so I'll have to at least do some checking in 1, leading to possibly
more code overall.
Well, then map and unmap it in step 1 and map it in step 2 again. We're =
in the slow path so performance isn't critical. Readability and =
maintainability however are :)
Alex