| Message ID | 20211125110251.2877218-13-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 3 | expand |
On Thu, Nov 25, 2021 at 01:02:49PM +0200, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > There are three originators for the PCI configuration space access: > 1. The domain that owns physical host bridge: MMIO handlers are > there so we can update vPCI register handlers with the values > written by the hardware domain, e.g. physical view of the registers > vs guest's view on the configuration space. > 2. Guest access to the passed through PCI devices: we need to properly > map virtual bus topology to the physical one, e.g. pass the configuration > space access to the corresponding physical devices. > 3. Emulated host PCI bridge access. It doesn't exist in the physical > topology, e.g. it can't be mapped to some physical host bridge. > So, all access to the host bridge itself needs to be trapped and > emulated. I'm kind of lost in this commit message. You are just adding a translate function in order for domUs to translate from virtual SBDF to the physical SBDF of the device. I realize you do that based on whether 'bridge' is set or not, so I assume this is just a way to signal whether the domain is a hardware domain or not. Ie: !!bridge == is_hardware_domain(v->domain). > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > --- > Since v4: > - indentation fixes > - constify struct domain > - updated commit message > - updates to the new locking scheme (pdev->vpci_lock) > Since v3: > - revisit locking > - move code to vpci.c > Since v2: > - pass struct domain instead of struct vcpu > - constify arguments where possible > - gate relevant code with CONFIG_HAS_VPCI_GUEST_SUPPORT > New in v2 > --- > xen/arch/arm/vpci.c | 18 ++++++++++++++++++ > xen/drivers/vpci/vpci.c | 27 +++++++++++++++++++++++++++ > xen/include/xen/vpci.h | 1 + > 3 files changed, 46 insertions(+) > > diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c > index 8e801f275879..3d134f42d07e 100644 > --- a/xen/arch/arm/vpci.c > +++ b/xen/arch/arm/vpci.c > @@ -41,6 +41,15 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, > /* data is needed to prevent a pointer cast on 32bit */ > unsigned long data; > > +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT > + /* > + * For the passed through devices we need to map their virtual SBDF > + * to the physical PCI device being passed through. > + */ > + if ( !bridge && !vpci_translate_virtual_device(v->domain, &sbdf) ) > + return 1; I'm unsure what returning 1 implies for Arm here, but you likely need to set '*r = ~0ul;'. > +#endif > + > if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa), > 1U << info->dabt.size, &data) ) > { > @@ -59,6 +68,15 @@ static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info, > struct pci_host_bridge *bridge = p; > pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); > > +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT > + /* > + * For the passed through devices we need to map their virtual SBDF > + * to the physical PCI device being passed through. > + */ > + if ( !bridge && !vpci_translate_virtual_device(v->domain, &sbdf) ) > + return 1; > +#endif > + > return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa), > 1U << info->dabt.size, r); > } > diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c > index c2fb4d4db233..bdc8c63f73fa 100644 > --- a/xen/drivers/vpci/vpci.c > +++ b/xen/drivers/vpci/vpci.c > @@ -195,6 +195,33 @@ static void vpci_remove_virtual_device(struct domain *d, > pdev->vpci->guest_sbdf.sbdf = ~0; > } > > +/* > + * Find the physical device which is mapped to the virtual device > + * and translate virtual SBDF to the physical one. > + */ > +bool vpci_translate_virtual_device(const struct domain *d, pci_sbdf_t *sbdf) > +{ > + struct pci_dev *pdev; > + I would add: ASSERT(!is_hardware_domain(d)); To make sure this is not used for the hardware domain. > + for_each_pdev( d, pdev ) > + { > + bool found; > + > + spin_lock(&pdev->vpci_lock); > + found = pdev->vpci && (pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf); > + spin_unlock(&pdev->vpci_lock); > + > + if ( found ) > + { > + /* Replace guest SBDF with the physical one. */ > + *sbdf = pdev->sbdf; > + return true; > + } > + } > + > + return false; > +} > + > /* Notify vPCI that device is assigned to guest. */ > int vpci_assign_device(struct domain *d, struct pci_dev *pdev) > { > diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h > index e5258bd7ce90..21d76929391f 100644 > --- a/xen/include/xen/vpci.h > +++ b/xen/include/xen/vpci.h > @@ -280,6 +280,7 @@ static inline void vpci_cancel_pending_locked(struct pci_dev *pdev) > /* Notify vPCI that device is assigned/de-assigned to/from guest. */ > int vpci_assign_device(struct domain *d, struct pci_dev *pdev); > int vpci_deassign_device(struct domain *d, struct pci_dev *pdev); > +bool vpci_translate_virtual_device(const struct domain *d, pci_sbdf_t *sbdf); > #else > static inline int vpci_assign_device(struct domain *d, struct pci_dev *pdev) > { If you add a dummy vpci_translate_virtual_device helper that returns false unconditionally here you could drop the #ifdefs in arm/vpci.c AFAICT. Thanks, Roger.
Hi, Roger! On 13.01.22 14:18, Roger Pau Monné wrote: > On Thu, Nov 25, 2021 at 01:02:49PM +0200, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> There are three originators for the PCI configuration space access: >> 1. The domain that owns physical host bridge: MMIO handlers are >> there so we can update vPCI register handlers with the values >> written by the hardware domain, e.g. physical view of the registers >> vs guest's view on the configuration space. >> 2. Guest access to the passed through PCI devices: we need to properly >> map virtual bus topology to the physical one, e.g. pass the configuration >> space access to the corresponding physical devices. >> 3. Emulated host PCI bridge access. It doesn't exist in the physical >> topology, e.g. it can't be mapped to some physical host bridge. >> So, all access to the host bridge itself needs to be trapped and >> emulated. > I'm kind of lost in this commit message. You are just adding a > translate function in order for domUs to translate from virtual SBDF > to the physical SBDF of the device. I realize you do that based on > whether 'bridge' is set or not, so I assume this is just a way to > signal whether the domain is a hardware domain or not. Ie: > !!bridge == is_hardware_domain(v->domain). Simply put: yes > >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> --- >> Since v4: >> - indentation fixes >> - constify struct domain >> - updated commit message >> - updates to the new locking scheme (pdev->vpci_lock) >> Since v3: >> - revisit locking >> - move code to vpci.c >> Since v2: >> - pass struct domain instead of struct vcpu >> - constify arguments where possible >> - gate relevant code with CONFIG_HAS_VPCI_GUEST_SUPPORT >> New in v2 >> --- >> xen/arch/arm/vpci.c | 18 ++++++++++++++++++ >> xen/drivers/vpci/vpci.c | 27 +++++++++++++++++++++++++++ >> xen/include/xen/vpci.h | 1 + >> 3 files changed, 46 insertions(+) >> >> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c >> index 8e801f275879..3d134f42d07e 100644 >> --- a/xen/arch/arm/vpci.c >> +++ b/xen/arch/arm/vpci.c >> @@ -41,6 +41,15 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, >> /* data is needed to prevent a pointer cast on 32bit */ >> unsigned long data; >> >> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT >> + /* >> + * For the passed through devices we need to map their virtual SBDF >> + * to the physical PCI device being passed through. >> + */ >> + if ( !bridge && !vpci_translate_virtual_device(v->domain, &sbdf) ) >> + return 1; > I'm unsure what returning 1 implies for Arm here, but you likely need > to set '*r = ~0ul;'. Good catch, will add > >> +#endif >> + >> if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa), >> 1U << info->dabt.size, &data) ) >> { >> @@ -59,6 +68,15 @@ static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info, >> struct pci_host_bridge *bridge = p; >> pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); >> >> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT >> + /* >> + * For the passed through devices we need to map their virtual SBDF >> + * to the physical PCI device being passed through. >> + */ >> + if ( !bridge && !vpci_translate_virtual_device(v->domain, &sbdf) ) >> + return 1; >> +#endif >> + >> return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa), >> 1U << info->dabt.size, r); >> } >> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c >> index c2fb4d4db233..bdc8c63f73fa 100644 >> --- a/xen/drivers/vpci/vpci.c >> +++ b/xen/drivers/vpci/vpci.c >> @@ -195,6 +195,33 @@ static void vpci_remove_virtual_device(struct domain *d, >> pdev->vpci->guest_sbdf.sbdf = ~0; >> } >> >> +/* >> + * Find the physical device which is mapped to the virtual device >> + * and translate virtual SBDF to the physical one. >> + */ >> +bool vpci_translate_virtual_device(const struct domain *d, pci_sbdf_t *sbdf) >> +{ >> + struct pci_dev *pdev; >> + > I would add: > > ASSERT(!is_hardware_domain(d)); > > To make sure this is not used for the hardware domain. Will add > >> + for_each_pdev( d, pdev ) >> + { >> + bool found; >> + >> + spin_lock(&pdev->vpci_lock); >> + found = pdev->vpci && (pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf); >> + spin_unlock(&pdev->vpci_lock); >> + >> + if ( found ) >> + { >> + /* Replace guest SBDF with the physical one. */ >> + *sbdf = pdev->sbdf; >> + return true; >> + } >> + } >> + >> + return false; >> +} >> + >> /* Notify vPCI that device is assigned to guest. */ >> int vpci_assign_device(struct domain *d, struct pci_dev *pdev) >> { >> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h >> index e5258bd7ce90..21d76929391f 100644 >> --- a/xen/include/xen/vpci.h >> +++ b/xen/include/xen/vpci.h >> @@ -280,6 +280,7 @@ static inline void vpci_cancel_pending_locked(struct pci_dev *pdev) >> /* Notify vPCI that device is assigned/de-assigned to/from guest. */ >> int vpci_assign_device(struct domain *d, struct pci_dev *pdev); >> int vpci_deassign_device(struct domain *d, struct pci_dev *pdev); >> +bool vpci_translate_virtual_device(const struct domain *d, pci_sbdf_t *sbdf); >> #else >> static inline int vpci_assign_device(struct domain *d, struct pci_dev *pdev) >> { > If you add a dummy vpci_translate_virtual_device helper that returns > false unconditionally here you could drop the #ifdefs in arm/vpci.c > AFAICT. Will try to do so > > Thanks, Roger. Thank you, Oleksandr
diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c index 8e801f275879..3d134f42d07e 100644 --- a/xen/arch/arm/vpci.c +++ b/xen/arch/arm/vpci.c @@ -41,6 +41,15 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, /* data is needed to prevent a pointer cast on 32bit */ unsigned long data; +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT + /* + * For the passed through devices we need to map their virtual SBDF + * to the physical PCI device being passed through. + */ + if ( !bridge && !vpci_translate_virtual_device(v->domain, &sbdf) ) + return 1; +#endif + if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa), 1U << info->dabt.size, &data) ) { @@ -59,6 +68,15 @@ static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info, struct pci_host_bridge *bridge = p; pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT + /* + * For the passed through devices we need to map their virtual SBDF + * to the physical PCI device being passed through. + */ + if ( !bridge && !vpci_translate_virtual_device(v->domain, &sbdf) ) + return 1; +#endif + return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa), 1U << info->dabt.size, r); } diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c index c2fb4d4db233..bdc8c63f73fa 100644 --- a/xen/drivers/vpci/vpci.c +++ b/xen/drivers/vpci/vpci.c @@ -195,6 +195,33 @@ static void vpci_remove_virtual_device(struct domain *d, pdev->vpci->guest_sbdf.sbdf = ~0; } +/* + * Find the physical device which is mapped to the virtual device + * and translate virtual SBDF to the physical one. + */ +bool vpci_translate_virtual_device(const struct domain *d, pci_sbdf_t *sbdf) +{ + struct pci_dev *pdev; + + for_each_pdev( d, pdev ) + { + bool found; + + spin_lock(&pdev->vpci_lock); + found = pdev->vpci && (pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf); + spin_unlock(&pdev->vpci_lock); + + if ( found ) + { + /* Replace guest SBDF with the physical one. */ + *sbdf = pdev->sbdf; + return true; + } + } + + return false; +} + /* Notify vPCI that device is assigned to guest. */ int vpci_assign_device(struct domain *d, struct pci_dev *pdev) { diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h index e5258bd7ce90..21d76929391f 100644 --- a/xen/include/xen/vpci.h +++ b/xen/include/xen/vpci.h @@ -280,6 +280,7 @@ static inline void vpci_cancel_pending_locked(struct pci_dev *pdev) /* Notify vPCI that device is assigned/de-assigned to/from guest. */ int vpci_assign_device(struct domain *d, struct pci_dev *pdev); int vpci_deassign_device(struct domain *d, struct pci_dev *pdev); +bool vpci_translate_virtual_device(const struct domain *d, pci_sbdf_t *sbdf); #else static inline int vpci_assign_device(struct domain *d, struct pci_dev *pdev) {