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

Re: [PATCH v5 08/11] xen/arm: Enable the existing x86 virtual PCI support for ARM.


  • To: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Wed, 13 Oct 2021 12:33:17 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.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:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=wVme+dt+GHnxXOGOyvRMjypqZzYv6JLcNENHaHfXkVg=; b=UweS1q1IkA0K/nksfAojAAiKltx3Mi3n5syl6sc92JO11ZsOGYO1uUnwg+vcBDiIeQ0W6hsMRKu0MMVm+dYaIrMuB5vhrG7ViIAoq+4jfS0nY/i1exMXLOysPeQ6Ijo1pw1ju0Koi6Emzr9cVW7zekwOhLKOZesD0ffW0KJmuCAQkkvBC1eDCqTqnbPcOKhT9Kx+fFfSlCCYo8ozpWOTf81VcCO/iIfmdO8w0EbmfnDFo3jJvZNrtU07UA1TNdMqFfh06cZRcPFejGwiXP0nrJCnvPsiV3HerCYi7KNpamCKKvmP6Q4nyc7sJVDwQsXslAyDbmAZ2kzkwODhwwRKHQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=i2fVI0JXAweA+HCyR7onWDcq3WG3acXLVnTXBw8Df2Cf7T0N6hvc6E6cN6Rw66nqnOCnd5FKx2VWfjSDp6DYTSdk5wx/c9cgFZ8F9RRLispQrt/ZRRjMPaiX2krq+R9tWR27lp9ToQc2GczwoaH7qtIN33VTYHpLNxuN7jDJO7kxoATPO9C8QbnCEVfcDKlR5ZTRopiL7osuTuIejPi+NE8zmKBX0ECCW2k5+95vSRrfnBUKD/yRiKrYOEGGB7pHDmFtPnMVzabUp5iIxYe6zXyEvuXMEN5GwGLpSzQ8lYh/gWmqGpyj1V1o5cjdrOd3JiHxbFcv8q8iZQJN9chLNw==
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Rahul Singh <Rahul.Singh@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andre Przywara <Andre.Przywara@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Wei Liu <wl@xxxxxxx>, Paul Durrant <paul@xxxxxxx>
  • Delivery-date: Wed, 13 Oct 2021 10:34:05 +0000
  • Ironport-data: A9a23:Ie2aMqOjCfzdP8XvrR2rkcFynXyQoLVcMsEvi/4bfWQNrUpz3zADy TYaXmzXaPePazTxKdwgao/ioEIC65WHm99iTQto+SlhQUwRpJueD7x1DKtR0wB+jCHnZBg6h ynLQoCYdKjYdpJYz/uUGuCJQUNUjMlkfZKhTr6ZUsxNbVU8En540Us4w7dRbrNA2rBVPSvc4 bsenOWHULOV82Yc3rU8sv/rRLtH5ZweiRtA1rAMTakjUGz2zhH5OKk3N6CpR0YUd6EPdgKMq 0Qv+5nilo/R109F5tpICd8XeGVSKlLZFVDmZna7x8FOK/WNz8A/+v9TCRYSVatYo2qSs49ui 4tpjo69GQgXebKRyOZGbxYNRkmSPYUekFPGCX22sMjVxEzaaXr8hf5pCSnaP6VBpLwxWzsXs 6VFdnZdNXhvhMrvqF6/YvNrick5atHiIasUu216zCGfBvEjKXzGa/iWu4MCh2Zs7ixINciHS c4yYH1NUAndfxtFGHlIWcIzh9790xETdBUH8QnI9MLb+VP7zRNz+KjgNsLPfd6HTtkTmVyXz kr4+GD+DgAfJcao4zOP+XKxhcfChSr+HokVEdWQ9PRnnVmSzWw7EwANWB2wpvzRola3RtZ3O 0ESvC00osAa60iDXtT7GRqirxasvAMYWtdWO/037keK0KW8yzyQBnUACAVAbtMmnMYsQHoh0 Vrht/nkHyB1ubuZD1eU7K6JrCiaMDIQa2QFYEcsXQYDptXuvow3phbOVcp4Vr64iMXvHjP9y CzMqzIx750IltIC3ai/+VHBghqvq4LPQwpz4R/YNkqM6A9jacidfZ659lHB5N5JNoPfRV6E1 EXogODHsrpIV8vU0nXQHqNdR9lF+sppLhWDmnIwANp/0w+/9i6sQ8dPvWF8f3tmZ5NslSDSX GffvgZY5Zl2NXSsbLNqb4/ZN/nG3ZQMBvy+CaiKNosmjoxZMVbdpnk3NBH4M3XFyRB0yckC1 YGnndFA5JrwIZ9syyaqXK8j2LsvyzFWKYj7FM2jkUrPPVZzYheopVY53LmmMr5RAECs+ly9H zNj2y2ikEo3vArWOHi/zGLrBQpWRUXX/LivwyCtSsaNIxB9BEYqAOLLzLUqduRNxvoOyr+Wo ivlABMDlDITYEErzy3RNRiPj5u1DP5CQY8TZ3RwbT5EJVByCWpQ0EvvX8RuJuR2nACS5fV1U +MEa6297gdnEVz6F8AmRcCl9uRKLU3z7SrXZnbNSGVvLvZIGl2Skve5L1SHycX7Jnfu3SfIi +b7jV2zrFtqb1kKMfs6n9r2lwnv4iNHxL4rN6YKS/EKEHjRHEFRA3WZptc8It0WKAWFwT2f1 g2MBgwfq/WLqIgwmOQlT4jex2twO+chTEdcAUfB6rO6aXvT8ma5mNcSW+eUZzHNEmjz/fz6N +lSyvj9NtwBnUpL7NUgQ+o6k/pm6ou9vaJewyRlAG7PMwahBIR/LyTUxsJIrKBMmONU4FPkR kKV99BGEryVI8e5QkUJLQ8oY73bh/EZkzXf99ovJ0D+6HMl9baLSxwKbRKNlDZcPP1+N4Z8m bUtv8sf6gqejBs2M4nZ0nAIpjrUdnFZCvcprJAXBoPvmzEH8FAabMyOEDLy7bGOd85IbhsgL AiLifeQnL9b3EfDLSY+TCCfwepHiJ0SkxlW11tedU+Rk9/Ii/Jrjh1c9TM7ElZcwhldir8hP 2FqMwt+JLmU/icuj89GBjj+FwZEDRyf20rw11pWyzGJExj2DjTAfD8nJOKA3EEF6GYNLDFU8 YaRxHvhTTu3Ltr62TE/WBI9pvHuJTCrGtYuRCxz8xy5IqQH
  • Ironport-hdrordr: A9a23:a/eKDaoaVSAvS3j1rwAWin8aV5u6L9V00zEX/kB9WHVpm5Oj+P xGzc526farslsssREb+OxpOMG7MBThHLpOkPMs1NCZLXTbUQqTXfpfBO7ZrQEIdBeOlNK1uZ 0QFpSWTeeAcWSS7vyKkTVQcexQueVvmZrA7Yy1rwYPPHFXguNbnn9E426gYzNLrWJ9dPwE/f Snl656T23KQwVpUi33PAhJY8Hz4/nw0L72ax8PABAqrCGIkDOT8bb/VzyVxA0XXT9jyaortT GtqX232oyT99WAjjPM3W7a6Jpb3PPn19t4HcSJzuwYMC/lhAqEbJloH5eCoDc2iuey70tCqq iBnz4Qe+BIr1/BdGC8phXgnyHmzTYV8nfnjWSVhHPyyPaJDA4SOo5kv8Z0YxHZ400vsJVXy6 RQxV+UsJJREFfpgDn9z8KgbWArqmOE5V4Z1cIDhX1WVoUTLJVLq5YEwU9TGJAcWArn9YEcFv V0Bs203ocWTbqjVQGagoBT+q3oYpxqdS32BnTq+/blnQS+pUoJjHfxn6ck7zA9HJFUcegM2w 2LCNUvqFniJvVmGZ6VP91xM/dfPFa9Ny4kAFjiUmgPK5t3Tk4li6SHq4ndt9vaMqDh8vMJ6e P8uRVjxDcPR34=
  • Ironport-sdr: NJwuhDS557qlyqg2P8PtVDxhoGClwSXjBZKY2figBweo6mYnOcGPDOWFAATIIe/z0jwlOU/JJh BDQkQLHYpXEI0tyIfvpwre0LxOLJrWfv6cQM9L4hx57mdDVgAC+Pci0RBZ8JbbgZZ99JpcfUe1 xdoJFaiSZcYjAmsCLaBDVlLKx3OUkhcM8o9yrMeLSVkUtAkJbCSTHgdnVhAFo623F9E6oOdtib w9xlV5ajJ8O2/c4BvD09+OZ9MXeydgN1k++GroK1DF73hGHJ/YWHG7fmVjVrVjd/Q9DOkrB5hs Ct5WMfd8BQGXwNdsp9fw7CJ3
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Wed, Oct 13, 2021 at 09:48:42AM +0000, Bertrand Marquis wrote:
> Hi Roger,
> 
> > On 13 Oct 2021, at 09:45, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> > 
> > On Wed, Oct 06, 2021 at 06:40:34PM +0100, 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.
> >> 
> >> For Dom0less systems scan_pci_devices() would be used to discover the
> >> PCI device in XEN and VPCI handler will be added during XEN boots.
> >> 
> >> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> >> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> >> ---
> >> Change in v5:
> >> - Add pci_cleanup_msi(pdev) in cleanup part.
> >> - Added Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> >> Change in v4:
> >> - Move addition of XEN_DOMCTL_CDF_vpci flag to separate patch
> >> Change in v3:
> >> - Use is_pci_passthrough_enabled() in place of pci_passthrough_enabled 
> >> variable
> >> - Reject XEN_DOMCTL_CDF_vpci for x86 in arch_sanitise_domain_config()
> >> - Remove IS_ENABLED(CONFIG_HAS_VPCI) from has_vpci()
> >> Change in v2:
> >> - Add new XEN_DOMCTL_CDF_vpci flag
> >> - modify has_vpci() to include XEN_DOMCTL_CDF_vpci
> >> - enable vpci support when pci-passthough option is enabled.
> >> ---
> >> ---
> >> xen/arch/arm/Makefile         |   1 +
> >> xen/arch/arm/domain.c         |   4 ++
> >> xen/arch/arm/domain_build.c   |   3 +
> >> xen/arch/arm/vpci.c           | 102 ++++++++++++++++++++++++++++++++++
> >> xen/arch/arm/vpci.h           |  36 ++++++++++++
> >> xen/drivers/passthrough/pci.c |  18 ++++++
> >> xen/include/asm-arm/domain.h  |   7 ++-
> >> xen/include/asm-x86/pci.h     |   2 -
> >> xen/include/public/arch-arm.h |   7 +++
> >> xen/include/xen/pci.h         |   2 +
> >> 10 files changed, 179 insertions(+), 3 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 44d7cc81fa..fb9c976ea2 100644
> >> --- a/xen/arch/arm/Makefile
> >> +++ b/xen/arch/arm/Makefile
> >> @@ -7,6 +7,7 @@ ifneq ($(CONFIG_NO_PLAT),y)
> >> obj-y += platforms/
> >> endif
> >> obj-$(CONFIG_TEE) += tee/
> >> +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 36138c1b2e..fbb52f78f1 100644
> >> --- a/xen/arch/arm/domain.c
> >> +++ b/xen/arch/arm/domain.c
> >> @@ -39,6 +39,7 @@
> >> #include <asm/vgic.h>
> >> #include <asm/vtimer.h>
> >> 
> >> +#include "vpci.h"
> >> #include "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/domain_build.c b/xen/arch/arm/domain_build.c
> >> index c5afbe2e05..f4c89bde8c 100644
> >> --- a/xen/arch/arm/domain_build.c
> >> +++ b/xen/arch/arm/domain_build.c
> >> @@ -3053,6 +3053,9 @@ void __init create_dom0(void)
> >>     if ( iommu_enabled )
> >>         dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> >> 
> >> +    if ( is_pci_passthrough_enabled() )
> >> +        dom0_cfg.flags |= XEN_DOMCTL_CDF_vpci;
> >> +
> >>     dom0 = domain_create(0, &dom0_cfg, true);
> >>     if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
> >>         panic("Error creating domain 0\n");
> >> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
> >> new file mode 100644
> >> index 0000000000..76c12b9281
> >> --- /dev/null
> >> +++ b/xen/arch/arm/vpci.c
> >> @@ -0,0 +1,102 @@
> >> +/*
> >> + * xen/arch/arm/vpci.c
> >> + *
> >> + * 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>
> >> +
> >> +#include <asm/mmio.h>
> >> +
> >> +#define REGISTER_OFFSET(addr)  ( (addr) & 0x00000fff)
> >> +
> >> +/* Do some sanity checks. */
> >> +static bool vpci_mmio_access_allowed(unsigned int reg, unsigned int len)
> >> +{
> >> +    /* Check access size. */
> >> +    if ( len > 8 )
> >> +        return false;
> >> +
> >> +    /* 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;
> >> +    unsigned long data = ~0UL;
> >> +    unsigned int size = 1U << info->dabt.size;
> >> +
> >> +    sbdf.sbdf = MMCFG_BDF(info->gpa);
> >> +    reg = REGISTER_OFFSET(info->gpa);
> >> +
> >> +    if ( !vpci_mmio_access_allowed(reg, size) )
> >> +        return 0;
> >> +
> >> +    data = vpci_read(sbdf, reg, min(4u, size));
> >> +    if ( size == 8 )
> >> +        data |= (uint64_t)vpci_read(sbdf, reg + 4, 4) << 32;
> >> +
> >> +    *r = data;
> >> +
> >> +    return 1;
> >> +}
> >> +
> >> +static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
> >> +                           register_t r, void *p)
> >> +{
> >> +    unsigned int reg;
> >> +    pci_sbdf_t sbdf;
> >> +    unsigned long data = r;
> >> +    unsigned int size = 1U << info->dabt.size;
> >> +
> >> +    sbdf.sbdf = MMCFG_BDF(info->gpa);
> >> +    reg = REGISTER_OFFSET(info->gpa);
> >> +
> >> +    if ( !vpci_mmio_access_allowed(reg, size) )
> >> +        return 0;
> >> +
> >> +    vpci_write(sbdf, reg, min(4u, size), data);
> >> +    if ( size == 8 )
> >> +        vpci_write(sbdf, reg + 4, 4, data >> 32);
> > 
> > I think those two helpers (and vpci_mmio_access_allowed) are very
> > similar to the existing x86 ones (see vpci_mmcfg_{read,write}), up to
> > the point where I would consider moving the shared code to vpci.c as
> > vpci_ecam_{read,write} and call them from the arch specific trap
> > handlers.
> 
> Would it be ok to have ecam specific code in the vpci common code ?

I think so, ECAM is part of the PCI specification and architecture
agnostic, so fine to place in common code.

> This is an optimisation and we could do that later on as this is an other
> change to be done and tested which will start to make things very
> challenging with the friday deadline.

I guess this could be done, by adding a TODO note that the handlers
should be unified with the x86 ones and placed in common code.

I however get the feeling that such work would ultimately be ignored,
as there's always more pressing stuff to be done, and people tend to
forget about those cleanups once things get committed.

Thanks, Roger.



 


Rackspace

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