[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 13:35:07 +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=/l0qq5D6M3CGfGZF/3xXMitwhOP01cIvv/ii/v/70fs=; b=CEDC4FF81BW6W9oYQ6/VWAISVMpS0wQKyx4dosVn85Nf3x8nFjWmXhIMQb+GDBSSBQhvTNoSLfnOSnkrmEgVlNXxvGShPRSDWbfWXzXzyakuX3nradjLBVLCvokqwmr7OE1Pszx5WVMbXXTqX6nvADbQH+lBdJRImpy6+33Ippqc+8l/jEqQuHjis9UzVAnZC1hl5IbgZp4Buv/E7wAhhA2HyiZUekb160YO8kZUoAkx8B0303NY9bJA7d/yJPfiobcTNscDLfgEE6Dijb8bB3VMWFuXXPReTcJ/XItbQxhoVHlyqZ4bhNfJ61q6ShNMDb9Uobx2z2HMEuDwTyaQLQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=UvzNR6MeB/eRU/ETRTtKEVvnbSTF3Ci56kunfiS2JweTub8NuqLYPeVTUaAOvwgqGAADFei8dCItqK1NX1PQKlMNuBRPf81RM/grQkVma2eHRbwgAUCYl7Fc6QWVlYd4TIwp7mkZv6DC3i+eG3499kXUD8hflLJo+ZKROssabYnptZj1tt3YfwN5AI3GiCRhMPIVTF8C1zebJ9PPRcF6fFCN0gIq2y+e62zNULudozv7cZI6IUZPYZsEbTeggP7VOVORbOJhshqvMaEx1sYb6IbuXCn9WatI250bfjG3cCmz5TXaFwLP+C1sTtZ08tOh1oNLx6dD2lnwTZNj/zFIJg==
  • Authentication-results: esa5.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 12:35:47 +0000
  • Ironport-data: A9a23:NfLYd6gGaWvtJvn2MCzvlDp4X161qxYKZh0ujC45NGQN5FlHY01je htvX22COvvcMzaneI1wboXk80sDv5bXmtM1TVc9pCkxFn8b9cadCdqndUqhZCn6wu8v7a5EA 2fyTvGacajYm1eF/k/F3oAMKRCQ7InQLlbGILes1htZGEk0GE/NtTo5w7Rj2tcx24Dja++wk YiaT/P3aQfNNwFcagr424rbwP+4lK2v0N+wlgVWicFj5DcypVFMZH4sDfjZw0/DaptVBoaHq 9Prl9lVyI97EyAFUbtJmp6jGqEDryW70QKm0hK6UID66vROS7BbPg/W+5PwZG8O4whlkeydx /0dv6CdTyYMIpaVwvk3TEcbCxBkA4RZreqvzXiX6aR/zmXDenrohf5vEFs3LcsT/eMf7WNmr KJCbmpXN1ba2rzwkOnTpupE36zPKOHxO4wSoDd4xCzxBvc6W5HTBa7N4Le02R9u35sWTa+OP aL1bxJIMz7ZQF59E24WK4wPouKnvln1eSZh/Qf9Sa0fvDGIkV0ZPKLWGMLcZ9iiVchT2EGCq Qru/W70HxUbP9y30iee/zSngeqntQrhRI8XI5ip+fdrjUO7y3QaDVsdUl7Tif69h02lUtRTM Xsd/CY0sLMy/0ymSNr6dxCgqXvCtRkZM/JKGu0n7EeWy6zb4y6QHG1CRTlEAPQsudUqXzUs2 hmMlsnwGD10mLSPTDSW8bL8hTGvPSkYK0cSaClCShEKi/HzrYd2gh/RQ9JLFK+uksazCTz22 yqNriU1m/MUl8Fj/6y98Uqd22r0jpfMRw8xoA7QWwqN8AR9Y4K0Yp2y3lLS5/1AMYWxQ0GIu T4PnM32xOcKAJKWnSqBWtIRDaqp7PaINj7bqVN3Fpxn/DOok1a4ZpxZ6jx6IEZvM+4HdCXvb UuVvhlejKK/J1PzM/UxOdjoTZ13k+6wTrwJS8w4cPIeaKRcci6i4Bo3P1O6wD6uiHk2r4ggb MLzndmXMV4WDqFuzTyTTugb0KM2yi1W+V4/VawX3Dz8j+PAOSf9paMtdQLXM7tntP/sTBD9r o4HX/Zm3SmzRwEXjsP/1YcIZW4HInEgbXwdg5wGL7XTSuaK9YxINhMw/V/DU9A090i2vr2Rl p1YZqO+4ACu7ZEgAV/bAk2PkJu1Af5CQYgHFSItJ020/HMofJyi6qwSH7NuI+V+pbY9k64sE KJUEyllPhipYm6Wk9j6RcOsxLGOiTzx3V7eV8ZbSGVXk2Fcq/zhpYa/I1qHGNgmBSurr8ouy 4BMJSuAKafvsz9KVZ6MANr2lgvZlSFExIpaAhWUSvEOJhSE2NU6ekTZ065sS+lReEqr+9dv/ 1vMaT8Cu/L3qpM4mPGQw/jsQ3GBSbUuRyK33gDzsN6LCMUt1jHynt8bDrfZJGC1uaGd0PzKW Ni5BsrUaZUvtF1Lr5B9A/Bsy6c/7MHovLhU0kJvG3CjUrhhIuoIzqCu0ZYdu6tT6KVevAfqC EuD9sMDYeeCOd//EU5XLw0gN7zR2fYRkzjUzPI0PESlu3MnoOvZCR1fb0uWlShQDLppK4d5k +0vj9Ebtl6kgR0wP9fY0i0NrzaQLmYNWrkMv40BBNO5kRIiz1xPOMSOCiL/7JyVRc9LN00mf m2diKbY3uwOzUveaXsjU3PK2LMF15gJvRlLyn4EJkiIxYWZ1qNmgkUJ/G1uHApPzxhB3+ZiA URRNhV4dfeU4jNlpMlfRGTwSQtPMwKUpx7qwFwTmWyHE0TxDj7RLHcwMPqm9VwC9z4OZSBS+ byVxTq3UTvue82tjCI+VVQ89q7mRN11sAbDhNqmD4KOGJxjOWjphaqnZGwprRr7AJxu2B2b9 LcypOsgO7fmMSMworEgD9jI3LsdfxmIOWheTKwz56gOB2zdJGm/1DXmx5pdoS+RyygmKXOFN vE=
  • Ironport-hdrordr: A9a23:61Ev0a5p3TRvv1jaTgPXwSyBI+orL9Y04lQ7vn2ZFiY6TiXIra +TdaoguSMc6AxwZJkh8erwXpVoZUmsiKKdgLNhR4tKOTOGhILGFvAG0WKP+UyFJ8S6zJ8g6U 4CSdkONDSTNykDsS+S2mDReLxMsbr3kpxAx92utEuFJTsaFZ2IhD0JczpzfHcGIzWvUvECZe WhD4d81nGdUEVSSv7+KmgOXuDFqdGOvJX6YSQeDxpizAWVlzun5JPzDhDdh34lIn5y6IZn1V KAvx3y562lvf3+4hjA11XL55ATvNf60NNMCOGFl8BQADTxjQSDYphnRtS5zXoIidDqzGxvvM jHoh8mMcg2w3TNflutqR+o4AXk2CZG0Q6V9XaoxV/Y5eDpTjMzDMRMwahDdAHC1kYmtNZglI pWwmOwrfNsfF39tRW4w+KNewBhl0Kyr3Znu/UUlWZjXYwXb6IUhZAD/XlSDIwLEEvBmc8a+d FVfYHhDcttABCnhyizhBgs/DXsZAV+Iv6+eDlChiTPuAIm2UyQzCMjtbsidzk7hdYAoqJ/lp f525JT5cVzp/8tHNJA7dg6MLmK40z2MFvx2TGpUBza/J9uAQO5l3ew2sRz2N2X
  • Ironport-sdr: /1JR4a2TC0pDSYZaSVn2+Mgmpfs3yG0go8p1H4QU6LBKhnNDIHOFuYwp6+l6Q2NXoxa7PvkvHZ 9ydAQXyBTTJUQ94ZhYVqJp1aBaoGl39I+DpQnXfGnsb06EiEde8OPDX6J/diZ3KPYvuhjz9JlN /OReVj/2/OQktGdGgaDJxLF8Gw3GuheJkZxmryHwBif7hck+XVgAScos30CRy7qz+z/GjcBz4H AooF4cL+wVph1NG8nKO4CgUdO+ZgYkzO4p0KCIx+HEnWdN49eOt0gkEawz44StqyrYoM1B+5j2 m7Hty47iQeS5sK1QFpFbv5mL
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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;
> +}
> +
> +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;
> +}

