Re: [PATCH v6 09/25] KVM: arm64: iommu: Add memory pool

[Vincent Donnefort]

[...]

> > nit: kvm_iommu_init uses nr_pages. 
> > 
> > >  {
> > >  	guard(mutex)(&kvm_iommu_reg_lock);
> > >  
> > > @@ -21,6 +22,36 @@ int kvm_iommu_register_driver(struct kvm_iommu_ops *hyp_ops)
> > >  	if (kvm_nvhe_sym(kvm_iommu_ops))
> > >  		return -EBUSY;
> > >  
> > > +	/* See kvm_iommu_pages() */
> > > +	if (pool_pages > kvm_nvhe_sym(hyp_kvm_iommu_pages)) {
> > > +		kvm_err("Not enough memory for the IOMMU pool, need 0x%x pages, check kvm-arm.hyp_iommu_pages",
> > > +			pool_pages);
> > > +		return -ENOMEM;
> > > +	}
> > > +
> > >  	kvm_nvhe_sym(kvm_iommu_ops) = hyp_ops;
> > >  	return 0;
> > >  }
> > > +
> > > +unsigned int kvm_iommu_pages(void)
> > > +{
> > > +	/*
> > > +	 * This is used very early during setup_arch() before any initcalls
> > > +	 * or any drivers are registered.
> > > +	 * This value is set by a command line option.
> > > +	 * Later, when the driver is registered, it will pass the number
> > > +	 * pages needed for it's page tables, if it was less that what
> > > +	 * the system has already allocated, the registration will fail.
> > > +	 */
> > > +	return kvm_nvhe_sym(hyp_kvm_iommu_pages);
> > > +}
> > 
> > Could we actually estimate the memory needed, based on the allocation for the
> > host stage-2 and an estimation of MMIO coverage? This would allow the whole
> > thing to run without setting this value as a best effort and to make it
> > optional, which is surely more user-friendly?
> 
> Not really, this abstracts the IOMMU, so it does not have to match the
> host stage-2, in addition that the SMMUv3 driver requires extra pages
> for other in-memory data-structure.

What I thought would be to take __hyp_pgtable_total_pages() and then add on top
of that memory to cover the MMIO region. As of "here's memory we think your
driver will need" and of course that can't take into account any
"driver"-specific struct.

To be fair, that is a bit weird to have memory reservation so early for a
"driver" but perhaps that is okay because the pKVM protection is only complete
if we have that "driver"?

> 
> At the moment, this is set from the command line which is not really
> best effort as the driver later confirms how much memory it needs.
> 
> > 
> > > +
> > > +/* Number of pages to reserve for iommu pool*/
> > > +static int __init early_hyp_iommu_pages(char *arg)
> > > +{
> > > +	if (!arg)
> > > +		return -EINVAL;
> > > +
> > > +	return kstrtouint(arg, 0, &kvm_nvhe_sym(hyp_kvm_iommu_pages));
> > 
> > How about using a memory size here with memparse()? This would be more
> > transparent for the commandline which can work on different page-size systems.
> 
> Makes sense, will do.
> 
> Thanks,
> Mostafa