diff mbox

[kernel,v2,1/6] KVM: PPC: Rework H_PUT_TCE/H_GET_TCE handlers

Message ID 1453361977-19589-2-git-send-email-aik@ozlabs.ru (mailing list archive)
State New, archived
Headers show

Commit Message

Alexey Kardashevskiy Jan. 21, 2016, 7:39 a.m. UTC
This reworks the existing H_PUT_TCE/H_GET_TCE handlers to have following
patches applied nicer.

This moves the ioba boundaries check to a helper and adds a check for
least bits which have to be zeros.

The patch is pretty mechanical (only check for least ioba bits is added)
so no change in behaviour is expected.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changelog:
v2:
* compare @ret with H_SUCCESS instead of assuming H_SUCCESS is zero
* made error reporting cleaner
---
 arch/powerpc/kvm/book3s_64_vio_hv.c | 111 +++++++++++++++++++++++-------------
 1 file changed, 72 insertions(+), 39 deletions(-)

Comments

David Gibson Jan. 22, 2016, 12:42 a.m. UTC | #1
On Thu, Jan 21, 2016 at 06:39:32PM +1100, Alexey Kardashevskiy wrote:
> This reworks the existing H_PUT_TCE/H_GET_TCE handlers to have following
> patches applied nicer.
> 
> This moves the ioba boundaries check to a helper and adds a check for
> least bits which have to be zeros.
> 
> The patch is pretty mechanical (only check for least ioba bits is added)
> so no change in behaviour is expected.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Concept looks good, but there are a couple of nits.

