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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

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?

Is this what you mean?

>
> Cheers,
>
Thank you,

Oleksandr