Issue With Patch Compilation Fails ( xen/arm: Introduce a generic way to describe device) with HAS_PCI and HAS_PASSTHROUGH.

Issue With Patch Compilation Fails ( xen/arm: Introduce a generic way to describe device) with HAS_PCI and HAS_PASSTHROUGH.

[Jaggi, Manish]

Hi Julien,


Following patch generated compiler error when HAS_PCI adn HAS_PASSTHROUGH enabled.
Please advice how to fix this issue, or you can revert this patch.
Should I add a device structure in pci_dev or there is another way.

----
    xen/arm: Introduce a generic way to describe device
    
    Currently, Xen is supporting PCI and Platform device (based on Device Tree).
   
    While Xen only supports Platform device on ARM, Xen will gain support of
    PCI soon.
    
    Some drivers, such as IOMMU drivers, may handle PCI and platform device in
    the same way. Only few lines of code differs.
    
    Rather than requesting to provide 2 set of functions (one for PCI and
    one for platform device), introduce a generic structure "device" which
    is embedded in each specialized device.
    
    As x86 only supports PCI, introduce a new type device_t which will be an
    alias to pci_dev for this architecture. It will avoid to add a new field
    for this place.
    
    Signed-off-by: Julien Grall <julien.grall@linaro.org>
    Acked-by: Jan Beulich <jbeulich@suse.com>
    Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
    CC: Keir Fraser <keir@xen.org>
    CC: Andrew Cooper <andrew.cooper3@citrix.com>

----

Compilation error
pci.c: In function ‘iommu_add_device’:
pci.c:1263:5: error: implicit declaration of function ‘pci_to_dev’ [-Werror=implicit-function-declaration]
pci.c:1263:5: error: nested extern declaration of ‘pci_to_dev’ [-Werror=nested-externs]
pci.c:1263:5: error: passing argument 2 of ‘hd->platform_ops->add_device’ makes pointer from integer without a cast [-Werror]
pci.c:1263:5: note: expected ‘struct device_t *’ but argument is of type ‘int’
pci.c:1272:9: error: passing argument 2 of ‘hd->platform_ops->add_device’ makes pointer from integer without a cast [-Werror]
pci.c:1272:9: note: expected ‘struct device_t *’ but argument is of type ‘int’


Regards,
Manish Jaggi      

Re: Issue With Patch Compilation Fails ( xen/arm: Introduce a generic way to describe device) with HAS_PCI and HAS_PASSTHROUGH.

[Stefano Stabellini]

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

On Tue, 7 Apr 2015, Jaggi, Manish wrote:
> Hi Julien,
> 
> 
> Following patch generated compiler error when HAS_PCI adn HAS_PASSTHROUGH enabled.
> Please advice how to fix this issue, or you can revert this patch.
> Should I add a device structure in pci_dev or there is another way.

Hello Manish,

we have never really built Xen on ARM with HAS_PCI=y so it is normal
that it won't compile out of the box, it is not just a problem caused by
the commit below.  I imagine that you'll need to do more than setting
HAS_PCI to y in order to get PCI and PCI passthrough working properly
with Xen on ARM.  Feel free to go ahead and propose any changes
necessary.

Cheers,

Stefano


