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

Re: [PATCH v5 06/14] vpci/header: implement guest BAR register handlers


  • To: Oleksandr Andrushchenko <andr2000@xxxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Wed, 12 Jan 2022 18:34:29 +0100
  • 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=qGA+8NuGfkZOot8O5jPiHhr4FywJLuwxEOFpXbc0eqs=; b=FAuha1PBmPQyO5dRCEw6zc4VlvGzPYil/myjXH1GtrRuwO9uHb1zAlRtiCTkohJGYEyXoPWkGe/AK+kmvkGEcne/0iZGFQ7fWz5JrRn8albgG/IklGdrl/E882TjGhMEme9B+8yOKOA215wevPJkaaiVtZ5tIK3ixqO2nuYRBK9fVjrDdLyra2kpGw1JkeZxKhJSNL9tfkoDmg6qRTUjO8/Q01oD3M9zJU6LsVqcJR//Z7jkZ/L8ESwRk8uIzzEwnVnajxkUyAHA00oPNSQC16PRpnzxME9xvBy473I7NZ4tjUkpAJK6Tea2KpwojH9I5qS6/hPnmwBXN0xhlaf+0A==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=R3SbyU4YsjGITIlPXWWs497SAGimwks+j2k1ITTwMLHo8KynKnpxD10sH5LQkB/4sLYuz8A5Ba4vlnynoejZIoisLMRCiLbbV5I0jVQJT6lEJIxGqaO6s7sDMhDMwXVfE1oE5p+eS6g2PH6DyoxeswgTup9ZERZAkCZglvKZp30zDaipR/t4Df8AOSJox3g/jnnn/yD2y4NsnYhoA1M5dSHxxd/UkDSFaHggohq5dR8aEMoE21ULD44q3H7o09PAcyve5VdMKfsHXim0amYZWa1MlD7lpStQ2J3XWGaPVqTzB9zV2NXyf69/gpjCfXgHHkf+uE7nfT1NCiNxqAIj+g==
  • Authentication-results: esa1.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: <xen-devel@xxxxxxxxxxxxxxxxxxxx>, <julien@xxxxxxx>, <sstabellini@xxxxxxxxxx>, <oleksandr_tyshchenko@xxxxxxxx>, <volodymyr_babchuk@xxxxxxxx>, <Artem_Mygaiev@xxxxxxxx>, <jbeulich@xxxxxxxx>, <andrew.cooper3@xxxxxxxxxx>, <george.dunlap@xxxxxxxxxx>, <paul@xxxxxxx>, <bertrand.marquis@xxxxxxx>, <rahul.singh@xxxxxxx>, Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
  • Delivery-date: Wed, 12 Jan 2022 17:35:00 +0000
  • Ironport-data: A9a23:PWrf/qmhDztcN3DUBkuoL+To5gxBIERdPkR7XQ2eYbSJt1+Wr1Gzt xIfDGuCM63ZYTH8eY10bI209RlU6JTdx4BiQVM++Sw2ESMWpZLJC+rCIxarNUt+DCFioGGLT Sk6QoOdRCzhZiaE/n9BClVlxJVF/fngqoDUUYYoAQgsA180IMsdoUg7wbRh29cy2YLR7z6l4 rseneWOYDdJ5BYsWo4kw/rrRMRH5amaVJsw5zTSVNgT1LPsvyB94KE3fMldG0DQUIhMdtNWc s6YpF2PEsE1yD92Yj+tuu6TnkTn2dc+NyDW4pZdc/DKbhSvOkXee0v0XRYRQR4/ttmHozx+4 N9UksCVWSczBJPrnsZCdBNqFHFlJZQTrdcrIVDn2SCS50jPcn+qyPRyFkAme4Yf/46bA0kXq 6ZecmpUKEne2aTmm9pXScE17ignBNPsM44F/Glp0BnSDOo8QICFSKLPjTNd9Glq3J0RQa2DD yYfQTZORwvifxptAHceD5QRoN2OmCXuWSIN/Tp5ooJoujOOnWSdyoPFKNPIfvSaSMMTmVyXz krE9WnkBhARNPSE1CGItHmrg4fntDnnVYclMay3//9nnnWe3mUWThYRUDOTpv20iVW3Xd5FH EUS9jAztqg59EGtTd7VUgWxpTiPuRt0c8BZE/A+rhqMzKXUyw+DAy4PSTspQNYrrtMsTDomk FqAhcr0BCdHuaeQD3ma89+8ry62OCUTBX8PY2kDVwRty8nupsQ/gwzCSv5nEbWplZvlFDfo2 TeIoSMiwbIJgqYj3qO35wqf22qEqZ3ATwpz7QLSNkq76Qd+aJ+gdpaf41HR5vZdL66UVlCE+ nMDnqC26+QDDoqEkiCXd/kcB7Gi5/uDMzr0jEZmGt8q8DHF02W4YYla7TV6JUFoGsUJYznkZ AnUoww52XNIFCL0N+ktOdv3Upl0i/i7fTj4ahzKRtBXRcRUZC+dxh9JY2zP+Grflxg3iZhqb P93bv2QJXodDK1myh+/SOEczaIny0gC+I/DeXzo50/5iOTDPRZ5XZ9AaQLTNb5hsMtotS2Iq 443Ciec9/lIvAQSiAHz+JVbE10FJGNT6Xve+50OLb7rzuaL9Qgc5x7tLVEJJ90Nc0d9zL6gE pSBtqlwkguXaZrvc1TiV5ybQOmzNauTVFpiVcDWAX6m2mI4faGk57oFep08cNEPrbI/lK8uE adVI5rbU5yjrwgrHRxHPPERS6Q4JXyWaf+mZXL5MFDTgbY9L+A2xjMUVlS2r3RfZsZGncA/v 6ehxmvmrWkrHGxf4DLtQKv3lTuZ5CFF8MorBhegCoQNJC3ErdY7QwSs3q5fC5xdcn3rm2rFv zt69D9F/4EhVadvromQ7U1Fxq/0e9ZD8r1yRDiEve3oZHiDrgJOA+ZoCY61QNwUb0utkI2Kb uRJ1fDsdvoBmVdBqY1nFLh3i6k54rPSS3Vyl2yIxV3HMAamDK1OOH6D0ZUdv6FB3OYB6wC3R liO6p9RPrDQYJHpF1sYJQwEaOWf1K5LxmmOvKpteEiqtjVq+LenUFlJO0XegiJqM7YoYpgux v0suZBK5lXn2AYqKNuPkgtd63+Ici4bS6wiu5xDWN3rhwMnx0tse5vZDiOqspiDZ88VahshI yOOhbqEjLNZnxKQf302HHnL/OxcmZVR50wakA5cfwyEw4OXiOU20Rtd9SUMYj5UlhgXgfhuP mVLNlFuIfnc9Tlfm8UeDXunHBtMBUPF9xWpmUcJjmDQU2KhSnfJcD8mIe+I8U0UrzBcczxc8 O3KwWrpS2+3LsT43y90Uk95sf3zC9d281SaysygGs2EGbg8YCbk3fDyNTZZ9UO/DJNjnlDDq Mlr4P10OP/yOiMnqqEmD5WXiOYLQxeeKW0eGfxs8cvlx40HlO1eDdRWF32MRw==
  • Ironport-hdrordr: A9a23:/bvhcqq56OHOVXI8jNSOpEoaV5vPL9V00zEX/kB9WHVpm5Oj+f xGzc516farslossREb+expOMG7MBXhHLpOkPQs1NaZLXPbUQ6TTb2KgrGSpgEIdxeOktK1kJ 0QD5SWa+eAfGSS7/yKmDVQeuxIqLLsndHK9IXjJjVWPHpXgslbnnlE422gYzRLrWd9dP0E/M 323Ls5m9PsQwVdUu2LQl0+G8TTrdzCk5zrJTYAGh4c8QGLyRel8qTzHRS01goXF2on+8ZuzU H11yjCoomzufCyzRHRk0fV8pRtgdPkjv9OHtaFhMQ5IijlziyoeINicbufuy1dmpDk1H8a1P 335zswNcV67H3cOkmzvBvWwgHllA0j7nfzoGXo90fLkIjcfnYXGsBBjYVWfl/y8Ew7puxx16 pNwiawq4dXJQmoplWy2/H4EzVR0makq3srluAey1ZFV5EFVbNXpYsDuGtIDZY7Gj7g4oxPKp ggMCjl3ocXTbqmVQGbgoE2q+bcHEjbXy32DnTqg/blkgS/xxtCvg4lLM92pAZ2yHtycegB2w 3+CNUaqFh5dL5jUUtMPpZwfSKJMB2+ffvtChPaHb21LtBOB5ryw6SHlYndotvaP6A18A==
  • Ironport-sdr: NrW8EMrL4pfTXGpnoZTTjEuVFcW2l9i0cmgpf209t9Lq6Nc6y/PCWgUyUrK7GmIN+mqMsyLfs/ WYEfwPmuTxPSuslojIVXX8pBKQpJ8+M56QjEweDKqggAMSeDUy8hvi6QLekQBl4RYXacSnvUbm paUI+osuFBQxiru1Rg/a62wP9kFZhbwxBH+C1ozOeXki0E5lXm39rJ9lNpDQoay7V9ADNvMQH8 zBbupQ6gjjmpMJlcoSycop2aTjZKpHDWBTaO9ddiW8nBjz2VRhUfFUMv8TzDeHkvUw6fJYmLCW e56NCxOMjzRMT5afXsI41FCL
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

