Re: [PATCH kernel v2 5/6] KVM: PPC: Move reusable bits of H_PUT_TCE handler to helpers

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4237 bytes --]

On Mon, Jan 25, 2016 at 11:12:36AM +1100, David Gibson wrote:
> On Thu, Jan 21, 2016 at 06:39:36PM +1100, Alexey Kardashevskiy wrote:
> > Upcoming multi-tce support (H_PUT_TCE_INDIRECT/H_STUFF_TCE hypercalls)
> > will validate TCE (not to have unexpected bits) and IO address
> > (to be within the DMA window boundaries).
> > 
> > This introduces helpers to validate TCE and IO address. The helpers are
> > exported as they compile into vmlinux (to work in realmode) and will be
> > used later by KVM kernel module in virtual mode.
> > 
> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > ---
> > Changes:
> > v2:
> > * added note to the commit log about why new helpers are exported
> > * did not add a note that xxx_validate() validate TCEs for KVM (not for
> > host kernel DMA) as the helper names and file location tell what are
> > they for
> > ---
> >  arch/powerpc/include/asm/kvm_ppc.h  |  4 ++
> >  arch/powerpc/kvm/book3s_64_vio_hv.c | 92 ++++++++++++++++++++++++++++++++-----
> >  2 files changed, 84 insertions(+), 12 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> > index 2241d53..9513911 100644
> > --- a/arch/powerpc/include/asm/kvm_ppc.h
> > +++ b/arch/powerpc/include/asm/kvm_ppc.h
> > @@ -166,6 +166,10 @@ extern int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu);
> >  
> >  extern long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
> >  				struct kvm_create_spapr_tce *args);
> > +extern long kvmppc_ioba_validate(struct kvmppc_spapr_tce_table *stt,
> > +		unsigned long ioba, unsigned long npages);
> > +extern long kvmppc_tce_validate(struct kvmppc_spapr_tce_table *tt,
> > +		unsigned long tce);
> >  extern long kvmppc_h_put_tce(struct kvm_vcpu *vcpu, unsigned long liobn,
> >  			     unsigned long ioba, unsigned long tce);
> >  extern long kvmppc_h_get_tce(struct kvm_vcpu *vcpu, unsigned long liobn,
> > diff --git a/arch/powerpc/kvm/book3s_64_vio_hv.c b/arch/powerpc/kvm/book3s_64_vio_hv.c
> > index e142171..8cd3a95 100644
> > --- a/arch/powerpc/kvm/book3s_64_vio_hv.c
> > +++ b/arch/powerpc/kvm/book3s_64_vio_hv.c
> > @@ -36,6 +36,7 @@
> >  #include <asm/kvm_host.h>
> >  #include <asm/udbg.h>
> >  #include <asm/iommu.h>
> > +#include <asm/tce.h>
> >  
> >  #define TCES_PER_PAGE	(PAGE_SIZE / sizeof(u64))
> >  
> > @@ -64,18 +65,90 @@ static struct kvmppc_spapr_tce_table *kvmppc_find_table(struct kvm_vcpu *vcpu,
> >   * 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,
> > +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 mask = IOMMU_PAGE_MASK_4K;
> >  	unsigned long idx = ioba >> IOMMU_PAGE_SHIFT_4K;
> >  	unsigned long size = stt->window_size >> IOMMU_PAGE_SHIFT_4K;
> >  
> > -	if ((ioba & mask) || (idx + npages > size))
> > +	if ((ioba & ~mask) || (idx + npages > size))
> >  		return H_PARAMETER;
> >  
> >  	return H_SUCCESS;
> >  }
> > +EXPORT_SYMBOL_GPL(kvmppc_ioba_validate);
> > +
> > +/*
> > + * Validates TCE address.
> > + * At the moment flags and page mask are validated.
> > + * As the host kernel does not access those addresses (just puts them
> > + * to the table and user space is supposed to process them), we can skip
> > + * checking other things (such as TCE is a guest RAM address or the page
> > + * was actually allocated).
> > + *
> > + * WARNING: This will be called in real-mode on HV KVM and virtual
> > + *          mode on PR KVM
> > + */
> > +long kvmppc_tce_validate(struct kvmppc_spapr_tce_table *stt,
> >  unsigned long tce)
> 
> It would be nice to write this in terms of kvmppc_ioba_validate() above.

Duh, sorry.  Realised shortly afterwards that's nonsense.  One is
looking at the IOBA, the other at the real address.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]