[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v1 06/14] xen/arm: Add support for PCI ecam operations

  • To: Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • From: Rahul Singh <Rahul.Singh@xxxxxxx>
  • Date: Wed, 15 Sep 2021 16:38:45 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=LXA62+xMX11sxy88lI6BLc2NogFQu9Gt+R69UZkk75M=; b=RsqB1tMsIyPD58DrIDVXVEPDcMQJ1JlfGHyDn0skhaQuu9lEXkVcyKmFOGDhiltfC8uQtx13LpK7bfGZNn7wKhUnxuEWf/1oK4cAkEpxZumQikCBh0p1UMv7pYplALCDdEErjg0vV586MTjmEz6AY2ZC3sNsTRabprCE1+r6WqVZ1mcfIVq5DGsg4pYhEWGwtiOVP3LhqBa7skI4Ym/iQaloZgddf7Nkt2Oef82WO5nQHkOar0mzE1PY+IzhDQwnbGwb/1/4patHzwXqnt9/VlzBwTW7HUBNpYr/dv1Byygl702WeKTU+4MIrMUz/8ZdOzkyVi/6GJvpp5RrI+M51g==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=VK1nn+Nd3PfZoTvsbno56pPfOrTKQWBG2Ce1/JR78fiF2uvBrawCX8ZOnNuJOiIwczcAK0SSBpMr7SMLw3Ju5KA+LFYGISAjJV5l7/JvzJIq5Q25BQGTWrn6HH/zSZk9B2ly72yo83bs9oiAfugWh8qgof+jCtrPg/ihHXaNL2KfRhanc9fVw4VrncW/sILQxxxvTWMLrD9ZdIcpCzmP1/HjxBS1W0yrPAWfclU1LSsdAtvM/MBluEe3BbRdJr/c8PWPPXCW65v0am6+B+66DTmM0wAjI6yEQM+yOFL4+a5q1E/ExW2SkmAMFTWepcrUkTARWP21TVPGWliukUIEbA==
  • Authentication-results-original: kernel.org; dkim=none (message not signed) header.d=none;kernel.org; dmarc=none action=none header.from=arm.com;
  • Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, Julien Grall <julien@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Wed, 15 Sep 2021 16:39:35 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: kernel.org; dkim=none (message not signed) header.d=none;kernel.org; dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHXlPK6hMBa2cV9wU+RB+r01dTauquceRAAgAcQPwCAAMcsAIABJesA
  • Thread-topic: [PATCH v1 06/14] xen/arm: Add support for PCI ecam operations

Hi Stefano,

> On 15 Sep 2021, at 12:06 am, Stefano Stabellini <sstabellini@xxxxxxxxxx> 
> wrote:
> On Tue, 14 Sep 2021, Rahul Singh wrote:
>>>> +        return NULL;
>>>> +
>>>> +    busn -= cfg->busn_start;
>>>> +    base = cfg->win + (busn << cfg->ops->bus_shift);
>>>> +
>>>> +    return base + (PCI_DEVFN(sbdf_t.dev, sbdf_t.fn) << devfn_shift) + 
>>>> where;
>>>> +}
>>> I understand that the arm32 part is not implemented and not part of this
>>> series, that's fine. However if the plan is that arm32 will dynamically
>>> map each bus individually, then I imagine this function will have an
>>> ioremap in the arm32 version. Which means that we also need an
>>> unmap_bus call in struct pci_ops. I understand that pci_ecam_unmap_bus
>>> would be a NOP today for arm64, but I think it makes sense to have it if
>>> we want the API to be generic.
>> As per my understanding we don’t need pci_ecam_unmap_bus(..) as I don’t see 
>> any use case to unmap the 
>> bus dynamically. We can add the support for per_bus_mapping for ARM32 once 
>> we implement arm32 part.
>> Let me know your view on this.
> In the patch titled "xen/arm: PCI host bridge discovery within XEN on
> ARM" there is the following in-code comment:
> * On 64-bit systems, we do a single ioremap for the whole config space
> * since we have enough virtual address range available.  On 32-bit, we
> * ioremap the config space for each bus individually.
> *
> * As of now only 64-bit is supported 32-bit is not supported.
> So I take it that on arm32 we don't have enough virtual address range
> available, therefore we cannot ioremap the whole range. Instead, we'll
> have to ioremap the config space of each bus individually.

