From: Julien Grall <julien@xen.org>
To: Oleksandr Andrushchenko <andr2000@gmail.com>,
xen-devel@lists.xenproject.org
Cc: sstabellini@kernel.org, oleksandr_tyshchenko@epam.com,
volodymyr_babchuk@epam.com, Artem_Mygaiev@epam.com,
roger.pau@citrix.com, jbeulich@suse.com,
andrew.cooper3@citrix.com, george.dunlap@citrix.com,
paul@xen.org, bertrand.marquis@arm.com, rahul.singh@arm.com,
Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Subject: Re: [PATCH v6 3/7] xen/arm: setup MMIO range trap handlers for hardware domain
Date: Tue, 16 Nov 2021 19:12:23 +0000 [thread overview]
Message-ID: <2eb6b4e8-95e1-9566-3209-c28964b0d643@xen.org> (raw)
In-Reply-To: <20211105063326.939843-4-andr2000@gmail.com>
Hi Oleksandr,
On 05/11/2021 06:33, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> In order for vPCI to work it needs to maintain guest and hardware
> domain's views of the configuration space. For example, BARs and
> COMMAND registers require emulation for guests and the guest view
> of the registers needs to be in sync with the real contents of the
> relevant registers. For that ECAM address space needs to also be
> trapped for the hardware domain, so we need to implement PCI host
> bridge specific callbacks to properly setup MMIO handlers for those
> ranges depending on particular host bridge implementation.
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
> Since v5:
> - add vpci_sbdf_from_gpa helper for gpa to SBDF translation
> - take bridge's bus start into account while calculating SBDF
> Since v4:
> - unsigned int for functions working with count
> - gate number of MMIO handlers needed for CONFIG_HAS_PCI_MSI
> and fix their number, e.g. single handler for PBA and
> MSI-X tables (Roger)
> - re-work code for assigning MMIO handlers to be simpler
> and account on the fact that there could multiple host bridges
> exist for the hwdom
> Since v3:
> - fixed comment formatting
> Since v2:
> - removed unneeded assignment (count = 0)
> - removed unneeded header inclusion
> - update commit message
> Since v1:
> - Dynamically calculate the number of MMIO handlers required for vPCI
> and update the total number accordingly
> - s/clb/cb
> - Do not introduce a new callback for MMIO handler setup
> ---
> xen/arch/arm/domain.c | 2 +
> xen/arch/arm/pci/pci-host-common.c | 27 ++++++++++++
> xen/arch/arm/vpci.c | 66 ++++++++++++++++++++++++++----
> xen/arch/arm/vpci.h | 6 +++
> xen/include/asm-arm/pci.h | 5 +++
> 5 files changed, 98 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 96e1b235501d..92a6c509e5c5 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -739,6 +739,8 @@ int arch_domain_create(struct domain *d,
> if ( (rc = domain_vgic_register(d, &count)) != 0 )
> goto fail;
>
> + count += domain_vpci_get_num_mmio_handlers(d);
> +
> if ( (rc = domain_io_init(d, count + MAX_IO_HANDLER)) != 0 )
> goto fail;
>
> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
> index 47104b22b221..0d271a6e8881 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -289,6 +289,33 @@ int pci_get_host_bridge_segment(const struct dt_device_node *node,
> return -EINVAL;
> }
>
> +int pci_host_iterate_bridges(struct domain *d,
> + int (*cb)(struct domain *d,
> + struct pci_host_bridge *bridge))
> +{
> + struct pci_host_bridge *bridge;
> + int err;
> +
> + list_for_each_entry( bridge, &pci_host_bridges, node )
> + {
> + err = cb(d, bridge);
> + if ( err )
> + return err;
> + }
> + return 0;
> +}
> +
> +unsigned int pci_host_get_num_bridges(void)
> +{
> + struct pci_host_bridge *bridge;
> + unsigned int count = 0;
How about making this static and...
> +
> + list_for_each_entry( bridge, &pci_host_bridges, node )
> + count++;
... only call list_for_each_entry() when count is -1? So we would only
go through the list once.
This should be fine given hostbridge can only be added during boot (we
would need to protect pci_host_bridges with a lock otherwise).
> +
> + return count;
> +}
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
> index 23f45386f4b3..5a6ebd8b9868 100644
> --- a/xen/arch/arm/vpci.c
> +++ b/xen/arch/arm/vpci.c
> @@ -16,16 +16,31 @@
>
> #include <asm/mmio.h>
>
> +static pci_sbdf_t vpci_sbdf_from_gpa(const struct pci_host_bridge *bridge,
> + paddr_t gpa)
> +{
> + pci_sbdf_t sbdf;
> +
> + if ( bridge )
> + {
> + sbdf.sbdf = VPCI_ECAM_BDF(gpa - bridge->cfg->phys_addr);
> + sbdf.seg = bridge->segment;
> + sbdf.bus += bridge->cfg->busn_start;
> + }
> + else
> + sbdf.sbdf = VPCI_ECAM_BDF(gpa - GUEST_VPCI_ECAM_BASE);
> +
> + return sbdf;
> +}
> +
> static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
> register_t *r, void *p)
> {
> - pci_sbdf_t sbdf;
> + struct pci_host_bridge *bridge = p;
> + pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
> /* data is needed to prevent a pointer cast on 32bit */
> unsigned long data;
>
> - /* We ignore segment part and always handle segment 0 */
> - sbdf.sbdf = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE);
> -
> if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
> 1U << info->dabt.size, &data) )
> {
> @@ -41,10 +56,8 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
> static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
> register_t r, void *p)
> {
> - pci_sbdf_t sbdf;
> -
> - /* We ignore segment part and always handle segment 0 */
> - sbdf.sbdf = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE);
> + struct pci_host_bridge *bridge = p;
> + pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
>
> return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa),
> 1U << info->dabt.size, r);
> @@ -55,17 +68,54 @@ static const struct mmio_handler_ops vpci_mmio_handler = {
> .write = vpci_mmio_write,
> };
>
> +static int vpci_setup_mmio_handler_cb(struct domain *d,
> + struct pci_host_bridge *bridge)
> +{
> + struct pci_config_window *cfg = bridge->cfg;
> +
> + register_mmio_handler(d, &vpci_mmio_handler,
> + cfg->phys_addr, cfg->size, bridge);
> + return 0;
> +}
> +
> int domain_vpci_init(struct domain *d)
> {
> if ( !has_vpci(d) )
> return 0;
>
> + if ( is_hardware_domain(d) )
> + return pci_host_iterate_bridges(d, vpci_setup_mmio_handler_cb);
> +
> + /* Guest domains use what is programmed in their device tree. */
I would rather not make the assumption that the guest is using a
Device-Tree. So how about:
/*
* The hardware domain gets one virtual hostbridge by "real"
* hostbridges.
* Guests get the virtual platform layout (one virtual host bridge for
* now).
*/
The comment would have to be moved before if ( is_hardware_domain(d) ).
> register_mmio_handler(d, &vpci_mmio_handler,
> GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL);
>
> return 0;
> }
>
> +unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d)
AFAICT, this function would also be called even if vPCI is not enabled
for the domain. So we should add:
if ( !has_vpci(d) )
return 0;
> +{
> + unsigned int count;
> +
> + if ( is_hardware_domain(d) )
> + /* For each PCI host bridge's configuration space. */
> + count = pci_host_get_num_bridges();
This first part makes sense to me. But...
> + else
... I don't understand how the else is related to this commit. Can you
clarify it?
> + /*
> + * There's a single MSI-X MMIO handler that deals with both PBA
> + * and MSI-X tables per each PCI device being passed through.
> + * Maximum number of supported devices is 32 as virtual bus
> + * topology emulates the devices as embedded endpoints.
> + * +1 for a single emulated host bridge's configuration space.
> + */
> + count = 1;
> +#ifdef CONFIG_HAS_PCI_MSI
> + count += 32;
Surely, this is a decision that is based on other factor in the vPCI
code. So can use a define and avoid hardcoding the number?
> +#endif
> +
> + return count;
> +}
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h
> index d8a7b0e3e802..3c713f3fcdb5 100644
> --- a/xen/arch/arm/vpci.h
> +++ b/xen/arch/arm/vpci.h
> @@ -17,11 +17,17 @@
>
> #ifdef CONFIG_HAS_VPCI
> int domain_vpci_init(struct domain *d);
> +unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d);
> #else
> static inline int domain_vpci_init(struct domain *d)
> {
> return 0;
> }
> +
> +static inline unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d)
> +{
> + return 0;
> +}
> #endif
>
> #endif /* __ARCH_ARM_VPCI_H__ */
> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
> index c20eba643d86..969333043431 100644
> --- a/xen/include/asm-arm/pci.h
> +++ b/xen/include/asm-arm/pci.h
> @@ -110,6 +110,11 @@ void arch_pci_init_pdev(struct pci_dev *pdev);
>
> int pci_get_new_domain_nr(void);
>
> +int pci_host_iterate_bridges(struct domain *d,
> + int (*clb)(struct domain *d,
NIT: This is more common to call a callback 'cb'. In any case, I would
prefer if the names matches the one used in the implementation.
> + struct pci_host_bridge *bridge));
> +unsigned int pci_host_get_num_bridges(void);
> +
> #else /*!CONFIG_HAS_PCI*/
>
> struct arch_pci_dev { };
>
Cheers,
--
Julien Grall
next prev parent reply other threads:[~2021-11-16 19:13 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-05 6:33 [PATCH v6 0/7] PCI devices passthrough on Arm, part 2 Oleksandr Andrushchenko
2021-11-05 6:33 ` [PATCH v6 1/7] xen/arm: rename DEVICE_PCI to DEVICE_PCI_HOSTBRIDGE Oleksandr Andrushchenko
2021-11-16 18:26 ` Julien Grall
2021-11-05 6:33 ` [PATCH v6 2/7] xen/arm: add pci-domain for disabled devices Oleksandr Andrushchenko
2021-11-16 18:48 ` Julien Grall
2021-11-17 6:56 ` Oleksandr Andrushchenko
2021-11-17 21:33 ` Julien Grall
2021-11-18 7:13 ` Oleksandr Andrushchenko
2021-11-22 15:29 ` Julien Grall
2021-11-22 16:23 ` Oleksandr Andrushchenko
2021-11-22 17:17 ` Julien Grall
2021-11-23 6:31 ` Oleksandr Andrushchenko
2021-11-23 16:05 ` Julien Grall
2021-11-23 16:44 ` Oleksandr Andrushchenko
2021-11-23 17:15 ` Julien Grall
2021-11-24 6:54 ` Oleksandr Andrushchenko
2021-11-05 6:33 ` [PATCH v6 3/7] xen/arm: setup MMIO range trap handlers for hardware domain Oleksandr Andrushchenko
2021-11-09 9:20 ` Oleksandr Andrushchenko
2021-11-16 19:12 ` Julien Grall [this message]
2021-11-18 7:27 ` Oleksandr Andrushchenko
2021-11-18 10:46 ` Oleksandr Andrushchenko
2021-11-22 17:36 ` Julien Grall
2021-11-23 6:58 ` Oleksandr Andrushchenko
2021-11-23 16:12 ` Julien Grall
2021-11-23 16:41 ` Oleksandr Andrushchenko
2021-11-23 16:58 ` Julien Grall
2021-11-24 7:22 ` Oleksandr Andrushchenko
2021-11-05 6:33 ` [PATCH v6 4/7] xen/arm: do not map PCI ECAM and MMIO space to Domain-0's p2m Oleksandr Andrushchenko
2021-11-23 16:42 ` Julien Grall
2021-11-24 7:42 ` Oleksandr Andrushchenko
2021-11-05 6:33 ` [PATCH v6 5/7] xen/arm: do not map IRQs and memory for disabled devices Oleksandr Andrushchenko
2021-11-16 19:22 ` Julien Grall
2021-11-18 6:59 ` Oleksandr Andrushchenko
2021-11-22 19:31 ` Julien Grall
2021-11-23 7:23 ` Oleksandr Andrushchenko
2021-11-05 6:33 ` [PATCH v6 6/7] xen/arm: process pending vPCI map/unmap operations Oleksandr Andrushchenko
2021-11-05 7:40 ` Jan Beulich
2021-11-17 21:26 ` Julien Grall
2021-11-05 6:33 ` [PATCH v6 7/7] xen/arm: do not use void pointer in pci_host_common_probe Oleksandr Andrushchenko
2021-11-17 11:12 ` Rahul Singh
2021-11-17 21:45 ` Julien Grall
2021-11-18 7:34 ` Oleksandr Andrushchenko
2021-11-22 17:48 ` Julien Grall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2eb6b4e8-95e1-9566-3209-c28964b0d643@xen.org \
--to=julien@xen.org \
--cc=Artem_Mygaiev@epam.com \
--cc=andr2000@gmail.com \
--cc=andrew.cooper3@citrix.com \
--cc=bertrand.marquis@arm.com \
--cc=george.dunlap@citrix.com \
--cc=jbeulich@suse.com \
--cc=oleksandr_andrushchenko@epam.com \
--cc=oleksandr_tyshchenko@epam.com \
--cc=paul@xen.org \
--cc=rahul.singh@arm.com \
--cc=roger.pau@citrix.com \
--cc=sstabellini@kernel.org \
--cc=volodymyr_babchuk@epam.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).