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

Re: [PATCH v5 08/10] xen/arm: Setup MMIO range trap handlers for hardware domain


  • To: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Mon, 25 Oct 2021 15:40:36 +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=PiIxI6WkK73yAaGqnLv3DJVEO2USAk8Ei936fAO1J94=; b=gLkcNHXrth72R5NZY9YuuJBVGbOmWPAp2QIHGKlIzgJse+Y/nJ3t90ex+PduiMSiJArZifu+8iVzBwCfULUcck5E2TN4XcuAMcTZojHF0U4ieBqj5weghSpwoEOH1j2iiqZNYHKBJfmooJXV56msagftOo7wxEP87q5yTadeqXqbfvOc8ELcDfFnxl9gJ292E2wlztXvWaCBKeSbvvPh5Xq+vRI8H69AgkGz5QOJq1orJYvV1qS5Y59eZPi+1s4LQ/BXmIQU43Z/EWq9B42ss/zElZsavLrPHFqpexoItcXIX4rdmdLivlqKT7vSozMc7mOiCqdBSfPUtqMISUA9MQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=HwOzM2AVH6BQVkRLbDkSAyPtmYE6ElYTYn80mVnO0F4cilxicuKUqPnTnyXf+lQ7tZkRCHjSxWwwjG3DA3JcrOJ28WpVbNbwUhTm6v4QVOtB2fr5ebkZK/dlXpZc2YsoqfdjMOyqpv3sytjAzzB8+ZkKOQBlpHmQMvMpolfVGCO8i2aoOS16XxMuJb47GFjna7cmHTbHr1VBcaN7yCGPpxbWAKtlzgW0Mmh/Le7W53Y0nbqLeqaVqiQsgklYpTHpJ5B64sjP8K2QTMDsRKck+6b2oWIgtxcUw0x+IFV2HiOaiAtcm2SyIXJK5dO9GNyxY4dV22lzla1RSTJDm4Dkag==
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "julien@xxxxxxx" <julien@xxxxxxx>, "sstabellini@xxxxxxxxxx" <sstabellini@xxxxxxxxxx>, Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Artem Mygaiev <Artem_Mygaiev@xxxxxxxx>, "jbeulich@xxxxxxxx" <jbeulich@xxxxxxxx>, "andrew.cooper3@xxxxxxxxxx" <andrew.cooper3@xxxxxxxxxx>, "george.dunlap@xxxxxxxxxx" <george.dunlap@xxxxxxxxxx>, "paul@xxxxxxx" <paul@xxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Rahul Singh <rahul.singh@xxxxxxx>
  • Delivery-date: Mon, 25 Oct 2021 13:40:52 +0000
  • Ironport-data: A9a23:WcB8ma9pJXccK9c145EhDrUDoniTJUtcMsCJ2f8bNWPcYEJGY0x3z mQWWz+BbP2Ca2D8fdEkadnnoRhV7JbXy9JmGgFpqCo8E34SpcT7XtnIdU2Y0wF+jyHgoOCLy +1EN7Es+ehtFie0Si9AttENlFEkvU2ybuOU5NXsZ2YhGmeIdA970Ug6wrZj39Yx6TSEK1jlV e3a8pW31GCNg1aYAkpMg05UgEoy1BhakGpwUm0WPZinjneH/5UmJMt3yZWKB2n5WuFp8tuSH I4v+l0bElTxpH/BAvv9+lryn9ZjrrT6ZWBigVIOM0Sub4QrSoXfHc/XOdJFAXq7hQllkPhg9 PtivpOXSzwDGerVnr0wWjlHFCdHaPguFL/veRBTsOSWxkzCNXDt3+9vHAc9OohwFuRfWD8Us 6ZCcXZUM07F17neLLGTE4GAguw5K8bmJsUHs2xIxjDFF/c2B5vERs0m4PcFgWps35gXR54yY eIzdBZxNT7STSRREQgWA40UktqQvHrgJmgwRFW9+vNsvjm7IBZK+LrnPcfRe9eKbd5IhUver WXDl0zQGA0XMeu62DWM83+yruLXlCa9U4UXfJWg/+NuqE2ewCoUEhJ+fUGyoeS9zFW/Xd1fA 0UO/2wlqq1a3EamVMXnVhu05nuNpAcBWsF4Gvc/rgqKz8L83QGdAWQVSy9bX/YvvsQ2WD8C2 0eAmpXiAjkHmK2YTzeR+6mZqRu2ODMJNikSaCkcVwwH7tL/5oYpgXrnadJuE7W8iNHvLhj2z yqXtyg1h7gVjskj2r2y+BbMhDfEjprUSg844C3HU2Tj6Rl2DKaCY4Gr8lHd4ex3EJeCTlKBs X4HnOCT9OkLS5qKkUSlW/4RFbuk4/KENjz0glN1GZQlsTO39BaekZt4uW8kYh0za4BdJGGvM BS7VR5tCIF7LUeEQqR4RICKIeNt1K65ON3FSffSV48bCnRuTzOv8CZrbE+W+mnilkkwjK0yU aumndaQ4WUyUvs/kmLnLwsJ+fpynHpmnDKMLXzu503/ieL2WZKDdVsS3LJihMgC56SYvB6dz d9bM8abo/m0eLyjOneJmWL/wFZjEJTaOXwUg5EPHgJgClA/cI3ENxM26eh5E7GJZ4wPyo/1E oiVAye0MmbXi3zdMhmtYXt+cr7pVpsXhStlZnF1ZQ7yiiB7O9bHAEIjm30fJ+lPGAtLlqYcc hX4U5/YXqQnpsrvomx1gWbBQHxKK03w2FPm09uNazkjZZ9wLzElCfe/FjYDABImV3Lt3eNn+ uXI/lqCHfIrGlQzZO6LOanH5w7g4hAgdBdaAhKgzi97Ix63ruCH6kXZ05cKHi37AUySlmXBj 13NX0ZwSCuki9ZdzeQlTJus9u+BO+B/AlBbDy/c67O3PjPd5W2t3clLV+PgQNwXfDqcFHyKa boHwvfiHucAmVoW4YNwH6wylfA15sf1pq8cxQNhRS2ZY1OuA7JmA3+HwcgQ6fEdmu4H4VO7C hCV591XGbSVI8e5QlQfExUoM7aY3vYOlziMsflseBfm5DV69aasWFlJO0XekzRUKbZ4adt3w eootMMMxRa4jx4mboSPgixOrjzeJX0cSaQ38JodBdaz2AYsz1hDZ73aCzP3v87TO4kdbBFyL 2bN1qTYhrlayk7TSFYJFCDAjbhHmJADmBFW11tedV6HrcXI260s1xpL/DVpEgkMlkdb0/h+M 3RAPlFuIfnc5C9hgcVOUjz+GwxFAxHFqEX9x0FQyT/cRkisEGfMMHc8KaCG+0VAqzBQeT1S/ be5zmf5UGm1IJGtj3VqAUM1+eb+SdFR9xHZnJH1FsuIKJA2fD75j/L8fmEPsRbmXZs8iUCvS TOGJwqshXkX7RItnpA=
  • Ironport-hdrordr: A9a23:afKOCqN2jRVUGcBcTsWjsMiBIKoaSvp037BN7TEXdfU1SL39qy nKpp8mPHDP5Ar5NEtOpTniAsm9qBHnm6KdiLN5Vd3OYOCMggqVBbAnwYz+wyDxXw3Sn9QtsJ uIqpIOa+EY22IK7/rH3A==
  • Ironport-sdr: YvovpOaf3zz1cZhm4TXZIb5FYcA1mmiqNsNxa10/QuVX6wrIG1zWwojKYEHpbp6z7UGWXfOUj4 Xp3isYRXrmj9wgFM1hmO7wARjP1MNZt1fQpa1jcaaFhkYrmPTtdnG8PPwU86D1Ckm+bWhkAgvr 99JwdQwrGRoHxe9RcvbfCPMa1aGn2VwhrbIfOccb6eQp2rpPqo5ULIJ4dAXK11H7fTqVFI+3c9 v47vTyQIpm2YC1K7Ls/BMzbTlIRjRfJ9yThAgMI/2mcZUoOd7Raf42beAFBal0JiQFut7DDDlx n1CmgSpacL+yZ7mUzGaCiF+n
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Mon, Oct 25, 2021 at 09:38:00AM +0000, Oleksandr Andrushchenko wrote:
> Hi, Roger!
> 
> On 13.10.21 13:11, Roger Pau Monné wrote:
> > On Fri, Oct 08, 2021 at 08:55:33AM +0300, Oleksandr Andrushchenko wrote:
> >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> >>
> >> 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@xxxxxxxx>
> >> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> >> Reviewed-by: Rahul Singh <rahul.singh@xxxxxxx>
> >> Tested-by: Rahul Singh <rahul.singh@xxxxxxx>
> >> ---
> >> 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 | 28 ++++++++++++++++++++++++
> >>   xen/arch/arm/vpci.c                | 34 ++++++++++++++++++++++++++++++
> >>   xen/arch/arm/vpci.h                |  6 ++++++
> >>   xen/include/asm-arm/pci.h          |  5 +++++
> >>   5 files changed, 75 insertions(+)
> >>
> >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> >> index 79012bf77757..fa6fcc5e467c 100644
> >> --- a/xen/arch/arm/domain.c
> >> +++ b/xen/arch/arm/domain.c
> >> @@ -733,6 +733,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 )
> > IMO it might be better to convert the fixed array into a linked list,
> > I guess this made sense when Arm had a very limited number of mmio
> > trap handlers, but having to do all this accounting seems quite
> > tedious every time you want to add new handlers.
> Yes, I think we need to do so, but this improvement was not meant
> to be in this patch.