>     xen/arm: Introduce a generic way to describe device
>     
>     Currently, Xen is supporting PCI and Platform device (based on Device Tree).
>    
>     While Xen only supports Platform device on ARM, Xen will gain support of
>     PCI soon.
>     
>     Some drivers, such as IOMMU drivers, may handle PCI and platform device in
>     the same way. Only few lines of code differs.
>     
>     Rather than requesting to provide 2 set of functions (one for PCI and
>     one for platform device), introduce a generic structure "device" which
>     is embedded in each specialized device.
>     
>     As x86 only supports PCI, introduce a new type device_t which will be an
>     alias to pci_dev for this architecture. It will avoid to add a new field
>     for this place.
>     
>     Signed-off-by: Julien Grall <julien.grall@linaro.org>
>     Acked-by: Jan Beulich <jbeulich@suse.com>
>     Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>     CC: Keir Fraser <keir@xen.org>
>     CC: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> ----
> 
> Compilation error
> pci.c: In function ‘iommu_add_device’:
> pci.c:1263:5: error: implicit declaration of function ‘pci_to_dev’ [-Werror=implicit-function-declaration]
> pci.c:1263:5: error: nested extern declaration of ‘pci_to_dev’ [-Werror=nested-externs]
> pci.c:1263:5: error: passing argument 2 of ‘hd->platform_ops->add_device’ makes pointer from integer without a cast [-Werror]
> pci.c:1263:5: note: expected ‘struct device_t *’ but argument is of type ‘int’
> pci.c:1272:9: error: passing argument 2 of ‘hd->platform_ops->add_device’ makes pointer from integer without a cast [-Werror]
> pci.c:1272:9: note: expected ‘struct device_t *’ but argument is of type ‘int’
> 
> 
> Regards,
> Manish Jaggi      

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Issue With Patch Compilation Fails ( xen/arm: Introduce a generic way to describe device) with HAS_PCI and HAS_PASSTHROUGH.

[Manish Jaggi]

On Tuesday 07 April 2015 10:13 PM, Stefano Stabellini wrote:
> On Tue, 7 Apr 2015, Jaggi, Manish wrote:
>> Hi Julien,
>>
>>
>> Following patch generated compiler error when HAS_PCI adn HAS_PASSTHROUGH enabled.
>> Please advice how to fix this issue, or you can revert this patch.
>> Should I add a device structure in pci_dev or there is another way.
> Hello Manish,
>
> we have never really built Xen on ARM with HAS_PCI=y so it is normal
> that it won't compile out of the box, it is not just a problem caused by
> the commit below.
The problem with the patch is it introduces two different structures for 
device for x86 and arm. While x86 device = pci_dev, for ARM there is a 
proper device structure and != pci_dev.
So the compilation failure is by design.
>   I imagine that you'll need to do more than setting
> HAS_PCI to y in order to get PCI and PCI passthrough working properly
> with Xen on ARM.  Feel free to go ahead and propose any changes
> necessary.
ok
>
> Cheers,
>
> Stefano
>
>
>>      xen/arm: Introduce a generic way to describe device
>>      
>>      Currently, Xen is supporting PCI and Platform device (based on Device Tree).
>>     
>>      While Xen only supports Platform device on ARM, Xen will gain support of
>>      PCI soon.
>>      
>>      Some drivers, such as IOMMU drivers, may handle PCI and platform device in
>>      the same way. Only few lines of code differs.
>>      
>>      Rather than requesting to provide 2 set of functions (one for PCI and
>>      one for platform device), introduce a generic structure "device" which
>>      is embedded in each specialized device.
>>      
>>      As x86 only supports PCI, introduce a new type device_t which will be an
>>      alias to pci_dev for this architecture. It will avoid to add a new field
>>      for this place.
>>      
>>      Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>      Acked-by: Jan Beulich <jbeulich@suse.com>
>>      Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>      CC: Keir Fraser <keir@xen.org>
>>      CC: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> ----
>>
>> Compilation error
>> pci.c: In function ‘iommu_add_device’:
>> pci.c:1263:5: error: implicit declaration of function ‘pci_to_dev’ [-Werror=implicit-function-declaration]
>> pci.c:1263:5: error: nested extern declaration of ‘pci_to_dev’ [-Werror=nested-externs]
>> pci.c:1263:5: error: passing argument 2 of ‘hd->platform_ops->add_device’ makes pointer from integer without a cast [-Werror]
>> pci.c:1263:5: note: expected ‘struct device_t *’ but argument is of type ‘int’
>> pci.c:1272:9: error: passing argument 2 of ‘hd->platform_ops->add_device’ makes pointer from integer without a cast [-Werror]
>> pci.c:1272:9: note: expected ‘struct device_t *’ but argument is of type ‘int’
>>
>>
>> Regards,
>> Manish Jaggi


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Issue With Patch Compilation Fails ( xen/arm: Introduce a generic way to describe device) with HAS_PCI and HAS_PASSTHROUGH.