Yes you are right my understand is also same.
> I assumed that the idea was to call ioremap and iounmap dynamically,
> otherwise the total amount of virtual address range required would still
> be the same.

As per my understanding for 32-bit we need per_bus mapping as we don’t have 
enough virtual address space in one chunk
but we can have virtual address space in different chunk.

I am not sure if we need to map/unmap the virtual address space for each 
read/write call. 
I just checked the Linux code[1]  and there also mapping is done once not for 
each read/write call.


> I also thought that the dynamic mapping function, the one
> which would end up calling ioremap on arm32, would be pci_ecam_map_bus.
> If so, then we are missing the corresponding unmapping function, the one
> that would call iounmap on arm32 and do nothing on arm64, called before
> returning from pci_generic_config_read and pci_generic_config_write.
> As always, I am not asking for the arm32 implementation, but if we are
> introducing internal APIs, it would be good that they are consistent.
>>>> @@ -50,10 +51,41 @@ struct pci_host_bridge {
>>>>    u8 bus_start;                    /* Bus start of this bridge. */
>>>>    u8 bus_end;                      /* Bus end of this bridge. */
>>>>    void *sysdata;                   /* Pointer to the config space window*/
>>>> +    const struct pci_ops *ops;
>>>> };
>>>> +struct pci_ops {
>>>> +    void __iomem *(*map_bus)(struct pci_host_bridge *bridge, uint32_t 
>>>> sbdf,
>>>> +                             uint32_t offset);
>>>> +    int (*read)(struct pci_host_bridge *bridge, uint32_t sbdf,
>>>> +                uint32_t reg, uint32_t len, uint32_t *value);
>>>> +    int (*write)(struct pci_host_bridge *bridge, uint32_t sbdf,
>>>> +                 uint32_t reg, uint32_t len, uint32_t value);
>>>> +};
>>>> +
>>>> +/*
>>>> + * struct to hold pci ops and bus shift of the config window
>>>> + * for a PCI controller.
>>>> + */
>>>> +struct pci_ecam_ops {
>>>> +    unsigned int            bus_shift;
>>>> +    struct pci_ops          pci_ops;
>>>> +    int (*init)(struct pci_config_window *);
>>> Is this last member of the struct needed/used?
>> Yes It will be used by some vendor specific board( N1SDP ). Please check 
>> below.
>> https://git.linaro.org/landing-teams/working/arm/n1sdp-pcie-quirk.git/commit/?id=04b7e76d0fe6481a803f58e54e008a1489d713a5
> OK. I don't doubt that there might be bridge-specific initializations,
> but we already have things like:
> .dt_match = gen_pci_dt_match,
> .init = gen_pci_dt_init,
> The question is do we actually need two different vendor-specific init
> functions? One is driven by device tree, e.g. ZynqMP is calling
> gen_pci_dt_init. The other one is pci_ecam_ops->init, which is called
> from the following chain:
> DT_DEVICE_START -> gen_pci_dt_init -> pci_host_common_probe ->
> gen_pci_init -> pci_generic_ecam_ops.init
> What's the difference between gen_pci_dt_init and pci_ecam_ops.init in
> terms of purpose?

Yes I also agree with you we don’t need two init function. I will modify the 
code like below.
DT_DEVICE_START ->pci_host_common_probe -> gen_pci_init -> 

> I am happy to have pci_ecam_ops.init if it serves a different purpose
> from DT_DEVICE_START.init. In that case we might want to add an in-code
> comment so that an engineer would know where to add vendor-specific code
> (whether to DT_DEVICE_START.init or to pci_ecam_ops.init).



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.