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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 10.09.21 16:33, Julien Grall wrote:
>
>
> On 10/09/2021 14:27, Oleksandr Andrushchenko wrote:
>> Hi,
>>
>> On 10.09.21 16:20, Julien Grall wrote:
>>>
>>>
>>> On 10/09/2021 14:15, Oleksandr Andrushchenko wrote:
>>>> Hi, Julien!
>>>
>>> Hi,
>>>
>>>> On 10.09.21 16:04, Julien Grall wrote:
>>>>>
>>>>>
>>>>> On 10/09/2021 12:43, Oleksandr Andrushchenko wrote:
>>>>>> Hi, Julien!
>>>>>
>>>>> Hi Oleksandr,
>>>>>
>>>>>> 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.
>>>>> I would simply say that we want to expose an emulated hostbridge to the 
>>>>> dom0 so we need to unmap the configuration space.
>>>> Sounds good
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> 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
>>>>>
>>>>> Please see vgic_v3_init(). We update mmio_count that is then used as a 
>>>>> the second argument for domain_io_init().
>>>>
>>>> Ah, so
>>>>
>>>> 1) This needs to be done before the array for the handlers is allocated
>>>>
>>>> 2) How do I know at the time of 1) how many bridges we have?
>>>
>>> By counting the number of bridge you want to expose to dom0? I am not 
>>> entirely sure what else you expect me to say.
>>
>> Ok, so I'll go over the device tree and find out all the bridges, e.g. 
>> devices with DEVICE_PCI type.
>>
>> Then I'll also need to exclude those being passed through (xen,passthrough) 
>> and the rest are the bridges for Domain-0?
>
> What you want to know if how many times register_mmio_handler() will be 
> called from domain_vpci_init().
>
> You introduced a function pci_host_iterate_bridges() that will walk over the 
> bridges and then call the callback vpci_setup_mmio_handler(). So you could 
> introduce a new callback that will return 1 if 
> bridge->ops->register_mmio_handler is not NULL or 0.

Ok, clear. Something like:

     if ( (rc = domain_vgic_register(d, &count)) != 0 )
         goto fail;

     *find out how many bridges and update count*


     if ( (rc = domain_io_init(d, count + MAX_IO_HANDLER)) != 0 )
         goto fail;

>
> Cheers,
>
Thank you,

Oleksandr