Re: [PATCH ACS v4 1/1] Enabling PCI ACS P2P upstream forwarding

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 10/06/09 13:33, Allen Kay wrote:
> Changes from v3 to v4:
>         - enable ACS only if iommu is enabled or if kernel is running as xen dom0
>         - changed xen_domain_type from enum to #define to allow it to be used
>           in pci/probe.c and to avoid excessive changes to code structure.
>   

Why can't it be used as an enum?

> +++ b/drivers/pci/pci.h
> @@ -311,4 +311,14 @@ static inline int pci_resource_alignment(struct pci_dev *dev,
>  	return resource_alignment(res);
>  }
>  
> +extern void pci_enable_acs(struct pci_dev *dev);
> +
> +#ifdef CONFIG_DMAR
> +extern int iommu_detected;
> +#endif
> +
> +#ifdef CONFIG_XEN
> +extern int xen_domain_type;
> +#endif
>   

That's pretty ugly; duplicate externs are pretty bad idea.  Why not just
include the right header?  If its because its asm-x86, I'm happy to move
the definitions to somewhere more common.

> +
>  #endif /* DRIVERS_PCI_H */
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 8105e32..32703c7 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1014,6 +1014,17 @@ static void pci_init_capabilities(struct pci_dev *dev)
>  
>  	/* Single Root I/O Virtualization */
>  	pci_iov_init(dev);
> +
> +	/* Enable ACS P2P upstream forwarding if HW iommu is detected */
> +	if (iommu_detected)
> +		pci_enable_acs(dev);
> +
> +#ifdef CONFIG_XEN
> +	/* HW iommu is not visible in xen dom0 */
> +	if (xen_domain_type)
> +		pci_enable_acs(dev);
> +#endif
>   

The idea is that you're supposed to use xen_domain() to test for
Xenness; it expands to a constant 0 if Xen isn't configured, so there's
no need to use #ifdefs.

> +
>  }
>  
>  void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> diff --git a/include/linux/dmar.h b/include/linux/dmar.h
> index 69a6fba..d13c21c 100644
> --- a/include/linux/dmar.h
> +++ b/include/linux/dmar.h
> @@ -196,7 +196,7 @@ extern irqreturn_t dmar_fault(int irq, void *dev_id);
>  extern int arch_setup_dmar_msi(unsigned int irq);
>  
>  #ifdef CONFIG_DMAR
> -extern int iommu_detected, no_iommu;
> +extern int no_iommu;
>  extern struct list_head dmar_rmrr_units;
>  struct dmar_rmrr_unit {
>  	struct list_head list;		/* list of rmrr units	*/
> diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
> index dd0bed4..d798770 100644
> --- a/include/linux/pci_regs.h
> +++ b/include/linux/pci_regs.h
> @@ -502,6 +502,7 @@
>  #define PCI_EXT_CAP_ID_VC	2
>  #define PCI_EXT_CAP_ID_DSN	3
>  #define PCI_EXT_CAP_ID_PWR	4
> +#define PCI_EXT_CAP_ID_ACS	13
>  #define PCI_EXT_CAP_ID_ARI	14
>  #define PCI_EXT_CAP_ID_ATS	15
>  #define PCI_EXT_CAP_ID_SRIOV	16
> @@ -662,4 +663,16 @@
>  #define  PCI_SRIOV_VFM_MO	0x2	/* Active.MigrateOut */
>  #define  PCI_SRIOV_VFM_AV	0x3	/* Active.Available */
>  
> +/* Access Control Service */
> +#define PCI_ACS_CAP		0x04	/* ACS Capability Register */
> +#define  PCI_ACS_SV		0x01	/* Source Validation */
> +#define  PCI_ACS_TB		0x02	/* Translation Blocking */
> +#define  PCI_ACS_RR		0x04	/* P2P Request Redirect */
> +#define  PCI_ACS_CR		0x08	/* P2P Completion Redirect */
> +#define  PCI_ACS_UF		0x10	/* Upstream Forwarding */
> +#define  PCI_ACS_EC		0x20	/* P2P Egress Control */
> +#define  PCI_ACS_DT		0x40	/* Direct Translated P2P */
> +#define PCI_ACS_CTRL		0x06	/* ACS Control Register */
> +#define PCI_ACS_EGRESS_CTL_V	0x08	/* ACS Egress Control Vector */
> +
>  #endif /* LINUX_PCI_REGS_H */
>   

NAK

    J