There should be no need for those handlers. As said elsewhere: for
guests registers not explicitly handled should return ~0 for reads and
drop writes, which is what you are proposing here.

> +
>  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]);
>          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);

This whole call should be made conditional to is_hwdom, as said above
there's no need for the guest_rom handlers.

Likewise I assume you expect IO BARs to simply return ~0 and drop
writes, as there's no explicit handler added for those?

>          if ( rc )
>              rom->type = VPCI_BAR_EMPTY;
>      }
> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> index ed127a08a953..0a73b14a92dc 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -68,7 +68,10 @@ struct vpci {
>      struct vpci_header {
>          /* Information about the PCI BARs of this device. */
>          struct vpci_bar {
> +            /* Physical view of the BAR. */

No, that's not the physical view, it's the physical (host) address.

>              uint64_t addr;
> +            /* Guest view of the BAR: address and lower bits. */
> +            uint64_t guest_reg;

I continue to think it would be clearer if you store the guest address
here (gaddr, without the low bits) and add those in guest_bar_read
based on bar->{type,prefetchable}. Then it would be equivalent to the
existing 'addr' field.

I wonder whether we need to protect the added code with
CONFIG_HAS_VPCI_GUEST_SUPPORT, this would effectively be dead code
otherwise. Long term I don't think we wish to differentiate between
dom0 and domU vPCI support at build time, so I'm unsure whether it's
helpful to pollute the code with CONFIG_HAS_VPCI_GUEST_SUPPORT when
the plan is to remove those long term.

Thanks, Roger.



 


Rackspace

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