Re: [PATCH v1 02/14] xen/pci: solve compilation error on ARM with HAS_PCI enabled

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.08.2021 14:02, Rahul Singh wrote:
> Compilation error is observed when HAS_PCI is enabled for ARM
> architecture.
> 
> Add definition for arch_iommu_use_permitted() and
> arch_pci_clean_pirqs().Implement dummy functions for pci_conf_read*() to
> fix compilation error.
> 
> pci.c: In function ‘deassign_device’:
> pci.c:849:49: error: implicit declaration of function ‘pci_to_dev’;
> did you mean ‘dt_to_dev’? [-Werror=implicit-function-declaration]
>             pci_to_dev(pdev));
> 
> pci.c:18: undefined reference to `pci_conf_read16’
> pci.c:880: undefined reference to `arch_pci_clean_pirqs’
> pci.c:1392: undefined reference to `arch_iommu_use_permitted'
> 
> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>

A couple of nits, notwithstanding Julien's more general concern:

> --- /dev/null
> +++ b/xen/arch/arm/pci/pci-access.c
> @@ -0,0 +1,61 @@
> +/*
> + * Copyright (C) 2021 Arm Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/pci.h>
> +
> +static uint32_t pci_config_read(pci_sbdf_t sbdf, unsigned int reg,
> +                                unsigned int len)
> +{
> +    return ~0U;
> +}
> +
> +static void pci_config_write(pci_sbdf_t sbdf, unsigned int reg,
> +                             unsigned int len, uint32_t val)
> +{
> +}
> +
> +/*
> + * Wrappers for all PCI configuration access functions.
> + */
> +
> +#define PCI_OP_WRITE(size, type)                                            \
> +    void pci_conf_write##size (pci_sbdf_t sbdf,unsigned int reg, type val)  \

One of the two blanks preceding the backslash wants to move after the first
comma. And the blank preceding the opening parenthesis wants to be dropped.

> +{                                                                           \
> +    pci_config_write(sbdf, reg, size / 8, val);                             \
> +}
> +
> +#define PCI_OP_READ(size, type)                                             \
> +    type pci_conf_read##size (pci_sbdf_t sbdf, unsigned int reg)            \

The latter of the two applies here as well.

> +{                                                                           \
> +    return pci_config_read(sbdf, reg, size / 8);                            \
> +}
> +
> +PCI_OP_READ(8, u8)
> +PCI_OP_READ(16, u16)
> +PCI_OP_READ(32, u32)
> +PCI_OP_WRITE(8, u8)
> +PCI_OP_WRITE(16, u16)
> +PCI_OP_WRITE(32, u32)

We aim at eliminating u<N> uses in favor of uint<N>_t - please don't
introduce new uses.

Jan