Ack, just wanted to raise that this model seems to be getting more
complex than just setting up a list.

> >> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
> >> index 76c12b92814f..6e179cd3010b 100644
> >> --- a/xen/arch/arm/vpci.c
> >> +++ b/xen/arch/arm/vpci.c
> >> @@ -80,17 +80,51 @@ static const struct mmio_handler_ops vpci_mmio_handler 
> >> = {
> >>       .write = vpci_mmio_write,
> >>   };
> >>   
> >> +static int vpci_setup_mmio_handler(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, NULL);
> > I'm confused here, don't you need to use a slightly different handler
> > for dom0 so that you can differentiate between the segments of the
> > host bridges?
> >
> > AFAICT the translation done by vpci_mmio_handler using MMCFG_BDF
> > always assume segment 0.
> You are absolutely right here: I can set up hwdom specific
> handlers, so I can properly translate the segment.
> On the other hand, when virtual bus topology added, the SBDF
> translation from virtual to physical SBDF resides in the Arm's
> vpci_mmio_{read|write}, like the below:
>      if ( priv->is_virt_ecam &&
>           !vpci_translate_virtual_device(v->domain, &sbdf) )
>              return 1;
> (BTW Jan asked in some other comment why it is Arm specific:
> I tend to keep it Arm specific until the point when x86 wants that
> as well. Until that point the code, if moved to common, will be
> unneeded and as Jan calls that "dead")
> So, I think that I can extend vpci_mmio_{read|write} to account
> on the hwdom like (virtual bus code is the future code):
> 
> static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>                            register_t *r, void *p)
> {
> ...
>      struct vpci_mmio_priv *priv = (struct vpci_mmio_priv *)p;
> 
>      if ( priv->is_virt_ecam )
>          sbdf.sbdf = MMCFG_BDF(info->gpa); /* For virtual bus topology the 
> segment is always 0. */
>      else
>      {
>          sbdf.sbdf = MMCFG_BDF(info->gpa);
>          sbdf.seg = priv->segment;
>      }
>      reg = REGISTER_OFFSET(info->gpa);
> 
> ...
>      /*
>       * For the passed through devices we need to map their virtual SBDF
>       * to the physical PCI device being passed through.
>       */
>      if ( priv->is_virt_ecam &&
>           !vpci_translate_virtual_device(v->domain, &sbdf) )
>              return 1;
> 
> Will it work for you?

Right, I guess it could work as long as the differences between the
hardware domain and the unprivileged ones are not too big.

The nice part about having different handlers is that you avoid a
bunch of conditionals (ie: no need to check for is_virt_ecam) which
makes the code easier to follow. OTOH it could introduce more code
duplication.

Thanks, Roger.



 


Rackspace

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