A couple more comments I realized while walking the dog.

On Thu, Nov 25, 2021 at 01:02:43PM +0200, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> 
> Add relevant vpci register handlers when assigning PCI device to a domain
> and remove those when de-assigning. This allows having different
> handlers for different domains, e.g. hwdom and other guests.
> 
> Emulate guest BAR register values: this allows creating a guest view
> of the registers and emulates size and properties probe as it is done
> during PCI device enumeration by the guest.
> 
> ROM BAR is only handled for the hardware domain and for guest domains
> there is a stub: at the moment PCI expansion ROM handling is supported
> for x86 only and it might not be used by other architectures without
> emulating x86. Other use-cases may include using that expansion ROM before
> Xen boots, hence no emulation is needed in Xen itself. Or when a guest
> wants to use the ROM code which seems to be rare.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> ---
> Since v4:
> - updated commit message
> - s/guest_addr/guest_reg
> Since v3:
> - squashed two patches: dynamic add/remove handlers and guest BAR
>   handler implementation
> - fix guest BAR read of the high part of a 64bit BAR (Roger)
> - add error handling to vpci_assign_device
> - s/dom%pd/%pd
> - blank line before return
> Since v2:
> - remove unneeded ifdefs for CONFIG_HAS_VPCI_GUEST_SUPPORT as more code
>   has been eliminated from being built on x86
> Since v1:
>  - constify struct pci_dev where possible
>  - do not open code is_system_domain()
>  - simplify some code3. simplify
>  - use gdprintk + error code instead of gprintk
>  - gate vpci_bar_{add|remove}_handlers with CONFIG_HAS_VPCI_GUEST_SUPPORT,
>    so these do not get compiled for x86
>  - removed unneeded is_system_domain check
>  - re-work guest read/write to be much simpler and do more work on write
>    than read which is expected to be called more frequently
>  - removed one too obvious comment
> ---
>  xen/drivers/vpci/header.c | 72 +++++++++++++++++++++++++++++++++++----
>  xen/include/xen/vpci.h    |  3 ++
>  2 files changed, 69 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index ba333fb2f9b0..8880d34ebf8e 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -433,6 +433,48 @@ static void bar_write(const struct pci_dev *pdev, 
> unsigned int reg,
>      pci_conf_write32(pdev->sbdf, reg, val);
>  }
>  
> +static void guest_bar_write(const struct pci_dev *pdev, unsigned int reg,
> +                            uint32_t val, void *data)
> +{
> +    struct vpci_bar *bar = data;
> +    bool hi = false;
> +
> +    if ( bar->type == VPCI_BAR_MEM64_HI )
> +    {
> +        ASSERT(reg > PCI_BASE_ADDRESS_0);
> +        bar--;
> +        hi = true;
> +    }
> +    else
> +    {
> +        val &= PCI_BASE_ADDRESS_MEM_MASK;
> +        val |= bar->type == VPCI_BAR_MEM32 ? PCI_BASE_ADDRESS_MEM_TYPE_32
> +                                           : PCI_BASE_ADDRESS_MEM_TYPE_64;
> +        val |= bar->prefetchable ? PCI_BASE_ADDRESS_MEM_PREFETCH : 0;
> +    }
> +
> +    bar->guest_reg &= ~(0xffffffffull << (hi ? 32 : 0));
> +    bar->guest_reg |= (uint64_t)val << (hi ? 32 : 0);
> +
> +    bar->guest_reg &= ~(bar->size - 1) | ~PCI_BASE_ADDRESS_MEM_MASK;

You need to assert that the guest set address has the same page offset
as the physical address on the host, or otherwise things won't work as
expected. Ie: guest_addr & ~PAGE_MASK == addr & ~PAGE_MASK.

> +}
> +
> +static uint32_t guest_bar_read(const struct pci_dev *pdev, unsigned int reg,
> +                               void *data)
> +{
> +    const struct vpci_bar *bar = data;
> +    bool hi = false;
> +
> +    if ( bar->type == VPCI_BAR_MEM64_HI )
> +    {
> +        ASSERT(reg > PCI_BASE_ADDRESS_0);
> +        bar--;
> +        hi = true;
> +    }
> +
> +    return bar->guest_reg >> (hi ? 32 : 0);
> +}
> +
>  static void rom_write(const struct pci_dev *pdev, unsigned int reg,
>                        uint32_t val, void *data)
>  {
> @@ -481,6 +523,17 @@ static void rom_write(const struct pci_dev *pdev, 
> unsigned int reg,
>          rom->addr = val & PCI_ROM_ADDRESS_MASK;
>  }
>  
> +static void guest_rom_write(const struct pci_dev *pdev, unsigned int reg,
> +                            uint32_t val, void *data)
> +{
> +}
> +
> +static uint32_t guest_rom_read(const struct pci_dev *pdev, unsigned int reg,
> +                               void *data)
> +{
> +    return 0xffffffff;
> +}
> +
>  static int init_bars(struct pci_dev *pdev)
>  {
>      uint16_t cmd;
> @@ -489,6 +542,7 @@ static int init_bars(struct pci_dev *pdev)
>      struct vpci_header *header = &pdev->vpci->header;
>      struct vpci_bar *bars = header->bars;
>      int rc;
> +    bool is_hwdom = is_hardware_domain(pdev->domain);
>  
>      switch ( pci_conf_read8(pdev->sbdf, PCI_HEADER_TYPE) & 0x7f )
>      {
> @@ -528,8 +582,10 @@ static int init_bars(struct pci_dev *pdev)
>          if ( i && bars[i - 1].type == VPCI_BAR_MEM64_LO )
>          {
>              bars[i].type = VPCI_BAR_MEM64_HI;
> -            rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, 
> reg,
> -                                   4, &bars[i]);
> +            rc = vpci_add_register(pdev->vpci,
> +                                   is_hwdom ? vpci_hw_read32 : 
> guest_bar_read,
> +                                   is_hwdom ? bar_write : guest_bar_write,
> +                                   reg, 4, &bars[i]);
>              if ( rc )
>              {
>                  pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
> @@ -569,8 +625,10 @@ static int init_bars(struct pci_dev *pdev)
>          bars[i].size = size;
>          bars[i].prefetchable = val & PCI_BASE_ADDRESS_MEM_PREFETCH;
>  
> -        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, reg, 4,
> -                               &bars[i]);
> +        rc = vpci_add_register(pdev->vpci,
> +                               is_hwdom ? vpci_hw_read32 : guest_bar_read,
> +                               is_hwdom ? bar_write : guest_bar_write,
> +                               reg, 4, &bars[i]);

You need to initialize guest_reg to the physical host value also.

>          if ( rc )
>          {
>              pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
> @@ -590,8 +648,10 @@ static int init_bars(struct pci_dev *pdev)
>          header->rom_enabled = pci_conf_read32(pdev->sbdf, rom_reg) &
>                                PCI_ROM_ADDRESS_ENABLE;
>  
> -        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write, 
> rom_reg,
> -                               4, rom);
> +        rc = vpci_add_register(pdev->vpci,
> +                               is_hwdom ? vpci_hw_read32 : guest_rom_read,
> +                               is_hwdom ? rom_write : guest_rom_write,
> +                               rom_reg, 4, rom);
>          if ( rc )
>              rom->type = VPCI_BAR_EMPTY;

Also memory decoding needs to be initially disabled when used by
guests, in order to prevent the BAR being placed on top of a RAM
region. The guest physmap will be different from the host one, so it's
possible for BARs to end up placed on top of RAM regions initially
until the firmware or OS places them at a suitable address.

Thanks, Roger.



 


Rackspace

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