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

["Jaggi, Manish" <Manish.Jaggi@xxxxxxxxxxxxxxxxxx>]

Hi Julien,

________________________________________
From: xen-devel-bounces@xxxxxxxxxxxxx <xen-devel-bounces@xxxxxxxxxxxxx> on 
behalf of Julien Grall <julien.grall@xxxxxxxxxx>
Sent: Monday, April 13, 2015 4:12 PM
To: Jaggi, Manish; Xen Devel; Stefano Stabellini; Julien Grall; Ian Campbell; 
Kumar, Vijaya; Prasun.kapoor@xxxxxxxxxx
Subject: Re: [Xen-devel] [PATCH 2/2] xen/arm: Make HAS_PCI compilable on ARM by 
adding place-holder code

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 <manish.jaggi@xxxxxxxxxxxxxxxxxx>
> ---
>  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/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. 
[Manish] ok
>          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.
[manish] ok. 

>
>  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.
[Manish] pci_dev is not in struct device currently. Didn't get you
 I have added as It is required  for to_pci_dev call.

> +#endif

[..]

> +int dev_is_pci(device_t *dev);

This could easily be a macro.
[manish] ok

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

[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 ?

Regards,

--
Julien Grall

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