Re: [PATCH v3 04/11] vpci/header: Add and remove register handlers dynamically

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Sep 30, 2021 at 10:52:16AM +0300, 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.
> 
> Use stubs for guest domains for now.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> 
> ---
> 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
> ---
>  xen/drivers/vpci/header.c | 72 ++++++++++++++++++++++++++++++++++-----
>  xen/drivers/vpci/vpci.c   |  4 +--
>  xen/include/xen/vpci.h    |  8 +++++
>  3 files changed, 74 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 3d571356397a..1ce98795fcca 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -397,6 +397,17 @@ 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)
> +{
> +}
> +
> +static uint32_t guest_bar_read(const struct pci_dev *pdev, unsigned int reg,
> +                               void *data)
> +{
> +    return 0xffffffff;
> +}
> +
>  static void rom_write(const struct pci_dev *pdev, unsigned int reg,
>                        uint32_t val, void *data)
>  {
> @@ -445,14 +456,25 @@ static void rom_write(const struct pci_dev *pdev, 
> unsigned int reg,
>          rom->addr = val & PCI_ROM_ADDRESS_MASK;
>  }
>  
> -static int add_bar_handlers(const struct pci_dev *pdev)
> +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;
> +}

FWIW, I would also be fine with introducing the code for those
handlers at the same time.

> +
> +static int add_bar_handlers(const struct pci_dev *pdev, bool is_hwdom)

I would rather use is_hardware_domain(pdev->domain) than passing a
boolean here, no need to duplicate data which is already available
from the pdev.

>  {
>      unsigned int i;
>      struct vpci_header *header = &pdev->vpci->header;
>      struct vpci_bar *bars = header->bars;
>      int rc;
>  
> -    /* Setup a handler for the command register. */
> +    /* Setup a handler for the command register: same for hwdom and guests. 
> */
>      rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write, 
> PCI_COMMAND,
>                             2, header);
>      if ( rc )
> @@ -475,8 +497,13 @@ static int add_bar_handlers(const struct pci_dev *pdev)
>                  rom_reg = PCI_ROM_ADDRESS;
>              else
>                  rom_reg = PCI_ROM_ADDRESS1;
> -            rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write,
> -                                   rom_reg, 4, &bars[i]);
> +            if ( is_hwdom )
> +                rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write,
> +                                       rom_reg, 4, &bars[i]);
> +            else
> +                rc = vpci_add_register(pdev->vpci,
> +                                       guest_rom_read, guest_rom_write,
> +                                       rom_reg, 4, &bars[i]);

I think you could use:

else if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) )
    rc = vpci_add_register(...
else
    ASSERT_UNREACHABLE();

And then guard the guest_ handlers with CONFIG_HAS_VPCI_GUEST_SUPPORT.

>              if ( rc )
>                  return rc;
>          }
> @@ -485,8 +512,13 @@ static int add_bar_handlers(const struct pci_dev *pdev)
>              uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4;
>  
>              /* This is either VPCI_BAR_MEM32 or VPCI_BAR_MEM64_{LO|HI}. */
> -            rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, 
> reg,
> -                                   4, &bars[i]);
> +            if ( is_hwdom )
> +                rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write,
> +                                       reg, 4, &bars[i]);
> +            else
> +                rc = vpci_add_register(pdev->vpci,
> +                                       guest_bar_read, guest_bar_write,
> +                                       reg, 4, &bars[i]);
>              if ( rc )
>                  return rc;
>          }
> @@ -520,7 +552,7 @@ static int init_bars(struct pci_dev *pdev)
>      }
>  
>      if ( pdev->ignore_bars )
> -        return add_bar_handlers(pdev);
> +        return add_bar_handlers(pdev, true);
>  
>      /* Disable memory decoding before sizing. */
>      cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
> @@ -582,7 +614,7 @@ static int init_bars(struct pci_dev *pdev)
>                                PCI_ROM_ADDRESS_ENABLE;
>      }
>  
> -    rc = add_bar_handlers(pdev);
> +    rc = add_bar_handlers(pdev, true);
>      if ( rc )
>      {
>          pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
> @@ -593,6 +625,30 @@ static int init_bars(struct pci_dev *pdev)
>  }
>  REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
>  
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +int vpci_bar_add_handlers(const struct domain *d, const struct pci_dev *pdev)
> +{
> +    int rc;
> +
> +    /* Remove previously added registers. */
> +    vpci_remove_device_registers(pdev);

Shouldn't this be done by vpci_assign_device as a preparation for
assigning the device?

> +
> +    rc = add_bar_handlers(pdev, is_hardware_domain(d));

Also this model seems to assume that vPCI will require the hardware
domain to have owned the device before it being assigned to a guest,
but for example when using a PV dom0 that won't be the case, and hence
we would need the vPCI fields to be filled when assigning to a guest.

Hence I wonder whether we shouldn't do a full re-initialization when
assigning to a guest instead of this partial one.

> +    if ( rc )
> +        gdprintk(XENLOG_ERR,
> +                 "%pp: failed to add BAR handlers for dom%pd: %d\n",
> +                 &pdev->sbdf, d, rc);
> +    return rc;
> +}
> +
> +int vpci_bar_remove_handlers(const struct domain *d, const struct pci_dev 
> *pdev)
> +{
> +    /* Remove previously added registers. */
> +    vpci_remove_device_registers(pdev);
> +    return 0;
> +}
> +#endif
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index 0fe86cb30d23..702f7b5d5dda 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -95,7 +95,7 @@ int vpci_assign_device(struct domain *d, const struct 
> pci_dev *dev)
>      if ( is_system_domain(d) || !has_vpci(d) )
>          return 0;
>  
> -    return 0;
> +    return vpci_bar_add_handlers(d, dev);
>  }
>  
>  /* Notify vPCI that device is de-assigned from guest. */
> @@ -105,7 +105,7 @@ int vpci_deassign_device(struct domain *d, const struct 
> pci_dev *dev)
>      if ( is_system_domain(d) || !has_vpci(d) )
>          return 0;
>  
> -    return 0;
> +    return vpci_bar_remove_handlers(d, dev);

I think it would be better to use something similar to
REGISTER_VPCI_INIT here, otherwise this will need to be modified every
time a new capability is handled by Xen.

Maybe we could reuse or expand REGISTER_VPCI_INIT adding another field
to be used for guest initialization?

>  }
>  #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
>  
> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> index ecc08f2c0f65..fd822c903af5 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -57,6 +57,14 @@ uint32_t vpci_hw_read32(const struct pci_dev *pdev, 
> unsigned int reg,
>   */
>  bool __must_check vpci_process_pending(struct vcpu *v);
>  
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +/* Add/remove BAR handlers for a domain. */
> +int vpci_bar_add_handlers(const struct domain *d,
> +                          const struct pci_dev *pdev);
> +int vpci_bar_remove_handlers(const struct domain *d,
> +                             const struct pci_dev *pdev);
> +#endif

This would then go away if we implement a mechanism similar to
REGISTER_VPCI_INIT.

Thanks, Roger.