Re: [PATCH 09/10] vpci/rcar: Implement vPCI.update_bar_header callback

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 11/12/20 12:00 PM, Roger Pau Monné wrote:
> On Mon, Nov 09, 2020 at 02:50:30PM +0200, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>
>> Update hardware domain's BAR header as R-Car Gen3 is a non-ECAM host
>> controller, so vPCI MMIO handlers do not work for it in hwdom.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>> ---
>>   xen/arch/arm/pci/pci-host-rcar-gen3.c | 69 +++++++++++++++++++++++++++
>>   1 file changed, 69 insertions(+)
>>
>> diff --git a/xen/arch/arm/pci/pci-host-rcar-gen3.c 
>> b/xen/arch/arm/pci/pci-host-rcar-gen3.c
>> index ec14bb29a38b..353ac2bfd6e6 100644
>> --- a/xen/arch/arm/pci/pci-host-rcar-gen3.c
>> +++ b/xen/arch/arm/pci/pci-host-rcar-gen3.c
>> @@ -23,6 +23,7 @@
>>   #include <xen/pci.h>
>>   #include <asm/pci.h>
>>   #include <xen/vmap.h>
>> +#include <xen/vpci.h>
>>   
>>   /* Error values that may be returned by PCI functions */
>>   #define PCIBIOS_SUCCESSFUL         0x00
>> @@ -307,12 +308,80 @@ int pci_rcar_gen3_config_write(struct pci_host_bridge 
>> *bridge, uint32_t _sbdf,
>>       return ret;
>>   }
>>   
>> +static void pci_rcar_gen3_hwbar_init(const struct pci_dev *pdev,
>> +                                     struct vpci_header *header)
>> +
>> +{
>> +    static bool once = true;
>> +    struct vpci_bar *bars = header->bars;
>> +    unsigned int num_bars;
>> +    int i;
> unsigned.
ok
>
>> +
>> +    /* Run only once. */
>> +    if (!once)
> Missing spaces.
ok
>
>> +        return;
>> +    once = false;
>> +
>> +    printk("\n\n ------------------------ %s -------------------\n", 
>> __func__);
>> +    switch ( pci_conf_read8(pdev->sbdf, PCI_HEADER_TYPE) & 0x7f )
>> +    {
>> +    case PCI_HEADER_TYPE_NORMAL:
>> +        num_bars = PCI_HEADER_NORMAL_NR_BARS;
>> +        break;
>> +
>> +    case PCI_HEADER_TYPE_BRIDGE:
>> +        num_bars = PCI_HEADER_BRIDGE_NR_BARS;
>> +        break;
>> +
>> +    default:
>> +        return;
>> +    }
>> +
>> +    for ( i = 0; i < num_bars; i++ )
>> +    {
>> +        uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4;
>> +
>> +        if ( bars[i].type == VPCI_BAR_MEM64_HI )
>> +        {
>> +            /*
>> +             * Skip hi part of the 64-bit register: it is read
>> +             * together with the lower part.
>> +             */
>> +            continue;
>> +        }
>> +
>> +        if ( bars[i].type == VPCI_BAR_IO )
>> +        {
>> +            /* Skip IO. */
>> +            continue;
>> +        }
>> +
>> +        if ( bars[i].type == VPCI_BAR_MEM64_LO )
>> +        {
>> +            /* Read both hi and lo parts of the 64-bit BAR. */
>> +            bars[i].addr =
>> +                (uint64_t)pci_conf_read32(pdev->sbdf, reg + 4) << 32 |
>> +                pci_conf_read32(pdev->sbdf, reg);
>> +        }
>> +        else if ( bars[i].type == VPCI_BAR_MEM32 )
>> +        {
>> +            bars[i].addr = pci_conf_read32(pdev->sbdf, reg);
>> +        }
>> +        else
>> +        {
>> +            /* Expansion ROM? */
>> +            continue;
>> +        }
> Wouldn't this be much simpler as:
Yes, seems to be simpler, thank you
>
> bars[i].addr = 0;
> switch ( bars[i].type )
> {
> case VPCI_BAR_MEM64_HI:
>      bars[i].addr = (uint64_t)pci_conf_read32(pdev->sbdf, reg + 4) << 32;
>      /+ fallthrough. */
> case VPCI_BAR_MEM64_LO:
>       bars[i].addr |= pci_conf_read32(pdev->sbdf, reg);
>       break;
>
> default:
>      break;
> }
>
> I also wonder why you only care about the address but not the size of
> the BAR.
Yes, size needs to be updated as well, even for RFC
>
> Thanks, Roger.

Thank you,

Oleksandr