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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,

> On 19 Aug 2021, at 1:28 pm, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Rahul,
> 
> On 19/08/2021 13:02, Rahul Singh wrote:
>> Compilation error is observed when HAS_PCI is enabled for ARM
>> architecture.
> 
> To be pedantic, what you are trying to solve is not a compilation error but 
> the fact that the PCI mandates helpers that doesn't yet exist on Arm. So...
> 
>> Add definition for arch_iommu_use_permitted() and
>> arch_pci_clean_pirqs().Implement dummy functions for pci_conf_read*() to
>> fix compilation error.
> 
> ... I am not really in favor of adding dummy implementation here. Instead, 
> the series should be re-ordered so we add the pci-access helpers first and 
> then enable HAS_PCI towards the end of the series.

Ok. I will remove the dummy function implementation from this patch and will 
enable the HAS_PCI 
after adding the PCI access functions patch and other bits.

> 
>> 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>
>> ---
>>  xen/arch/arm/Makefile               |  1 +
>>  xen/arch/arm/pci/Makefile           |  2 +
>>  xen/arch/arm/pci/pci-access.c       | 61 +++++++++++++++++++++++++++++
>>  xen/arch/arm/pci/pci.c              | 32 +++++++++++++++
>>  xen/drivers/passthrough/arm/iommu.c |  5 +++
>>  xen/include/asm-arm/pci.h           | 33 ++++++++++++++--
>>  6 files changed, 131 insertions(+), 3 deletions(-)
>>  create mode 100644 xen/arch/arm/pci/Makefile
>>  create mode 100644 xen/arch/arm/pci/pci-access.c
>>  create mode 100644 xen/arch/arm/pci/pci.c
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 3d3b97b5b4..0e14a5e5c8 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -6,6 +6,7 @@ ifneq ($(CONFIG_NO_PLAT),y)
>>  obj-y += platforms/
>>  endif
>>  obj-$(CONFIG_TEE) += tee/
>> +obj-$(CONFIG_HAS_PCI) += pci/
>>    obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o
>>  obj-y += bootfdt.init.o
>> diff --git a/xen/arch/arm/pci/Makefile b/xen/arch/arm/pci/Makefile
>> new file mode 100644
>> index 0000000000..a9ee0b9b44
>> --- /dev/null
>> +++ b/xen/arch/arm/pci/Makefile
>> @@ -0,0 +1,2 @@
>> +obj-y += pci.o
>> +obj-y += pci-access.o
>> diff --git a/xen/arch/arm/pci/pci-access.c b/xen/arch/arm/pci/pci-access.c
>> new file mode 100644
>> index 0000000000..b938047c03
>> --- /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)  
>> \
>> +{                                                                           
>> \
>> +    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)            
>> \
>> +{                                                                           
>> \
>> +    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)
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/pci/pci.c b/xen/arch/arm/pci/pci.c
>> new file mode 100644
>> index 0000000000..dc55d23778
>> --- /dev/null
>> +++ b/xen/arch/arm/pci/pci.c
>> @@ -0,0 +1,32 @@
>> +/*
>> + * 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>
>> +
>> +int arch_pci_clean_pirqs(struct domain *d)
>> +{
>> +    return 0;
>> +}
> 
> Please add a comment explaining why this just returns 0.

Here is the comment that I will add in next version.
/*
 * Return 0 as on ARM there is no pci physical irqs that required cleanup.
 */ 

> 
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/drivers/passthrough/arm/iommu.c 
>> b/xen/drivers/passthrough/arm/iommu.c
>> index db3b07a571..fdec1c5547 100644
>> --- a/xen/drivers/passthrough/arm/iommu.c
>> +++ b/xen/drivers/passthrough/arm/iommu.c
>> @@ -135,3 +135,8 @@ void arch_iommu_domain_destroy(struct domain *d)
>>  void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>>  {
>>  }
>> +
>> +bool arch_iommu_use_permitted(const struct domain *d)
>> +{
>> +    return true;
>> +}
> 
> Please add a comment explaining why returning true is always fine.

Here is the comment that I will add in next version:

/*
 * Return true as iommu use is always permitted if mem-sharing, 
 * mem-paging, or log-dirty functionality is not enabled.
 */ 

Regards,
Rahul