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 <[email protected]> > > > >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.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
_______________________________________________ Linuxppc-dev mailing list [email protected] https://lists.ozlabs.org/listinfo/linuxppc-dev
