Re: [PATCH v3 12/13] xen/iommu: smmu: Add Xen specific code to be able to use the driver

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-02-06 at 13:20 +0000, Stefano Stabellini wrote:
> > +#define iommu_group_get_iommudata(group) (group)->cfg
> 
> I would move all the Xen stuff at the beginning of the file or maybe to
> a separate file. You could #include the linux smmu driver into it.
> Maybe we cannot have runtime patching of this file, but at least we can
> keep Xen and Linux stuff clearly separate.

I was going to suggest something similar e.g. #include <ssmu-xen-shim.h>
or something.

> 
> 
> >  static DEFINE_SPINLOCK(arm_smmu_devices_lock);
> >  static LIST_HEAD(arm_smmu_devices);
> >  
> > @@ -455,6 +690,8 @@ static void parse_driver_options(struct arm_smmu_device *smmu)
> >  
> >  static struct device_node *dev_get_dev_node(struct device *dev)
> >  {
> > +	/* Xen: TODO: Add support for PCI */
> > +#if 0
> >  	if (dev_is_pci(dev)) {
> >  		struct pci_bus *bus = to_pci_dev(dev)->bus;
> >  
> > @@ -462,7 +699,7 @@ static struct device_node *dev_get_dev_node(struct device *dev)
> >  			bus = bus->parent;
> >  		return bus->bridge->parent->of_node;
> >  	}
> > -
> > +#endif
> 
> Would it be possible instead to add:
> 
> #define dev_is_pci (0)
> 
> above among the Xen definitions?

Would be preferable if possible.
>   
> > @@ -700,11 +938,10 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
> >  	/* Retry or terminate any stalled transactions */
> >  	if (fsr & FSR_SS)
> >  		writel_relaxed(resume, cb_base + ARM_SMMU_CB_RESUME);
> > -
> > -	return ret;
> >  }
> 
> For the sake of minimizing the changes to Linux functions, could you
> write a wrapper around this function instead? That might allow you to
> remove the changes related to the return value.

typedef void irqreturn_t; and "#define IRQ_NONE /**/" etc perhaps?

Or even just cast the function in the call to request_irq, the differing
return type shouldn't cause a problem in this context I think.

> 
> 
> > +/* Xen: Page tables are shared with the processor */
> > +#if 0
> >  static void arm_smmu_flush_pgtable(struct arm_smmu_device *smmu, void *addr,
> >  				   size_t size)
> >  {
> > @@ -749,6 +987,7 @@ static void arm_smmu_flush_pgtable(struct arm_smmu_device *smmu, void *addr,
> >  				DMA_TO_DEVICE);
> >  	}
> >  }
> > +#endif
> 
> But then you need to fix all the call sites. I think it is best to add a
> return at the beginning of the function.

Or add a nop stub... (return sounds better though)

> > @@ -886,9 +1131,21 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
> >  			reg |= (64 - smmu->s1_input_size) << TTBCR_T0SZ_SHIFT;
> >  		}
> >  	} else {
> > +#if CONFIG_ARM_32
> > +		/* Xen: Midway is using 40-bit address space.

I'm a bit surprised that the upstream driver isn't prepared to cope with
this sort of thing, there are a few LPAE arm32 systems around these
days. I had the same thought around the " /* Xen: The fault address
maybe higher than 32 bits on arm32 */" comment too.

Ian.