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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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@xxxxxxxxxx>
> > > >      Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > >      Acked-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > > >      CC: Keir Fraser <keir@xxxxxxx>
> > > >      CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > > > 
> > > > ----
> > > > 
> > > > 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel