Re: [PATCH 2/2] xen/arm: Make HAS_PCI compilable on ARM by adding place-holder code

[Julien Grall <julien.grall@citrix.com>]

On 14/04/15 11:33, Julien Grall wrote:
> On 14/04/15 10:28, Stefano Stabellini wrote:
>> On Tue, 14 Apr 2015, Jaggi, Manish wrote:
>>>> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
>>>> index de13359..b8ec882 100644
>>>> --- a/xen/include/asm-arm/pci.h
>>>> +++ b/xen/include/asm-arm/pci.h
>>>> @@ -1,7 +1,8 @@
>>>> -#ifndef __X86_PCI_H__
>>>> -#define __X86_PCI_H__
>>>> +#ifndef __ARM_PCI_H__
>>>> +#define __ARM_PCI_H__
>>>>
>>>>  struct arch_pci_dev {
>>>> +    void *dev;
>>>
>>> void * is error-prone. Why can't you use the use the real structure?
>>>
>>> [manish]Will change it.  I believe dev_archdata structure has also a void *  (in asm-arm/device.h). So all void * are error prone in xen ?
>>>
>>
>> As you know void* works around the type system, so it prevents the
>> compiler from making many type safety checks. We should try to avoid
>> them if we can.
> 
> In this case, the pointer add more management (allocation...).
> 
> As for the device tree solution, the field should be a struct device.
> 
>> I think that you are right, the void *iommu in dev_archdata should
>> actually be struct arm_smmu_xen_device *iommu.
> 
> I agree that void* should be void in most of the case when we know the
> underlaying type. Although there is some place where the number of type
> of unknown because it could be used to store driver specific case.
> 
> This is actually the case of this field. It contains private data for
> IOMMU driver. When a new driver will be implemented, it will likely use
> a different structure.
> 
> Furthermore, the SMMU drivers is self contained in a file, using struct
> arm_smmu_xen_device* would require to export/define some part of the
> driver in an header.

A similar example would be the scheduler coder (see sched_data in
include/xen/sched-if.h). We don't want to expose the underlying
structure out of the file.

Regards,


-- 
Julien Grall