Re: [PATCH 2/4] KVM: Introduce hva_to_gfn() for kvm_handle_hva()

Avi Kivity
2012-06-18T11:59:34Z

On 06/15/2012 02:31 PM, Takuya Yoshikawa wrote:
> This restricts hva handling in mmu code and makes it easier to extend
> kvm_handle_hva() so that it can treat a range of addresses later in this
> patch series.
>
>
>
> kvm_for_each_memslot(memslot, slots) {
> - unsigned long start = memslot->userspace_addr;
> - unsigned long end;
> -
> - end = start + (memslot->npages << PAGE_SHIFT);
> - if (hva >= start && hva < end) {
> - gfn_t gfn_offset = (hva - start) >> PAGE_SHIFT;
> - gfn_t gfn = memslot->base_gfn + gfn_offset;
> + gfn_t gfn = hva_to_gfn(hva, memslot);
>
> + if (gfn >= memslot->base_gfn &&
> + gfn < memslot->base_gfn + memslot->npages) {

First you convert it, then you check if the conversion worked? Let's
make is a straightforward check-then-convert (or check-and-convert).

> ret = 0;
>
> for (j = PT_PAGE_TABLE_LEVEL;
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 27ac8a4..92b2029 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -740,6 +740,13 @@ static inline gfn_t gfn_to_index(gfn_t gfn, gfn_t base_gfn, int level)
> (base_gfn >> KVM_HPAGE_GFN_SHIFT(level));
> }
>
> +static inline gfn_t hva_to_gfn(unsigned long hva, struct kvm_memory_slot *slot)
> +{
> + gfn_t gfn_offset = (hva - slot->userspace_addr) >> PAGE_SHIFT;
> +
> + return slot->base_gfn + gfn_offset;
> +}

Should be named hva_to_gfn_memslot(), like the below, to emphasise it
isn't generic.

> +
> static inline unsigned long gfn_to_hva_memslot(struct kvm_memory_slot *slot,
> gfn_t gfn)
> {
>

--
error compiling committee.c: too many arguments to function