[Manish Jaggi]

On Wednesday 08 April 2015 11:05 AM, Manish Jaggi wrote:
>
> On Tuesday 07 April 2015 10:13 PM, Stefano Stabellini wrote:
>> On Tue, 7 Apr 2015, Jaggi, Manish wrote:
>>> Hi Julien,
>>>
>>>
>>> Following patch generated compiler error when HAS_PCI adn 
>>> HAS_PASSTHROUGH enabled.
>>> Please advice how to fix this issue, or you can revert this patch.
>>> Should I add a device structure in pci_dev or there is another way.
>> Hello Manish,
>>
>> we have never really built Xen on ARM with HAS_PCI=y so it is normal
>> that it won't compile out of the box, it is not just a problem caused by
>> the commit below.
> The problem with the patch is it introduces two different structures 
> for device for x86 and arm. While x86 device = pci_dev, for ARM there 
> is a proper device structure and != pci_dev.
> So the compilation failure is by design.
>>   I imagine that you'll need to do more than setting
>> HAS_PCI to y in order to get PCI and PCI passthrough working properly
>> with Xen on ARM.  Feel free to go ahead and propose any changes
>> necessary.
> ok
The source of the problem is reusing code of two functions
- reassign_device
- assign_device
Earlier code has dt_ variants of these two functions.

Now the point is simple, should there be redundancy of two functions OR
change a lot of code in common file drivers/passthrough/pci.c and add 
pci_to_dev macros in all platform_ops calls ?

There are lot of issues with pci_to_dev approach
a) iommu_ops callbacks have a pci_dev parameter in x86 but have a device 
parameter in arm (smmu.c)
b) hack is done to make device as pci_dev and that is not a good way of 
doing.

I prefer having minimal/some redundancy of two functions rather than 
changing a lot of code.
So IMHO revert this patch.

>>
>> Cheers,
>>
>> Stefano
>>
>>
>>>      xen/arm: Introduce a generic way to describe device
>>>           Currently, Xen is supporting PCI and Platform device 
>>> (based on Device Tree).
>>>          While Xen only supports Platform device on ARM, Xen will 
>>> gain support of
>>>      PCI soon.
>>>           Some drivers, such as IOMMU drivers, may handle PCI and 
>>> platform device in
>>>      the same way. Only few lines of code differs.
>>>           Rather than requesting to provide 2 set of functions (one 
>>> for PCI and
>>>      one for platform device), introduce a generic structure 
>>> "device" which
>>>      is embedded in each specialized device.
>>>           As x86 only supports PCI, introduce a new type device_t 
>>> which will be an
>>>      alias to pci_dev for this architecture. It will avoid to add a 
>>> new field
>>>      for this place.
>>>           Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>      Acked-by: Jan Beulich <jbeulich@suse.com>
>>>      Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>      CC: Keir Fraser <keir@xen.org>
>>>      CC: Andrew Cooper <andrew.cooper3@citrix.com>
>>>
>>> ----
>>>
>>> Compilation error
>>> pci.c: In function ‘iommu_add_device’:
>>> pci.c:1263:5: error: implicit declaration of function ‘pci_to_dev’ 
>>> [-Werror=implicit-function-declaration]
>>> pci.c:1263:5: error: nested extern declaration of ‘pci_to_dev’ 
>>> [-Werror=nested-externs]
>>> pci.c:1263:5: error: passing argument 2 of 
>>> ‘hd->platform_ops->add_device’ makes pointer from integer without a 
>>> cast [-Werror]
>>> pci.c:1263:5: note: expected ‘struct device_t *’ but argument is of 
>>> type ‘int’
>>> pci.c:1272:9: error: passing argument 2 of 
>>> ‘hd->platform_ops->add_device’ makes pointer from integer without a 
>>> cast [-Werror]
>>> pci.c:1272:9: note: expected ‘struct device_t *’ but argument is of 
>>> type ‘int’
>>>
>>>
>>> Regards,
>>> Manish Jaggi
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Issue With Patch Compilation Fails ( xen/arm: Introduce a generic way to describe device) with HAS_PCI and HAS_PASSTHROUGH.

