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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Jan

> On 20 Aug 2021, at 8:01 am, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> 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.

I will modify as below . Please correct me if I misunderstood wrongly.

#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);                   \             
      
}

> 
>> +{                                                                           
>> \
>> +    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.

Ok . I will modify in next version.

Regards,
Rahul
> 
> Jan