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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

> +
> +    /* Run only once. */
> +    if (!once)

Missing spaces.

> +        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:

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.

Thanks, Roger.