Re: [PATCH 09/11] xen/arm: Setup MMIO range trap handlers for hardware domain

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Julien!

On 09.09.21 20:43, Julien Grall wrote:
> Hi Oleksandr,
>
> On 03/09/2021 09:33, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>
>> In order vPCI to work it needs all access to PCI configuration space
>> (ECAM) to be synchronized among all entities, e.g. hardware domain and
>> guests.
>
> I am not entirely sure what you mean by "synchronized" here. Are you refer to 
> the content of the configuration space?

We maintain hwdom's and guest's view of the registers we are interested in

So, to have hwdom's view we also need to trap its access to the configuration 
space.

Probably "synchronized" is not the right wording here.

>
>> For that implement PCI host bridge specific callbacks to
>> properly setup those ranges depending on particular host bridge
>> implementation.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>> ---
>>   xen/arch/arm/pci/ecam.c            | 11 +++++++++++
>>   xen/arch/arm/pci/pci-host-common.c | 16 ++++++++++++++++
>>   xen/arch/arm/vpci.c                | 13 +++++++++++++
>>   xen/include/asm-arm/pci.h          |  8 ++++++++
>>   4 files changed, 48 insertions(+)
>>
>> diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
>> index 91c691b41fdf..92ecb2e0762b 100644
>> --- a/xen/arch/arm/pci/ecam.c
>> +++ b/xen/arch/arm/pci/ecam.c
>> @@ -42,6 +42,16 @@ void __iomem *pci_ecam_map_bus(struct pci_host_bridge 
>> *bridge,
>>       return base + (PCI_DEVFN(sbdf_t.dev, sbdf_t.fn) << devfn_shift) + 
>> where;
>>   }
>>   +static int pci_ecam_register_mmio_handler(struct domain *d,
>> +                                          struct pci_host_bridge *bridge,
>> +                                          const struct mmio_handler_ops 
>> *ops)
>> +{
>> +    struct pci_config_window *cfg = bridge->sysdata;
>> +
>> +    register_mmio_handler(d, ops, cfg->phys_addr, cfg->size, NULL);
>
> We have a fixed array for handling the MMIO handlers.

Didn't know that:

xen/include/asm-arm/mmio.h:27:#define MAX_IO_HANDLER  16

> So you need to make sure we have enough space in it to store one handler per 
> handler.
>
> This is quite similar to the problem we had with the re-distribuor on GICv3. 
> Have a look there to see how we dealt with it.

Could you please point me to that solution? I can only see

     /* Register mmio handle for the Distributor */
     register_mmio_handler(d, &vgic_distr_mmio_handler, d->arch.vgic.dbase,
                           SZ_64K, NULL);

     /*
      * Register mmio handler per contiguous region occupied by the
      * redistributors. The handler will take care to choose which
      * redistributor is targeted.
      */
     for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
     {
         struct vgic_rdist_region *region = &d->arch.vgic.rdist_regions[i];

         register_mmio_handler(d, &vgic_rdistr_mmio_handler,
                               region->base, region->size, region);
     }
which IMO doesn't care about the number of MMIOs we can handle

>
>> +    return 0;
>> +}
>> +
>>   /* ECAM ops */
>>   const struct pci_ecam_ops pci_generic_ecam_ops = {
>>       .bus_shift  = 20,
>> @@ -49,6 +59,7 @@ const struct pci_ecam_ops pci_generic_ecam_ops = {
>>           .map_bus                = pci_ecam_map_bus,
>>           .read                   = pci_generic_config_read,
>>           .write                  = pci_generic_config_write,
>> +        .register_mmio_handler  = pci_ecam_register_mmio_handler,
>>       }
>>   };
>>   diff --git a/xen/arch/arm/pci/pci-host-common.c 
>> b/xen/arch/arm/pci/pci-host-common.c
>> index d2fef5476b8e..a89112bfbb7c 100644
>> --- a/xen/arch/arm/pci/pci-host-common.c
>> +++ b/xen/arch/arm/pci/pci-host-common.c
>> @@ -318,6 +318,22 @@ struct dt_device_node *pci_find_host_bridge_node(struct 
>> device *dev)
>>       }
>>       return bridge->dt_node;
>>   }
>> +
>> +int pci_host_iterate_bridges(struct domain *d,
>> +                             int (*clb)(struct domain *d,
>
> NIT: We tend to name callback variable 'cb'.
Sure
>
> Cheers,
>
Thank you,

Oleksandr