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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Julien!

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?

>
> Cheers,
>
Thank you,

Oleksandr