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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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/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 <xen/pci.h>
> +
> +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

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