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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Rahul!

On 15.09.21 23:33, Stefano Stabellini wrote:
> On Wed, 15 Sep 2021, Rahul Singh wrote:
>> Hi Oleksandr, Stefano,
>>
>>> On 15 Sep 2021, at 6:30 am, Oleksandr Andrushchenko 
>>> <Oleksandr_Andrushchenko@xxxxxxxx> wrote:
>>>
>>> Hi, Rahul!
>>>
>>> On 14.09.21 17:24, Oleksandr Andrushchenko wrote:
>>>> }
>>>>>>    +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);
>>>>>> +    return 0;
>>>>>> +}
>>>>> Given that struct pci_config_window is generic (it is not specific to
>>>>> one bridge), I wonder if we even need the .register_mmio_handler
>>>>> callback here.
>>>>>
>>>>> In fact,pci_host_bridge->sysdata doesn't even need to be a void*, it
>>>>> could be a struct pci_config_window*, right?
>>>> Rahul, this actually may change your series.
>>>>
>>>> Do you think we can do that?
>>>>
>>> This is the only change requested that left unanswered by now.
>>>
>>> Will it be possible that you change the API accordingly, so I can
>>>
>>> implement as Stefano suggests?
>> We need pci_host_bridge->sysdata as void* in case we need to implement the 
>> new non-ecam PCI controller in XEN.
>> Please have a look once in Linux code[1] how bridge->sysdata will be used. 
>> struct pci_config_window is used only for
>> ecam supported host controller. Different PCI host controller will have 
>> different PCI interface to access the PCI controller.
>>
>> [1] 
>> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pcie-rcar-host.c*n309__;Iw!!GF_29dbcQIUBPA!kjkv6KIlvXOEgVaS6BNPRo0gyABihhO0XmNHRPFJaFAGhhTEuK1mIsWqPs0cXEipzkT_MmA-Eg$
>>  [git[.]kernel[.]org]
>>
>> I think we need bridge->sysdata in future to implement the new PCI 
>> controller.
> In my opinion the pci_config_window is too important of an information
> to be left inside an opaque pointer, especially when the info under
> pci_config_window is both critical and vendor-neutral.
>
> My preference would be something like this:
>
>
> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
> index 9c28a4bdc4..c80d846da3 100644
> --- a/xen/include/asm-arm/pci.h
> +++ b/xen/include/asm-arm/pci.h
> @@ -55,7 +55,6 @@ struct pci_config_window {
>       uint8_t         busn_start;
>       uint8_t         busn_end;
>       void __iomem    *win;
> -    const struct    pci_ecam_ops *ops;
>   };
>   
>   /*
> @@ -68,7 +67,8 @@ struct pci_host_bridge {
>       uint16_t segment;                /* Segment number */
>       u8 bus_start;                    /* Bus start of this bridge. */
>       u8 bus_end;                      /* Bus end of this bridge. */
> -    void *sysdata;                   /* Pointer to the config space window*/
> +    struct pci_config_window* cfg;   /* Pointer to the bridge config window 
> */
> +    void *sysdata;                   /* Pointer to bridge private data */
>       const struct pci_ops *ops;
>   };
>
>
> As a reference the attached patch builds. However, I had to remove const
> where struct pci_ecam_ops *ops is used.

I'd like to know which route we go with this as this is now the last

thing which stops me from sending v2 of this series.

Thank you,

Oleksandr