> ---
> Changelog:
> v2:
> * compare @ret with H_SUCCESS instead of assuming H_SUCCESS is zero
> * made error reporting cleaner
> ---
>  arch/powerpc/kvm/book3s_64_vio_hv.c | 111 +++++++++++++++++++++++-------------
>  1 file changed, 72 insertions(+), 39 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_64_vio_hv.c b/arch/powerpc/kvm/book3s_64_vio_hv.c
> index 89e96b3..862f9a2 100644
> --- a/arch/powerpc/kvm/book3s_64_vio_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_vio_hv.c
> @@ -35,71 +35,104 @@
>  #include <asm/ppc-opcode.h>
>  #include <asm/kvm_host.h>
>  #include <asm/udbg.h>
> +#include <asm/iommu.h>
>  
>  #define TCES_PER_PAGE	(PAGE_SIZE / sizeof(u64))
>  
> +/*
> + * Finds a TCE table descriptor by LIOBN.
> + *
> + * WARNING: This will be called in real or virtual mode on HV KVM and virtual
> + *          mode on PR KVM
> + */
> +static struct kvmppc_spapr_tce_table *kvmppc_find_table(struct kvm_vcpu *vcpu,
> +		unsigned long liobn)
> +{
> +	struct kvm *kvm = vcpu->kvm;
> +	struct kvmppc_spapr_tce_table *stt;
> +
> +	list_for_each_entry_lockless(stt, &kvm->arch.spapr_tce_tables, list)

list_for_each_entry_lockless?  According to the comments in the
header, that's for RCU protected lists, whereas this one is just
protected by the lock in the kvm structure.  This is replacing a plain
list_for_each_entry().


> +		if (stt->liobn == liobn)
> +			return stt;
> +
> +	return NULL;
> +}
> +
> +/*
> + * Validates IO address.
> + *
> + * WARNING: This will be called in real-mode on HV KVM and virtual
> + *          mode on PR KVM
> + */
> +static long kvmppc_ioba_validate(struct kvmppc_spapr_tce_table *stt,
> +		unsigned long ioba, unsigned long npages)
> +{
> +	unsigned long mask = (1ULL << IOMMU_PAGE_SHIFT_4K) - 1;
> +	unsigned long idx = ioba >> IOMMU_PAGE_SHIFT_4K;
> +	unsigned long size = stt->window_size >> IOMMU_PAGE_SHIFT_4K;
> +
> +	if ((ioba & mask) || (idx + npages > size))

It doesn't matter for the current callers, but you should check for
overflow in idx + npages as well.
Alexey Kardashevskiy Jan. 22, 2016, 1:59 a.m. UTC | #2
On 01/22/2016 11:42 AM, David Gibson wrote:
> On Thu, Jan 21, 2016 at 06:39:32PM +1100, Alexey Kardashevskiy wrote:
>> This reworks the existing H_PUT_TCE/H_GET_TCE handlers to have following
>> patches applied nicer.
>>
>> This moves the ioba boundaries check to a helper and adds a check for
>> least bits which have to be zeros.
>>
>> The patch is pretty mechanical (only check for least ioba bits is added)
>> so no change in behaviour is expected.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>
> Concept looks good, but there are a couple of nits.
>
>> ---
>> Changelog:
>> v2:
>> * compare @ret with H_SUCCESS instead of assuming H_SUCCESS is zero
>> * made error reporting cleaner
>> ---
>>   arch/powerpc/kvm/book3s_64_vio_hv.c | 111 +++++++++++++++++++++++-------------
>>   1 file changed, 72 insertions(+), 39 deletions(-)
>>
>> diff --git a/arch/powerpc/kvm/book3s_64_vio_hv.c b/arch/powerpc/kvm/book3s_64_vio_hv.c
>> index 89e96b3..862f9a2 100644
>> --- a/arch/powerpc/kvm/book3s_64_vio_hv.c
>> +++ b/arch/powerpc/kvm/book3s_64_vio_hv.c
>> @@ -35,71 +35,104 @@
>>   #include <asm/ppc-opcode.h>
>>   #include <asm/kvm_host.h>
>>   #include <asm/udbg.h>
>> +#include <asm/iommu.h>
>>
>>   #define TCES_PER_PAGE	(PAGE_SIZE / sizeof(u64))
>>
>> +/*
>> + * Finds a TCE table descriptor by LIOBN.
>> + *
>> + * WARNING: This will be called in real or virtual mode on HV KVM and virtual
>> + *          mode on PR KVM
>> + */
>> +static struct kvmppc_spapr_tce_table *kvmppc_find_table(struct kvm_vcpu *vcpu,
>> +		unsigned long liobn)
>> +{
>> +	struct kvm *kvm = vcpu->kvm;
>> +	struct kvmppc_spapr_tce_table *stt;
>> +
>> +	list_for_each_entry_lockless(stt, &kvm->arch.spapr_tce_tables, list)
>
> list_for_each_entry_lockless?  According to the comments in the
> header, that's for RCU protected lists, whereas this one is just
> protected by the lock in the kvm structure.  This is replacing a plain
> list_for_each_entry().

My bad, the next patch should have done this
s/list_for_each_entry/list_for_each_entry_lockless/


>
>
>> +		if (stt->liobn == liobn)
>> +			return stt;
>> +
>> +	return NULL;
>> +}
>> +
>> +/*
>> + * Validates IO address.
>> + *
>> + * WARNING: This will be called in real-mode on HV KVM and virtual
>> + *          mode on PR KVM
>> + */
>> +static long kvmppc_ioba_validate(struct kvmppc_spapr_tce_table *stt,
>> +		unsigned long ioba, unsigned long npages)
>> +{
>> +	unsigned long mask = (1ULL << IOMMU_PAGE_SHIFT_4K) - 1;
>> +	unsigned long idx = ioba >> IOMMU_PAGE_SHIFT_4K;
>> +	unsigned long size = stt->window_size >> IOMMU_PAGE_SHIFT_4K;
>> +
>> +	if ((ioba & mask) || (idx + npages > size))
>
> It doesn't matter for the current callers, but you should check for
> overflow in idx + npages as well.


npages can be only 1..512 and this is checked in H_PUT_TCE/etc handlers.
idx is 52bit long max.
And this is not going to change because H_PUT_TCE_INDIRECT will always be 
limited by 512 (or one 4K page).

Do I still need the overflow check here?
David Gibson Jan. 24, 2016, 11:43 p.m. UTC | #3
On Fri, Jan 22, 2016 at 12:59:47PM +1100, Alexey Kardashevskiy wrote:
> On 01/22/2016 11:42 AM, David Gibson wrote:
> >On Thu, Jan 21, 2016 at 06:39:32PM +1100, Alexey Kardashevskiy wrote:
> >>This reworks the existing H_PUT_TCE/H_GET_TCE handlers to have following
> >>patches applied nicer.
> >>
> >>This moves the ioba boundaries check to a helper and adds a check for
> >>least bits which have to be zeros.
> >>
> >>The patch is pretty mechanical (only check for least ioba bits is added)
> >>so no change in behaviour is expected.
> >>
> >>Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >
> >Concept looks good, but there are a couple of nits.
> >
> >>---
> >>Changelog:
> >>v2:
> >>* compare @ret with H_SUCCESS instead of assuming H_SUCCESS is zero
> >>* made error reporting cleaner
> >>---
> >>  arch/powerpc/kvm/book3s_64_vio_hv.c | 111 +++++++++++++++++++++++-------------
> >>  1 file changed, 72 insertions(+), 39 deletions(-)
> >>
> >>diff --git a/arch/powerpc/kvm/book3s_64_vio_hv.c b/arch/powerpc/kvm/book3s_64_vio_hv.c
> >>index 89e96b3..862f9a2 100644
> >>--- a/arch/powerpc/kvm/book3s_64_vio_hv.c
> >>+++ b/arch/powerpc/kvm/book3s_64_vio_hv.c
> >>@@ -35,71 +35,104 @@
> >>  #include <asm/ppc-opcode.h>
> >>  #include <asm/kvm_host.h>
> >>  #include <asm/udbg.h>
> >>+#include <asm/iommu.h>
> >>
> >>  #define TCES_PER_PAGE	(PAGE_SIZE / sizeof(u64))
> >>
> >>+/*
> >>+ * Finds a TCE table descriptor by LIOBN.
> >>+ *
> >>+ * WARNING: This will be called in real or virtual mode on HV KVM and virtual
> >>+ *          mode on PR KVM
> >>+ */
> >>+static struct kvmppc_spapr_tce_table *kvmppc_find_table(struct kvm_vcpu *vcpu,
> >>+		unsigned long liobn)
> >>+{
> >>+	struct kvm *kvm = vcpu->kvm;
> >>+	struct kvmppc_spapr_tce_table *stt;
> >>+
> >>+	list_for_each_entry_lockless(stt, &kvm->arch.spapr_tce_tables, list)
> >
> >list_for_each_entry_lockless?  According to the comments in the
> >header, that's for RCU protected lists, whereas this one is just
> >protected by the lock in the kvm structure.  This is replacing a plain
> >list_for_each_entry().
> 
> My bad, the next patch should have done this
> s/list_for_each_entry/list_for_each_entry_lockless/

Ah, yes.  I hadn't yet looked at the second patch.

> >>+		if (stt->liobn == liobn)
> >>+			return stt;
> >>+
> >>+	return NULL;
> >>+}
> >>+
> >>+/*
> >>+ * Validates IO address.
> >>+ *
> >>+ * WARNING: This will be called in real-mode on HV KVM and virtual
> >>+ *          mode on PR KVM
> >>+ */
> >>+static long kvmppc_ioba_validate(struct kvmppc_spapr_tce_table *stt,
> >>+		unsigned long ioba, unsigned long npages)
> >>+{
> >>+	unsigned long mask = (1ULL << IOMMU_PAGE_SHIFT_4K) - 1;
> >>+	unsigned long idx = ioba >> IOMMU_PAGE_SHIFT_4K;
> >>+	unsigned long size = stt->window_size >> IOMMU_PAGE_SHIFT_4K;
> >>+
> >>+	if ((ioba & mask) || (idx + npages > size))
> >
> >It doesn't matter for the current callers, but you should check for
> >overflow in idx + npages as well.
> 
> 
> npages can be only 1..512 and this is checked in H_PUT_TCE/etc handlers.
> idx is 52bit long max.
> And this is not going to change because H_PUT_TCE_INDIRECT will always be
> limited by 512 (or one 4K page).

Ah, ok.

> Do I still need the overflow check here?

Hm, I guess it's not essential.  I'd still prefer to see it though,
since it's good practice in general.
Paul Mackerras Feb. 11, 2016, 4:11 a.m. UTC | #4
On Fri, Jan 22, 2016 at 12:59:47PM +1100, Alexey Kardashevskiy wrote:
> On 01/22/2016 11:42 AM, David Gibson wrote:
> >On Thu, Jan 21, 2016 at 06:39:32PM +1100, Alexey Kardashevskiy wrote:
[snip]
> >>+	if ((ioba & mask) || (idx + npages > size))
> >
> >It doesn't matter for the current callers, but you should check for
> >overflow in idx + npages as well.
> 
> 
> npages can be only 1..512 and this is checked in H_PUT_TCE/etc handlers.
> idx is 52bit long max.
> And this is not going to change because H_PUT_TCE_INDIRECT will always be
> limited by 512 (or one 4K page).
> 
> Do I still need the overflow check here?

You could add "|| npages > TCES_PER_PAGE" and that would make it clear
that there can't be any overflow, and it should get removed by the
compiler for the calls with constant npages.

Paul.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/powerpc/kvm/book3s_64_vio_hv.c b/arch/powerpc/kvm/book3s_64_vio_hv.c
index 89e96b3..862f9a2 100644
--- a/arch/powerpc/kvm/book3s_64_vio_hv.c
+++ b/arch/powerpc/kvm/book3s_64_vio_hv.c
@@ -35,71 +35,104 @@ 
 #include <asm/ppc-opcode.h>
 #include <asm/kvm_host.h>
 #include <asm/udbg.h>
+#include <asm/iommu.h>
 
 #define TCES_PER_PAGE	(PAGE_SIZE / sizeof(u64))
 
+/*
+ * Finds a TCE table descriptor by LIOBN.
+ *
+ * WARNING: This will be called in real or virtual mode on HV KVM and virtual
+ *          mode on PR KVM
+ */
+static struct kvmppc_spapr_tce_table *kvmppc_find_table(struct kvm_vcpu *vcpu,
+		unsigned long liobn)
+{
+	struct kvm *kvm = vcpu->kvm;
+	struct kvmppc_spapr_tce_table *stt;
+
+	list_for_each_entry_lockless(stt, &kvm->arch.spapr_tce_tables, list)
+		if (stt->liobn == liobn)
+			return stt;
+
+	return NULL;
+}
+
+/*
+ * Validates IO address.
+ *
+ * WARNING: This will be called in real-mode on HV KVM and virtual
+ *          mode on PR KVM
+ */
+static long kvmppc_ioba_validate(struct kvmppc_spapr_tce_table *stt,
+		unsigned long ioba, unsigned long npages)
+{
+	unsigned long mask = (1ULL << IOMMU_PAGE_SHIFT_4K) - 1;
+	unsigned long idx = ioba >> IOMMU_PAGE_SHIFT_4K;
+	unsigned long size = stt->window_size >> IOMMU_PAGE_SHIFT_4K;
+
+	if ((ioba & mask) || (idx + npages > size))
+		return H_PARAMETER;
+
+	return H_SUCCESS;
+}
+
 /* WARNING: This will be called in real-mode on HV KVM and virtual
  *          mode on PR KVM
  */
 long kvmppc_h_put_tce(struct kvm_vcpu *vcpu, unsigned long liobn,
 		      unsigned long ioba, unsigned long tce)
 {
-	struct kvm *kvm = vcpu->kvm;
-	struct kvmppc_spapr_tce_table *stt;
+	struct kvmppc_spapr_tce_table *stt = kvmppc_find_table(vcpu, liobn);
+	long ret;
+	unsigned long idx;
+	struct page *page;
+	u64 *tbl;
 
 	/* udbg_printf("H_PUT_TCE(): liobn=0x%lx ioba=0x%lx, tce=0x%lx\n", */
 	/* 	    liobn, ioba, tce); */
 
-	list_for_each_entry(stt, &kvm->arch.spapr_tce_tables, list) {
-		if (stt->liobn == liobn) {
-			unsigned long idx = ioba >> SPAPR_TCE_SHIFT;
-			struct page *page;
-			u64 *tbl;
+	if (!stt)
+		return H_TOO_HARD;
 
-			/* udbg_printf("H_PUT_TCE: liobn 0x%lx => stt=%p  window_size=0x%x\n", */
-			/* 	    liobn, stt, stt->window_size); */
-			if (ioba >= stt->window_size)
-				return H_PARAMETER;
+	ret = kvmppc_ioba_validate(stt, ioba, 1);
+	if (ret != H_SUCCESS)
+		return ret;
 
-			page = stt->pages[idx / TCES_PER_PAGE];
-			tbl = (u64 *)page_address(page);
+	idx = ioba >> SPAPR_TCE_SHIFT;
+	page = stt->pages[idx / TCES_PER_PAGE];
+	tbl = (u64 *)page_address(page);
 
-			/* FIXME: Need to validate the TCE itself */
-			/* udbg_printf("tce @ %p\n", &tbl[idx % TCES_PER_PAGE]); */
-			tbl[idx % TCES_PER_PAGE] = tce;
-			return H_SUCCESS;
-		}
-	}
+	/* FIXME: Need to validate the TCE itself */
+	/* udbg_printf("tce @ %p\n", &tbl[idx % TCES_PER_PAGE]); */
+	tbl[idx % TCES_PER_PAGE] = tce;
 
-	/* Didn't find the liobn, punt it to userspace */
-	return H_TOO_HARD;
+	return H_SUCCESS;
 }
 EXPORT_SYMBOL_GPL(kvmppc_h_put_tce);
 
 long kvmppc_h_get_tce(struct kvm_vcpu *vcpu, unsigned long liobn,
-		      unsigned long ioba)
+		unsigned long ioba)
 {
-	struct kvm *kvm = vcpu->kvm;
-	struct kvmppc_spapr_tce_table *stt;
+	struct kvmppc_spapr_tce_table *stt = kvmppc_find_table(vcpu, liobn);
+	long ret;
+	unsigned long idx;
+	struct page *page;
+	u64 *tbl;
 
-	list_for_each_entry(stt, &kvm->arch.spapr_tce_tables, list) {
-		if (stt->liobn == liobn) {
-			unsigned long idx = ioba >> SPAPR_TCE_SHIFT;
-			struct page *page;
-			u64 *tbl;
+	if (!stt)
+		return H_TOO_HARD;
 
-			if (ioba >= stt->window_size)
-				return H_PARAMETER;
+	ret = kvmppc_ioba_validate(stt, ioba, 1);
+	if (ret != H_SUCCESS)
+		return ret;
 
-			page = stt->pages[idx / TCES_PER_PAGE];
-			tbl = (u64 *)page_address(page);
+	idx = ioba >> SPAPR_TCE_SHIFT;
+	page = stt->pages[idx / TCES_PER_PAGE];
+	tbl = (u64 *)page_address(page);
 
-			vcpu->arch.gpr[4] = tbl[idx % TCES_PER_PAGE];
-			return H_SUCCESS;
-		}
-	}
+	vcpu->arch.gpr[4] = tbl[idx % TCES_PER_PAGE];
 
-	/* Didn't find the liobn, punt it to userspace */
-	return H_TOO_HARD;
+	return H_SUCCESS;
 }
 EXPORT_SYMBOL_GPL(kvmppc_h_get_tce);