On 29/05/2015 21:23, Radim Krčmář wrote:
>> > +int kvm_vcpu_write_guest(struct kvm_vcpu *vcpu, gpa_t gpa, const void *data,
>> > + unsigned long len)
>> > +{
>> > + gfn_t gfn = gpa >> PAGE_SHIFT;
>> > + int seg;
>> > + int offset = offset_in_page(gpa);
>> > + int ret;
>> > +
>> > + while ((seg = next_segment(len, offset)) != 0) {
>> > + ret = kvm_vcpu_write_guest_page(vcpu, gfn, data, offset, seg);
>> > + if (ret < 0)
>> > + return ret;
>> > + offset = 0;
>> > + len -= seg;
>> > + data += seg;
>> > + ++gfn;
>> > + }
>> > + return 0;
>> > +}
> (There is no need to pass vcpu, and kvm, in this API.
How so? A single kvm_vcpu_write_guest can cross multiple slots.
Paolo
> Extracting memslots early will help to keep more code common.
>
> I have patches that did a superset of this for the old API, so posting
> them after this series is finalized will be simple.)
2015-06-05 12:26+0200, Paolo Bonzini:
> On 29/05/2015 21:23, Radim Krčmář wrote:
>>> > +int kvm_vcpu_write_guest(struct kvm_vcpu *vcpu, gpa_t gpa, const void *data,
>>> > + unsigned long len)
>>> > +{
| [...]
>>> > +}
>> (There is no need to pass vcpu, and kvm, in this API.
>
> How so? A single kvm_vcpu_write_guest can cross multiple slots.
I meant passing 'struct kvm_memslots *' instead and as soon as possible,
which would still allow more slots. Something like this hunk:
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 10ae7e348dcc..8c6d84c12f18 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1718,8 +1718,8 @@ int kvm_vcpu_write_guest_page(struct kvm_vcpu *vcpu, gfn_t gfn,
}
EXPORT_SYMBOL_GPL(kvm_vcpu_write_guest_page);
-int kvm_write_guest(struct kvm *kvm, gpa_t gpa, const void *data,
- unsigned long len)
+static int __kvm_write_guest(struct kvm_memslots *slots, gpa_t gpa, const void *data,
+ unsigned long len)
{
gfn_t gfn = gpa >> PAGE_SHIFT;
int seg;
@@ -1727,7 +1727,8 @@ int kvm_write_guest(struct kvm *kvm, gpa_t gpa, const void *data,
int ret;
while ((seg = next_segment(len, offset)) != 0) {
- ret = kvm_write_guest_page(kvm, gfn, data, offset, seg);
+ ret = __kvm_write_guest_page(__gfn_to_memslot(slots, gfn), gfn,
+ data, offset, seg);
if (ret < 0)
return ret;
offset = 0;
@@ -1737,26 +1738,18 @@ int kvm_write_guest(struct kvm *kvm, gpa_t gpa, const void *data,
}
return 0;
}
+
+int kvm_write_guest(struct kvm *kvm, gpa_t gpa, const void *data,
+ unsigned long len)
+{
+ return __kvm_write_guest(kvm_memslots(kvm), gpa, data, len);
+}
EXPORT_SYMBOL_GPL(kvm_write_guest);
int kvm_vcpu_write_guest(struct kvm_vcpu *vcpu, gpa_t gpa, const void *data,
unsigned long len)
{
- gfn_t gfn = gpa >> PAGE_SHIFT;
- int seg;
- int offset = offset_in_page(gpa);
- int ret;
-
- while ((seg = next_segment(len, offset)) != 0) {
- ret = kvm_vcpu_write_guest_page(vcpu, gfn, data, offset, seg);
- if (ret < 0)
- return ret;
- offset = 0;
- len -= seg;
- data += seg;
- ++gfn;
- }
- return 0;
+ return __kvm_write_guest(kvm_vcpu_memslots(vcpu), gpa, data, len);
}
EXPORT_SYMBOL_GPL(kvm_vcpu_write_guest);
On 05/06/2015 14:10, Radim Krčmář wrote:
> 2015-06-05 12:26+0200, Paolo Bonzini:
>> On 29/05/2015 21:23, Radim Krčmář wrote:
>>>>> +int kvm_vcpu_write_guest(struct kvm_vcpu *vcpu, gpa_t gpa, const void *data,
>>>>> + unsigned long len)
>>>>> +{
> | [...]
>>>>> +}
>>> (There is no need to pass vcpu, and kvm, in this API.
>>
>> How so? A single kvm_vcpu_write_guest can cross multiple slots.
>
> I meant passing 'struct kvm_memslots *' instead and as soon as possible,
> which would still allow more slots.
Oh, indeed that works fine!
Paolo
On 05/06/2015 14:10, Radim Krčmář wrote:
> + ret = __kvm_write_guest_page(__gfn_to_memslot(slots, gfn), gfn,
> + data, offset, seg);
Even better, let's pass memslots to all the __ functions.
Paolo
2015-06-05 14:46+0200, Paolo Bonzini:
> On 05/06/2015 14:10, Radim Krčmář wrote:
> > + ret = __kvm_write_guest_page(__gfn_to_memslot(slots, gfn), gfn,
> > + data, offset, seg);
>
> Even better, let's pass memslots to all the __ functions.
Yeah, while scoping it, I noticed a bug in the series ...
makes me wish that C had a useful type system.
A quick fix would be to replace gpa with gfn in calls to
__kvm_read_guest_atomic(). I presume you'd prefer a new patch to
rebasing, so it's below.
---
KVM: fix gpa/gfn mixup in __kvm_read_guest_atomic
Refactoring passed gpa instead of gfn to __kvm_read_guest_atomic.
While at it, lessen code duplication by extracting slots earlier.
Fixes: 841509f38372 ("KVM: add vcpu-specific functions to read/write/translate GFNs")
Signed-off-by: Radim Krčmář <[email protected]>
---
virt/kvm/kvm_main.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 10ae7e348dcc..4fa1edc34630 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1645,11 +1645,14 @@ int kvm_vcpu_read_guest(struct kvm_vcpu *vcpu, gpa_t gpa, void *data, unsigned l
}
EXPORT_SYMBOL_GPL(kvm_vcpu_read_guest);
-static int __kvm_read_guest_atomic(struct kvm_memory_slot *slot, gfn_t gfn,
- void *data, int offset, unsigned long len)
+static int __kvm_read_guest_atomic(struct kvm_memslots *slots, gpa_t gpa,
+ void *data, unsigned long len)
{
int r;
unsigned long addr;
+ gfn_t gfn = gpa >> PAGE_SHIFT;
+ struct kvm_memory_slot *slot = __gfn_to_memslot(slots, gfn);
+ int offset = offset_in_page(gpa);
addr = gfn_to_hva_memslot_prot(slot, gfn, NULL);
if (kvm_is_error_hva(addr))
@@ -1665,22 +1668,18 @@ static int __kvm_read_guest_atomic(struct kvm_memory_slot *slot, gfn_t gfn,
int kvm_read_guest_atomic(struct kvm *kvm, gpa_t gpa, void *data,
unsigned long len)
{
- gfn_t gfn = gpa >> PAGE_SHIFT;
- struct kvm_memory_slot *slot = gfn_to_memslot(kvm, gfn);
- int offset = offset_in_page(gpa);
+ struct kvm_memslots *slots = kvm_memslots(kvm);
- return __kvm_read_guest_atomic(slot, gpa, data, offset, len);
+ return __kvm_read_guest_atomic(slots, gpa, data, len);
}
EXPORT_SYMBOL_GPL(kvm_read_guest_atomic);
int kvm_vcpu_read_guest_atomic(struct kvm_vcpu *vcpu, gpa_t gpa,
void *data, unsigned long len)
{
- gfn_t gfn = gpa >> PAGE_SHIFT;
- struct kvm_memory_slot *slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn);
- int offset = offset_in_page(gpa);
+ struct kvm_memslots *slots = kvm_vcpu_memslots(vcpu);
- return __kvm_read_guest_atomic(slot, gpa, data, offset, len);
+ return __kvm_read_guest_atomic(slots, gpa, data, len);
}
EXPORT_SYMBOL_GPL(kvm_vcpu_read_guest_atomic);
On 05/06/2015 17:13, Radim Krčmář wrote:
> 2015-06-05 14:46+0200, Paolo Bonzini:
>> On 05/06/2015 14:10, Radim Krčmář wrote:
>>> + ret = __kvm_write_guest_page(__gfn_to_memslot(slots, gfn), gfn,
>>> + data, offset, seg);
>>
>> Even better, let's pass memslots to all the __ functions.
>
> Yeah, while scoping it, I noticed a bug in the series ...
> makes me wish that C had a useful type system.
>
> A quick fix would be to replace gpa with gfn in calls to
> __kvm_read_guest_atomic(). I presume you'd prefer a new patch to
> rebasing, so it's below.
Since it was pushed only for 15 minutes or so, and the fix is two lines:
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 30425ce6a4a4..848af90b8091 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1669,7 +1669,7 @@ int kvm_read_guest_atomic(struct kvm *kvm, gpa_t gpa, void *data,
struct kvm_memory_slot *slot = gfn_to_memslot(kvm, gfn);
int offset = offset_in_page(gpa);
- return __kvm_read_guest_atomic(slot, gpa, data, offset, len);
+ return __kvm_read_guest_atomic(slot, gfn, data, offset, len);
}
EXPORT_SYMBOL_GPL(kvm_read_guest_atomic);
@@ -1680,7 +1680,7 @@ int kvm_vcpu_read_guest_atomic(struct kvm_vcpu *vcpu, gpa_t gpa,
struct kvm_memory_slot *slot = kvm_vcpu_gfn_to_memslot(vcpu, gfn);
int offset = offset_in_page(gpa);
- return __kvm_read_guest_atomic(slot, gpa, data, offset, len);
+ return __kvm_read_guest_atomic(slot, gfn, data, offset, len);
}
EXPORT_SYMBOL_GPL(kvm_vcpu_read_guest_atomic);
I just force-pushed kvm/next.
The patch is good, but I prefer to do minimal changes before fleeing
on holiday.
Paolo