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

[Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>]

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