[Stefano Stabellini]

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

On Wed, 8 Apr 2015, Manish Jaggi wrote:
> On Wednesday 08 April 2015 11:05 AM, Manish Jaggi wrote:
> > 
> > On Tuesday 07 April 2015 10:13 PM, Stefano Stabellini wrote:
> > > On Tue, 7 Apr 2015, Jaggi, Manish wrote:
> > > > Hi Julien,
> > > > 
> > > > 
> > > > Following patch generated compiler error when HAS_PCI adn
> > > > HAS_PASSTHROUGH enabled.
> > > > Please advice how to fix this issue, or you can revert this patch.
> > > > Should I add a device structure in pci_dev or there is another way.
> > > Hello Manish,
> > > 
> > > we have never really built Xen on ARM with HAS_PCI=y so it is normal
> > > that it won't compile out of the box, it is not just a problem caused by
> > > the commit below.
> > The problem with the patch is it introduces two different structures for
> > device for x86 and arm. While x86 device = pci_dev, for ARM there is a
> > proper device structure and != pci_dev.
> > So the compilation failure is by design.
> > >   I imagine that you'll need to do more than setting
> > > HAS_PCI to y in order to get PCI and PCI passthrough working properly
> > > with Xen on ARM.  Feel free to go ahead and propose any changes
> > > necessary.
> > ok
> The source of the problem is reusing code of two functions
> - reassign_device
> - assign_device
> Earlier code has dt_ variants of these two functions.
> 
> Now the point is simple, should there be redundancy of two functions OR
> change a lot of code in common file drivers/passthrough/pci.c and add
> pci_to_dev macros in all platform_ops calls ?

It doesn't look like we need such a big change, just few more pci_to_dev
and dev_is_pci calls under drivers/passthrough.
Certainly better than duplicating assign_device and reassign_device.


> There are lot of issues with pci_to_dev approach
> a) iommu_ops callbacks have a pci_dev parameter in x86 but have a device
> parameter in arm (smmu.c)
> b) hack is done to make device as pci_dev and that is not a good way of doing.
> 
> I prefer having minimal/some redundancy of two functions rather than changing
> a lot of code.

Code duplication is harmful and it is not that minimal. In any case it
is good to have a common arch-independent infrastructure. However if you
don't like 6c5d3075d97ebe26661df063ee95b14168ad10f7, that's fine, you
can come up with a generalization that works better.


> > > 
> > > Cheers,
> > > 
> > > Stefano
> > > 
> > > 
> > > >      xen/arm: Introduce a generic way to describe device
> > > >           Currently, Xen is supporting PCI and Platform device (based on
> > > > Device Tree).
> > > >          While Xen only supports Platform device on ARM, Xen will gain
> > > > support of
> > > >      PCI soon.
> > > >           Some drivers, such as IOMMU drivers, may handle PCI and
> > > > platform device in
> > > >      the same way. Only few lines of code differs.
> > > >           Rather than requesting to provide 2 set of functions (one for
> > > > PCI and
> > > >      one for platform device), introduce a generic structure "device"
> > > > which
> > > >      is embedded in each specialized device.
> > > >           As x86 only supports PCI, introduce a new type device_t which
> > > > will be an
> > > >      alias to pci_dev for this architecture. It will avoid to add a new
> > > > field
> > > >      for this place.
> > > >           Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > > >      Acked-by: Jan Beulich <jbeulich@suse.com>
> > > >      Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > >      CC: Keir Fraser <keir@xen.org>
> > > >      CC: Andrew Cooper <andrew.cooper3@citrix.com>
> > > > 
> > > > ----
> > > > 
> > > > Compilation error
> > > > pci.c: In function ‘iommu_add_device’:
> > > > pci.c:1263:5: error: implicit declaration of function ‘pci_to_dev’
> > > > [-Werror=implicit-function-declaration]
> > > > pci.c:1263:5: error: nested extern declaration of ‘pci_to_dev’
> > > > [-Werror=nested-externs]
> > > > pci.c:1263:5: error: passing argument 2 of
> > > > ‘hd->platform_ops->add_device’ makes pointer from integer without a cast
> > > > [-Werror]
> > > > pci.c:1263:5: note: expected ‘struct device_t *’ but argument is of type
> > > > ‘int’
> > > > pci.c:1272:9: error: passing argument 2 of
> > > > ‘hd->platform_ops->add_device’ makes pointer from integer without a cast
> > > > [-Werror]
> > > > pci.c:1272:9: note: expected ‘struct device_t *’ but argument is of type
> > > > ‘int’
> > > > 
> > > > 
> > > > Regards,
> > > > Manish Jaggi
> > 
> 

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Issue With Patch Compilation Fails ( xen/arm: Introduce a generic way to describe device) with HAS_PCI and HAS_PASSTHROUGH.

[Manish Jaggi]

On Wednesday 08 April 2015 02:53 PM, Stefano Stabellini wrote:
> On Wed, 8 Apr 2015, Manish Jaggi wrote:
>> On Wednesday 08 April 2015 11:05 AM, Manish Jaggi wrote:
>>> On Tuesday 07 April 2015 10:13 PM, Stefano Stabellini wrote:
>>>> On Tue, 7 Apr 2015, Jaggi, Manish wrote:
>>>>> Hi Julien,
>>>>>
>>>>>
>>>>> Following patch generated compiler error when HAS_PCI adn
>>>>> HAS_PASSTHROUGH enabled.
>>>>> Please advice how to fix this issue, or you can revert this patch.
>>>>> Should I add a device structure in pci_dev or there is another way.
>>>> Hello Manish,
>>>>
>>>> we have never really built Xen on ARM with HAS_PCI=y so it is normal
>>>> that it won't compile out of the box, it is not just a problem caused by
>>>> the commit below.
>>> The problem with the patch is it introduces two different structures for
>>> device for x86 and arm. While x86 device = pci_dev, for ARM there is a
>>> proper device structure and != pci_dev.
>>> So the compilation failure is by design.
>>>>    I imagine that you'll need to do more than setting
>>>> HAS_PCI to y in order to get PCI and PCI passthrough working properly
>>>> with Xen on ARM.  Feel free to go ahead and propose any changes
>>>> necessary.
>>> ok
>> The source of the problem is reusing code of two functions
>> - reassign_device
>> - assign_device
>> Earlier code has dt_ variants of these two functions.
>>
>> Now the point is simple, should there be redundancy of two functions OR
>> change a lot of code in common file drivers/passthrough/pci.c and add
>> pci_to_dev macros in all platform_ops calls ?
> It doesn't look like we need such a big change, just few more pci_to_dev
> and dev_is_pci calls under drivers/passthrough.
> Certainly better than duplicating assign_device and reassign_device.
>
>
>> There are lot of issues with pci_to_dev approach
>> a) iommu_ops callbacks have a pci_dev parameter in x86 but have a device
>> parameter in arm (smmu.c)
>> b) hack is done to make device as pci_dev and that is not a good way of doing.
>>
>> I prefer having minimal/some redundancy of two functions rather than changing
>> a lot of code.
> Code duplication is harmful and it is not that minimal. In any case it
> is good to have a common arch-independent infrastructure. However if you
> don't like 6c5d3075d97ebe26661df063ee95b14168ad10f7, that's fine, you
> can come up with a generalization that works better.
>
It is working now, I added a few changes in pci_dev->arch and device and 
implemented pci_to_dev for arm.
For a short term this is ok, I have a generalization of individual 
device.h to include/xen/device.h. Will send that after merging of PCI 
passthrough patches.
>>>> Cheers,
>>>>
>>>> Stefano
>>>>
>>>>
>>>>>       xen/arm: Introduce a generic way to describe device
>>>>>            Currently, Xen is supporting PCI and Platform device (based on
>>>>> Device Tree).
>>>>>           While Xen only supports Platform device on ARM, Xen will gain
>>>>> support of
>>>>>       PCI soon.
>>>>>            Some drivers, such as IOMMU drivers, may handle PCI and
>>>>> platform device in
>>>>>       the same way. Only few lines of code differs.
>>>>>            Rather than requesting to provide 2 set of functions (one for
>>>>> PCI and
>>>>>       one for platform device), introduce a generic structure "device"
>>>>> which
>>>>>       is embedded in each specialized device.
>>>>>            As x86 only supports PCI, introduce a new type device_t which
>>>>> will be an
>>>>>       alias to pci_dev for this architecture. It will avoid to add a new
>>>>> field
>>>>>       for this place.
>>>>>            Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>>>       Acked-by: Jan Beulich <jbeulich@suse.com>
>>>>>       Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>>>       CC: Keir Fraser <keir@xen.org>
>>>>>       CC: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>
>>>>> ----
>>>>>
>>>>> Compilation error
>>>>> pci.c: In function ‘iommu_add_device’:
>>>>> pci.c:1263:5: error: implicit declaration of function ‘pci_to_dev’
>>>>> [-Werror=implicit-function-declaration]
>>>>> pci.c:1263:5: error: nested extern declaration of ‘pci_to_dev’
>>>>> [-Werror=nested-externs]
>>>>> pci.c:1263:5: error: passing argument 2 of
>>>>> ‘hd->platform_ops->add_device’ makes pointer from integer without a cast
>>>>> [-Werror]
>>>>> pci.c:1263:5: note: expected ‘struct device_t *’ but argument is of type
>>>>> ‘int’
>>>>> pci.c:1272:9: error: passing argument 2 of
>>>>> ‘hd->platform_ops->add_device’ makes pointer from integer without a cast
>>>>> [-Werror]
>>>>> pci.c:1272:9: note: expected ‘struct device_t *’ but argument is of type
>>>>> ‘int’
>>>>>
>>>>>
>>>>> Regards,
>>>>> Manish Jaggi


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Issue With Patch Compilation Fails ( xen/arm: Introduce a generic way to describe device) with HAS_PCI and HAS_PASSTHROUGH.

