From: Julien Grall <julien@xen.org>
To: Rahul Singh <rahul.singh@arm.com>, xen-devel@lists.xenproject.org
Cc: bertrand.marquis@arm.com,
"Stefano Stabellini" <sstabellini@kernel.org>,
"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
"Jan Beulich" <jbeulich@suse.com>, "Paul Durrant" <paul@xen.org>,
"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH v1 11/14] xen/arm: Enable the existing x86 virtual PCI support for ARM.
Date: Thu, 9 Sep 2021 14:50:24 +0100 [thread overview]
Message-ID: <a868e1e7-8400-45df-6eaa-69f1e2c99383@xen.org> (raw)
In-Reply-To: <370f4f87c148eaee5ac5ec69346828e6473f0f2d.1629366665.git.rahul.singh@arm.com>
Hi Rahul,
On 19/08/2021 13:02, Rahul Singh wrote:
> The existing VPCI support available for X86 is adapted for Arm.
> When the device is added to XEN via the hyper call
> “PHYSDEVOP_pci_device_add”, VPCI handler for the config space
> access is added to the Xen to emulate the PCI devices config space.
>
> A MMIO trap handler for the PCI ECAM space is registered in XEN
> so that when guest is trying to access the PCI config space,XEN
> will trap the access and emulate read/write using the VPCI and
> not the real PCI hardware.
>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
> xen/arch/arm/Makefile | 1 +
> xen/arch/arm/domain.c | 4 ++
> xen/arch/arm/vpci.c | 96 +++++++++++++++++++++++++++++++++++
> xen/arch/arm/vpci.h | 37 ++++++++++++++
> xen/drivers/passthrough/pci.c | 7 +++
> xen/drivers/vpci/Makefile | 3 +-
> xen/drivers/vpci/header.c | 2 +
> xen/include/asm-arm/domain.h | 5 +-
> xen/include/asm-arm/pci.h | 8 +++
> xen/include/public/arch-arm.h | 4 ++
> 10 files changed, 165 insertions(+), 2 deletions(-)
> create mode 100644 xen/arch/arm/vpci.c
> create mode 100644 xen/arch/arm/vpci.h
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 0e14a5e5c8..7cdce684a4 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -7,6 +7,7 @@ obj-y += platforms/
> endif
> obj-$(CONFIG_TEE) += tee/
> obj-$(CONFIG_HAS_PCI) += pci/
> +obj-$(CONFIG_HAS_VPCI) += vpci.o
>
> obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o
> obj-y += bootfdt.init.o
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 19c756ac3d..d99c653626 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -40,6 +40,7 @@
> #include <asm/vtimer.h>
>
> #include "vuart.h"
> +#include "vpci.h"
Please order the includes alphabetically. So this one should go before
"vuart.h".
>
> DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
>
> @@ -767,6 +768,9 @@ int arch_domain_create(struct domain *d,
> if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) )
> goto fail;
>
> + if ( (rc = domain_vpci_init(d)) != 0 )
> + goto fail;
> +
> return 0;
>
> fail:
> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
> new file mode 100644
> index 0000000000..da8b1ca13c
> --- /dev/null
> +++ b/xen/arch/arm/vpci.c
> @@ -0,0 +1,96 @@
> +/*
> + * xen/arch/arm/vpci.c
> + * Copyright (c) 2021 Arm Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +#include <xen/sched.h>
NIT: Please add a newline between generic and arch specific includes.
> +#include <asm/mmio.h>
> +
> +/* Do some sanity checks. */
> +static bool vpci_mmio_access_allowed(unsigned int reg, unsigned int len)
> +{
> + /* Check access size. */
> + if ( len != 1 && len != 2 && len != 4 && len != 8 )
> + return false;
You will allow all the possible value of len (this is coming from the
HW). So I feels this is a bit too much to check for every I/O to the vPCI.
If you really want to keep the check then you can simply check that len
is < 8 because the two callers compute it with (1 << S). So there is no
way to set it 3, 5, 6 and 7.
> +
> + /* Check that access is size aligned. */
> + if ( (reg & (len - 1)) )
> + return false;
> +
> + return true;
> +}
> +
> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
> + register_t *r, void *p)
> +{
> + unsigned int reg;
> + pci_sbdf_t sbdf;
> + uint32_t data = 0;
> + unsigned int size = 1U << info->dabt.size;
> +
> + sbdf.sbdf = (((info->gpa) & 0x0ffff000) >> 12);
> + reg = (((info->gpa) & 0x00000ffc) | (info->gpa & 3));
This logic is the same as below in vpci_mmio_write(). So I think you
want to provide an helper because this is not trivial to read.
Also, for the first line, I think you can re-use MMCFG_BDF() from the
x86 code. For the second line, I would define the value so it is clearer
to understand that they mean (although & 3 is fine to me) .
> +
> + if ( !vpci_mmio_access_allowed(reg, size) )
> + return 1;
So, you will a guest will read 0 if the access is unaligned. This seems
an odd behavior given this is not an allowed access. AFAIU, the HW would
likely trow a data abort because you can't do unalign access on
uncachable memory. So I think we should return 0 here to let the MMIO
handler inject a data abort.
> +
> + data = vpci_read(sbdf, reg, size);
So in vpci_mmio_access_allowed(), you will allow a guest to read a
64-bit value. But... vpci_read() will return a 32-bit value.
Looking at the x86 code, they have a second call to vpci_read() to
handle the top 32-bit. Any reason why this was not implemented on Arm?
If we need to implement it then I think this should be implement in
vpci_read() to avoid duplication between x86 and arm.
> +
> + memcpy(r, &data, size);
From my understanding, any unused bit should be 0. So this should be:
*r = data;
> +
> + return 1;
> +}
> +
> +static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
> + register_t r, void *p)
My remarks on vpci_mmio_read() applies here too.
> +{
> + unsigned int reg;
> + pci_sbdf_t sbdf;
> + uint32_t data = r;
> + unsigned int size = 1U << info->dabt.size;
> +
> + sbdf.sbdf = (((info->gpa) & 0x0ffff000) >> 12);
> + reg = (((info->gpa) & 0x00000ffc) | (info->gpa & 3));
> +
> + if ( !vpci_mmio_access_allowed(reg, size) )
> + return 1;
> +
> + vpci_write(sbdf, reg, size, data);
> +
> + return 1;
> +}
> +
> +static const struct mmio_handler_ops vpci_mmio_handler = {
> + .read = vpci_mmio_read,
> + .write = vpci_mmio_write,
> +};
> +
> +int domain_vpci_init(struct domain *d)
> +{
> + if ( !has_vpci(d) )
> + return 0;
> +
> + register_mmio_handler(d, &vpci_mmio_handler,
> + GUEST_VPCI_ECAM_BASE, GUEST_VPCI_ECAM_SIZE, NULL);
> +
> + return 0;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> +
> diff --git a/xen/arch/arm/vpci.h b/xen/arch/arm/vpci.h
> new file mode 100644
> index 0000000000..8a093bb705
> --- /dev/null
> +++ b/xen/arch/arm/vpci.h
> @@ -0,0 +1,37 @@
> +/*
> + * xen/arch/arm/vpci.h
> + * Copyright (c) 2021 Arm Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef __ARCH_ARM_VPCI_H__
> +#define __ARCH_ARM_VPCI_H__
> +
> +#ifdef CONFIG_HAS_VPCI
> +int domain_vpci_init(struct domain *d);
> +#else
> +static inline int domain_vpci_init(struct domain *d)
> +{
> + return 0;
> +}
> +#endif
> +
> +#endif /* __ARCH_ARM_VPCI_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index c23c8cb06b..56e261e9bd 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -767,6 +767,13 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> else
> iommu_enable_device(pdev);
>
> +#ifdef CONFIG_ARM
> + ret = vpci_add_handlers(pdev);
> + if ( ret ) {
> + printk(XENLOG_ERR "setup of vPCI for failed: %d\n",ret);
> + goto out;
> + }
> +#endif
> pci_enable_acs(pdev);
>
> out:
> diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile
> index 55d1bdfda0..1a1413b93e 100644
> --- a/xen/drivers/vpci/Makefile
> +++ b/xen/drivers/vpci/Makefile
> @@ -1 +1,2 @@
> -obj-y += vpci.o header.o msi.o msix.o
> +obj-y += vpci.o header.o
> +obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index ba9a036202..f8cd55e7c0 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -96,8 +96,10 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
> * FIXME: punching holes after the p2m has been set up might be racy for
> * DomU usage, needs to be revisited.
> */
> +#ifdef CONFIG_HAS_PCI_MSI
> if ( map && !rom_only && vpci_make_msix_hole(pdev) )
> return;
> +#endif
>
> for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
> {
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index c9277b5c6d..d742b94bd6 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -262,7 +262,10 @@ static inline void arch_vcpu_block(struct vcpu *v) {}
>
> #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag)
>
> -#define has_vpci(d) ({ (void)(d); false; })
> +/* For X86 VPCI is enabled and tested for PVH DOM0 only but
> + * for ARM we enable support VPCI for guest domain also.
> + */
> +#define has_vpci(d) ({ (void)(d); IS_ENABLED(CONFIG_HAS_VPCI); })
>
> #endif /* __ASM_DOMAIN_H__ */
>
> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
> index 756f8637ab..c58152de80 100644
> --- a/xen/include/asm-arm/pci.h
> +++ b/xen/include/asm-arm/pci.h
> @@ -27,6 +27,14 @@ struct arch_pci_dev {
> struct device dev;
> };
>
> +/* Arch-specific MSI data for vPCI. */
> +struct vpci_arch_msi {
> +};
> +
> +/* Arch-specific MSI-X entry data for vPCI. */
> +struct vpci_arch_msix_entry {
> +};
> +
> /*
> * struct to hold the mappings of a config space window. This
> * is expected to be used as sysdata for PCI controllers that
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 64a2ca30da..0a9749e768 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -422,6 +422,10 @@ typedef uint64_t xen_callback_t;
> #define GUEST_PL011_BASE xen_mk_ullong(0x22000000)
> #define GUEST_PL011_SIZE xen_mk_ullong(0x00001000)
>
> +/* VPCI ECAM mappings */
> +#define GUEST_VPCI_ECAM_BASE xen_mk_ullong(0x10000000)
All the values for the memory layout has been defined in ascending
order. So please add the vCPI at the correct place. If I am not
mistaken, dhis should be before the GUEST_ACPI_*.
> +#define GUEST_VPCI_ECAM_SIZE xen_mk_ullong(0x10000000)
Please document how to decide the size. This is important for the future
if we need to change the size.
Cheers,
--
Julien Grall
next prev parent reply other threads:[~2021-09-09 13:50 UTC|newest]
Thread overview: 98+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-19 12:02 [PATCH v1 00/14] PCI devices passthrough on Arm Rahul Singh
2021-08-19 12:02 ` [PATCH v1 01/14] xen/pci: Refactor MSI code that implements MSI functionality within XEN Rahul Singh
2021-08-19 12:18 ` Julien Grall
2021-08-19 14:16 ` Rahul Singh
2021-09-07 10:01 ` Julien Grall
2021-08-24 15:53 ` Jan Beulich
2021-08-31 12:31 ` Rahul Singh
2021-08-31 13:00 ` Jan Beulich
2021-08-26 13:23 ` Daniel P. Smith
2021-08-19 12:02 ` [PATCH v1 02/14] xen/pci: solve compilation error on ARM with HAS_PCI enabled Rahul Singh
2021-08-19 12:28 ` Julien Grall
2021-08-20 10:30 ` Rahul Singh
2021-08-20 11:37 ` Julien Grall
2021-08-20 11:55 ` Jan Beulich
2021-08-20 12:10 ` Julien Grall
2021-08-20 7:01 ` Jan Beulich
2021-08-20 11:21 ` Rahul Singh
2021-09-09 13:16 ` Julien Grall
2021-08-19 12:02 ` [PATCH v1 03/14] xen/pci: solve compilation error on ARM with ACPI && HAS_PCI Rahul Singh
2021-08-20 7:06 ` Jan Beulich
2021-08-20 11:41 ` Rahul Singh
2021-08-20 11:54 ` Jan Beulich
2021-09-09 1:11 ` Stefano Stabellini
2021-09-10 10:22 ` Rahul Singh
2021-08-19 12:02 ` [PATCH v1 04/14] xen/arm: Add support for PCI init to initialize the PCI driver Rahul Singh
2021-09-07 8:20 ` Julien Grall
2021-09-10 10:47 ` Rahul Singh
2021-09-09 1:16 ` Stefano Stabellini
2021-09-10 10:32 ` Rahul Singh
2021-08-19 12:02 ` [PATCH v1 05/14] xen/arm: PCI host bridge discovery within XEN on ARM Rahul Singh
2021-09-07 9:05 ` Julien Grall
2021-09-10 11:22 ` Rahul Singh
2021-09-10 11:53 ` Julien Grall
2021-09-09 22:54 ` Stefano Stabellini
2021-09-10 11:53 ` Rahul Singh
2021-09-13 14:52 ` Oleksandr Andrushchenko
2021-09-13 20:23 ` Stefano Stabellini
2021-09-14 4:35 ` Oleksandr Andrushchenko
2021-09-14 7:53 ` Oleksandr Andrushchenko
2021-08-19 12:02 ` [PATCH v1 06/14] xen/arm: Add support for PCI ecam operations Rahul Singh
2021-09-09 11:32 ` Julien Grall
2021-09-14 8:13 ` Rahul Singh
2021-09-09 23:21 ` Stefano Stabellini
2021-09-14 11:13 ` Rahul Singh
2021-09-14 23:06 ` Stefano Stabellini
2021-09-15 16:38 ` Rahul Singh
2021-09-15 20:45 ` Stefano Stabellini
2021-09-16 16:51 ` Rahul Singh
2021-08-19 12:02 ` [PATCH v1 07/14] xen/arm: Add support for Xilinx ZynqMP PCI host controller Rahul Singh
2021-09-09 23:34 ` Stefano Stabellini
2021-09-10 12:01 ` Rahul Singh
2021-09-13 14:46 ` Oleksandr Andrushchenko
2021-09-13 21:02 ` Stefano Stabellini
2021-09-14 4:31 ` Oleksandr Andrushchenko
2021-09-17 7:39 ` Rahul Singh
2021-08-19 12:02 ` [PATCH v1 08/14] xen:arm: Implement pci access functions Rahul Singh
2021-09-09 23:41 ` Stefano Stabellini
2021-09-14 16:05 ` Rahul Singh
2021-09-14 22:40 ` Stefano Stabellini
2021-09-15 7:54 ` Oleksandr Andrushchenko
2021-09-15 10:47 ` Rahul Singh
2021-08-19 12:02 ` [PATCH v1 09/14] xen/arm: Add cmdline boot option "pci=on" Rahul Singh
2021-08-19 12:09 ` Jan Beulich
2021-08-19 12:31 ` Julien Grall
2021-08-20 12:19 ` Rahul Singh
2021-08-20 14:34 ` Julien Grall
2021-08-20 14:37 ` Jan Beulich
2021-09-09 23:46 ` Stefano Stabellini
2021-09-09 23:48 ` Stefano Stabellini
2021-08-19 12:02 ` [PATCH v1 10/14] xen/arm: Discovering PCI devices and add the PCI devices in XEN Rahul Singh
2021-08-19 12:35 ` Julien Grall
2021-08-19 13:40 ` Jan Beulich
2021-08-20 13:05 ` Rahul Singh
2021-08-19 12:02 ` [PATCH v1 11/14] xen/arm: Enable the existing x86 virtual PCI support for ARM Rahul Singh
2021-08-24 16:09 ` Jan Beulich
2021-08-25 5:44 ` Oleksandr Andrushchenko
2021-08-25 6:35 ` Jan Beulich
2021-09-09 13:50 ` Julien Grall [this message]
2021-09-16 10:46 ` Rahul Singh
2021-09-10 0:26 ` Stefano Stabellini
2021-09-16 11:01 ` Rahul Singh
2021-09-16 20:26 ` Stefano Stabellini
2021-09-21 13:49 ` Rahul Singh
2021-09-21 21:38 ` Stefano Stabellini
2021-08-19 12:02 ` [PATCH v1 12/14] arm/libxl: Emulated PCI device tree node in libxl Rahul Singh
2021-08-19 13:00 ` Julien Grall
2021-08-20 16:03 ` Rahul Singh
2021-09-09 13:59 ` Julien Grall
2021-09-16 16:16 ` Rahul Singh
2021-09-10 0:51 ` Stefano Stabellini
2021-09-16 16:35 ` Rahul Singh
2021-08-19 12:02 ` [PATCH v1 13/14] xen/arm: Fixed error when PCI device is assigned to guest Rahul Singh
2021-08-19 12:12 ` Jan Beulich
2021-08-19 12:40 ` Julien Grall
2021-08-20 17:01 ` Rahul Singh
2021-08-19 12:02 ` [PATCH v1 14/14] xen/arm: Add linux,pci-domain property for hwdom if not available Rahul Singh
2021-09-10 1:00 ` Stefano Stabellini
2021-09-16 16:36 ` Rahul Singh
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=a868e1e7-8400-45df-6eaa-69f1e2c99383@xen.org \
--to=julien@xen.org \
--cc=Volodymyr_Babchuk@epam.com \
--cc=bertrand.marquis@arm.com \
--cc=jbeulich@suse.com \
--cc=paul@xen.org \
--cc=rahul.singh@arm.com \
--cc=roger.pau@citrix.com \
--cc=sstabellini@kernel.org \
--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).