From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH 2/2] xen/arm: Make HAS_PCI compilable on ARM by adding place-holder code Date: Mon, 13 Apr 2015 11:42:50 +0100 Message-ID: <552B9DAA.9030706@citrix.com> References: <552B74B3.6000702@caviumnetworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <552B74B3.6000702@caviumnetworks.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Manish Jaggi , Xen Devel , Stefano Stabellini , Julien Grall , Ian Campbell , "Kumar, Vijaya" , "Prasun.kapoor@cavium.com" List-Id: xen-devel@lists.xenproject.org Hi Manish, On 13/04/15 08:48, Manish Jaggi wrote: > Add ARM PCI Support > --------------- > a) Place holder functions are added for pci_conf_read/write calls. > b) Macros dev_is_pci, pci_to_dev are implemented in > drivers/passthrough/pci/arm code > > Signed-off-by: Manish Jaggi > --- > xen/arch/arm/Makefile | 1 + > xen/arch/arm/pci.c | 60 +++++++++++++++++++++++ > xen/drivers/passthrough/arm/Makefile | 1 + > xen/drivers/passthrough/arm/pci.c | 88 > ++++++++++++++++++++++++++++++++++ > xen/drivers/passthrough/arm/smmu.c | 1 - > xen/drivers/passthrough/pci.c | 9 ++-- > xen/include/asm-arm/device.h | 33 +++++++++---- > xen/include/asm-arm/domain.h | 3 ++ > xen/include/asm-arm/pci.h | 7 +-- > 9 files changed, 186 insertions(+), 17 deletions(-) > > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > index 935999e..aaf5d88 100644 > --- a/xen/arch/arm/Makefile > +++ b/xen/arch/arm/Makefile > @@ -39,6 +39,7 @@ obj-y += device.o > obj-y += decode.o > obj-y += processor.o > obj-y += smc.o > +obj-$(HAS_PCI) += pci.o > > #obj-bin-y += ....o > > diff --git a/xen/arch/arm/pci.c b/xen/arch/arm/pci.c > new file mode 100644 > index 0000000..9438f41 > --- /dev/null > +++ b/xen/arch/arm/pci.c > @@ -0,0 +1,60 @@ > +#include > + > +void _pci_conf_write( The _ is not necessary here. Please name the function pci_conf_write. > + uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func, > + uint32_t reg, uint32_t data, int bytes) unsigned int bytes? > +{ > +} > + > +uint32_t _pci_conf_read( > + uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func, > + uint32_t reg, int bytes) > +{ > + return 0; > +} Same here. > + > +uint8_t pci_conf_read8( > + uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func, > + uint32_t reg) > +{ > + return (uint8_t)_pci_conf_read(seg, bus, dev, func, reg, 1); > +} > + > +uint16_t pci_conf_read16( > + uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func, > + uint32_t reg) > +{ > + return (uint8_t)_pci_conf_read(seg, bus, dev, func, reg, 2); Wrong cast. > +} > + > +uint32_t pci_conf_read32( > + uint32_t seg, uint32_t bus, uint32_t dev, uint32_t func, > + uint32_t reg) > +{ > + return (uint8_t)_pci_conf_read(seg, bus, dev, func, reg, 4); Wrong cast. [..] > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index 004aba9..68c292b 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -1252,9 +1252,12 @@ static int assign_device(struct domain *d, u16 > seg, u8 bus, u8 devfn) > /* Prevent device assign if mem paging or mem sharing have been > * enabled for this domain */ > if ( unlikely(!need_iommu(d) && > - (d->arch.hvm_domain.mem_sharing_enabled || > - d->mem_event->paging.ring_page || > - p2m_get_hostp2m(d)->global_logdirty)) ) > + (d->mem_event->paging.ring_page > +#ifdef CONFIG_X86 > + || d->arch.hvm_domain.mem_sharing_enabled || > + p2m_get_hostp2m(d)->global_logdirty > +#endif > + )) ) Please avoid #ifdef CONFIG_* and introduce an arch macro. > return -EXDEV; > > if ( !spin_trylock(&pcidevs_lock) ) > diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h > index a72f7c9..009ff0a 100644 > --- a/xen/include/asm-arm/device.h > +++ b/xen/include/asm-arm/device.h > @@ -6,6 +6,17 @@ > enum device_type > { > DEV_DT, > + DEV_ENUMERATED, > +}; > + > +enum device_class > +{ > + DEVICE_SERIAL, > + DEVICE_IOMMU, > + DEVICE_GIC, > + DEVICE_PCI, > + /* Use for error */ > + DEVICE_UNKNOWN, > }; Why? It will be very unlikely that we have to create a structure device for the IOMMU, GIC and SERIAL. It would make more sense to add a DEV_PCI directly to device_type. > > struct dev_archdata { > @@ -16,28 +27,30 @@ struct dev_archdata { > struct device > { > enum device_type type; > + enum device_class class; > #ifdef HAS_DEVICE_TREE > struct dt_device_node *of_node; /* Used by drivers imported from > Linux */ > #endif > struct dev_archdata archdata; > +#ifdef HAS_PCI > + void *pci_dev; This field is not necessary. I've added one for the DT for keeping compatibility with Linux. > +#endif [..] > +int dev_is_pci(device_t *dev); This could easily be a macro. > > struct device_desc { > /* Device name */ > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index 9e0419e..41ae847 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -42,6 +42,8 @@ struct vtimer { > uint64_t cval; > }; > > +#define has_arch_pdevs(d) (!list_empty(&(d)->arch.pdev_list)) > + > struct arch_domain > { > #ifdef CONFIG_ARM_64 > @@ -56,6 +58,7 @@ struct arch_domain > xen_pfn_t *grant_table_gpfn; > > struct io_handler io_handlers; > + struct list_head pdev_list; > /* Continuable domain_relinquish_resources(). */ > enum { > RELMEM_not_started, > 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? Regards, -- Julien Grall