[Julien Grall]

On 08/04/2015 10:31, Manish Jaggi wrote:
>>> There are lot of issues with pci_to_dev approach
>>> a) iommu_ops callbacks have a pci_dev parameter in x86 but have a device
>>> parameter in arm (smmu.c)
>>> b) hack is done to make device as pci_dev and that is not a good way
>>> of doing.
>>>
>>> I prefer having minimal/some redundancy of two functions rather than
>>> changing
>>> a lot of code.
>> Code duplication is harmful and it is not that minimal. In any case it
>> is good to have a common arch-independent infrastructure. However if you
>> don't like 6c5d3075d97ebe26661df063ee95b14168ad10f7, that's fine, you
>> can come up with a generalization that works better.
>>
> It is working now, I added a few changes in pci_dev->arch and device and
> implemented pci_to_dev for arm.
> For a short term this is ok, I have a generalization of individual
> device.h to include/xen/device.h. Will send that after merging of PCI
> passthrough patches.

The  first version of the commit 6c5d307 [1] was using a generic 
solution lying in xen/device.h. Although it has been "Nack" by Jan 
Beulich because it was increasing the amount of memory used on x86.

The current solution fits the requirement for both x86 (RAM usage) and 
ARM (support 2 kind of device) without duplication the IOMMU ops.

Regards,

[1] 
http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg01659.html

-- 
Julien Grall