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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Oleksandr,

> On 17 Sep 2021, at 7:13 am, Oleksandr Andrushchenko 
> <Oleksandr_Andrushchenko@xxxxxxxx> wrote:
> 
> 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.

I will modify the code as per Stefano request and will send the next version.

Regards,
Rahul
> 
> Thank you,
> 
> Oleksandr