Re: [PATCH v2 07/13] KVM: add vcpu-specific functions to read/write/translate GFNs
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 07/13] KVM: add vcpu-specific functions to read/write/translate GFNs
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ář
---
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);
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 07/13] KVM: add vcpu-specific functions to read/write/translate GFNs
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 07/13] KVM: add vcpu-specific functions to read/write/translate GFNs
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 07/13] KVM: add vcpu-specific functions to read/write/translate GFNs
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);
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 07/13] KVM: add vcpu-specific functions to read/write/translate GFNs
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.)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 07/13] KVM: add vcpu-specific functions to read/write/translate GFNs
2015-05-27 19:05+0200, Paolo Bonzini:
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> @@ -1616,6 +1727,27 @@ int kvm_write_guest(struct kvm *kvm, gpa_t gpa, const
> void *data,
| int kvm_write_guest(struct kvm *kvm, 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_write_guest_page(kvm, gfn, data, offset, seg);
| if (ret < 0)
| return ret;
| offset = 0;
| len -= seg;
| data += seg;
| ++gfn;
| }
| return 0;
| }
>
> +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.
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.)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
