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

[Julien Grall <julien.grall@linaro.org>]

Hi Ian,

On 20/02/15 13:23, Ian Campbell wrote:
> 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.

See my comment on Stefano's mail for this.

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

It's already done but ... if (0) doesn't mean the code inside the if
could be invalid.

In this specific case, we don't have a field struct pci_bus *bus in our
pci_dev. So it won't compile.

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

I can give a look.

>>
>>
>>> +/* 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)

I don't succeed to make my point on this kind of things...

While I agree that we have to minimize the changes in the code. We
shouldn't do it at the cost of an incomprehensible cost.

The function arm_smmu_flush_pgtable doesn't make any sense at all on Xen
(the parameter are even invalid).

Furthermore there is only one call site.


>>> +#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.

At the time I resynced the SMMU drivers, they were using short page
tables. And therefore the number of address bits was limited for the guest.

I don't plan to lose again minimum 2 weeks for re-syncing the driver soon.

Regards,

-- 
Julien Grall