On Thu, Sep 30, 2021 at 10:52:16AM +0300, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <[email protected]>
>
> 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 <[email protected]>
>
> ---
> 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.
