| Message ID | 20230720003205.1828537-13-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 3 | expand |
On 20.07.2023 02:32, Volodymyr Babchuk wrote: > --- a/xen/drivers/vpci/vpci.c > +++ b/xen/drivers/vpci/vpci.c > @@ -177,6 +177,30 @@ static int add_virtual_device(struct pci_dev *pdev) > return 0; > } > > +/* > + * Find the physical device which is mapped to the virtual device > + * and translate virtual SBDF to the physical one. > + * This must hold domain's pci_lock in read mode. How about an assertion to that effect? > + */ > +bool vpci_translate_virtual_device(struct domain *d, pci_sbdf_t *sbdf) > +{ > + struct pci_dev *pdev; > + > + ASSERT(!is_hardware_domain(d)); > + > + for_each_pdev( d, pdev ) Nit: Style (either you consider for_each_pdev a [pseudo-]keyword or you don't; depending on that there's either a blank missing or there are two too many). Jan
On Thu, Jul 20, 2023 at 12:32:34AM +0000, Volodymyr Babchuk 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. > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > --- > Since v8: > - locks moved out of vpci_translate_virtual_device() > Since v6: > - add pcidevs locking to vpci_translate_virtual_device > - update wrt to the new locking scheme > Since v5: > - add vpci_translate_virtual_device for #ifndef CONFIG_HAS_VPCI_GUEST_SUPPORT > case to simplify ifdefery > - add ASSERT(!is_hardware_domain(d)); to vpci_translate_virtual_device > - reset output register on failed virtual SBDF translation > 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 | 47 +++++++++++++++++++++++++++++++++++++++-- > xen/drivers/vpci/vpci.c | 24 +++++++++++++++++++++ > xen/include/xen/vpci.h | 7 ++++++ > 3 files changed, 76 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c > index 3bc4bb5508..66701465af 100644 > --- a/xen/arch/arm/vpci.c > +++ b/xen/arch/arm/vpci.c > @@ -28,10 +28,33 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, > register_t *r, void *p) > { > struct pci_host_bridge *bridge = p; > - pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); > + pci_sbdf_t sbdf; > /* data is needed to prevent a pointer cast on 32bit */ > unsigned long data; > > + ASSERT(!bridge == !is_hardware_domain(v->domain)); > + > + sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); > + > + /* > + * For the passed through devices we need to map their virtual SBDF > + * to the physical PCI device being passed through. > + */ > + if ( !bridge ) > + { > + bool translated; > + > + read_lock(&v->domain->pci_lock); > + translated = vpci_translate_virtual_device(v->domain, &sbdf); > + read_unlock(&v->domain->pci_lock); > + > + if ( !translated ) > + { > + *r = ~0ul; > + return 1; > + } > + } > + > if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa), > 1U << info->dabt.size, &data) ) > { > @@ -48,7 +71,27 @@ static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info, > register_t r, void *p) > { > struct pci_host_bridge *bridge = p; > - pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); > + pci_sbdf_t sbdf; > + > + ASSERT(!bridge == !is_hardware_domain(v->domain)); > + > + sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); > + > + /* > + * For the passed through devices we need to map their virtual SBDF > + * to the physical PCI device being passed through. > + */ > + if ( !bridge ) > + { > + bool translated; > + > + read_lock(&v->domain->pci_lock); > + translated = vpci_translate_virtual_device(v->domain, &sbdf); > + read_unlock(&v->domain->pci_lock); You drop the lock here, so it's possible that returned SBDF is already stale by the time Xen does scan the domain pdev list again? I guess it's a minor issue. > + > + if ( !translated ) > + return 1; > + } > > 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 baaafe4a2a..2ce36e811d 100644 > --- a/xen/drivers/vpci/vpci.c > +++ b/xen/drivers/vpci/vpci.c > @@ -177,6 +177,30 @@ static int add_virtual_device(struct pci_dev *pdev) > return 0; > } > > +/* > + * Find the physical device which is mapped to the virtual device > + * and translate virtual SBDF to the physical one. > + * This must hold domain's pci_lock in read mode. > + */ > +bool vpci_translate_virtual_device(struct domain *d, pci_sbdf_t *sbdf) > +{ > + struct pci_dev *pdev; const for pdev and d. > + > + ASSERT(!is_hardware_domain(d)); > + > + for_each_pdev( d, pdev ) > + { > + if ( pdev->vpci && (pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf) ) > + { > + /* Replace guest SBDF with the physical one. */ > + *sbdf = pdev->sbdf; Since you are already iterating over the domain pdev list, won't it be more helpful to return the pdev? Thanks, Roger.
On Thu, Jul 20, 2023 at 12:32:34AM +0000, Volodymyr Babchuk 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. > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > --- > Since v8: > - locks moved out of vpci_translate_virtual_device() > Since v6: > - add pcidevs locking to vpci_translate_virtual_device > - update wrt to the new locking scheme > Since v5: > - add vpci_translate_virtual_device for #ifndef CONFIG_HAS_VPCI_GUEST_SUPPORT > case to simplify ifdefery > - add ASSERT(!is_hardware_domain(d)); to vpci_translate_virtual_device > - reset output register on failed virtual SBDF translation > 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 | 47 +++++++++++++++++++++++++++++++++++++++-- > xen/drivers/vpci/vpci.c | 24 +++++++++++++++++++++ > xen/include/xen/vpci.h | 7 ++++++ > 3 files changed, 76 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c > index 3bc4bb5508..66701465af 100644 > --- a/xen/arch/arm/vpci.c > +++ b/xen/arch/arm/vpci.c > @@ -28,10 +28,33 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, > register_t *r, void *p) > { > struct pci_host_bridge *bridge = p; > - pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); > + pci_sbdf_t sbdf; > /* data is needed to prevent a pointer cast on 32bit */ > unsigned long data; > > + ASSERT(!bridge == !is_hardware_domain(v->domain)); > + > + sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); > + > + /* > + * For the passed through devices we need to map their virtual SBDF > + * to the physical PCI device being passed through. > + */ > + if ( !bridge ) > + { > + bool translated; > + > + read_lock(&v->domain->pci_lock); > + translated = vpci_translate_virtual_device(v->domain, &sbdf); > + read_unlock(&v->domain->pci_lock); > + > + if ( !translated ) > + { > + *r = ~0ul; > + return 1; > + } > + } I've been thinking about this, is there any reason to not place this logic inside of vpci_sbdf_from_gpa()? I'm not sure you need to expose vpci_translate_virtual_device(). Thanks, Roger.
diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c index 3bc4bb5508..66701465af 100644 --- a/xen/arch/arm/vpci.c +++ b/xen/arch/arm/vpci.c @@ -28,10 +28,33 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info, register_t *r, void *p) { struct pci_host_bridge *bridge = p; - pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); + pci_sbdf_t sbdf; /* data is needed to prevent a pointer cast on 32bit */ unsigned long data; + ASSERT(!bridge == !is_hardware_domain(v->domain)); + + sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); + + /* + * For the passed through devices we need to map their virtual SBDF + * to the physical PCI device being passed through. + */ + if ( !bridge ) + { + bool translated; + + read_lock(&v->domain->pci_lock); + translated = vpci_translate_virtual_device(v->domain, &sbdf); + read_unlock(&v->domain->pci_lock); + + if ( !translated ) + { + *r = ~0ul; + return 1; + } + } + if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa), 1U << info->dabt.size, &data) ) { @@ -48,7 +71,27 @@ static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info, register_t r, void *p) { struct pci_host_bridge *bridge = p; - pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); + pci_sbdf_t sbdf; + + ASSERT(!bridge == !is_hardware_domain(v->domain)); + + sbdf = vpci_sbdf_from_gpa(bridge, info->gpa); + + /* + * For the passed through devices we need to map their virtual SBDF + * to the physical PCI device being passed through. + */ + if ( !bridge ) + { + bool translated; + + read_lock(&v->domain->pci_lock); + translated = vpci_translate_virtual_device(v->domain, &sbdf); + read_unlock(&v->domain->pci_lock); + + if ( !translated ) + return 1; + } 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 baaafe4a2a..2ce36e811d 100644 --- a/xen/drivers/vpci/vpci.c +++ b/xen/drivers/vpci/vpci.c @@ -177,6 +177,30 @@ static int add_virtual_device(struct pci_dev *pdev) return 0; } +/* + * Find the physical device which is mapped to the virtual device + * and translate virtual SBDF to the physical one. + * This must hold domain's pci_lock in read mode. + */ +bool vpci_translate_virtual_device(struct domain *d, pci_sbdf_t *sbdf) +{ + struct pci_dev *pdev; + + ASSERT(!is_hardware_domain(d)); + + for_each_pdev( d, pdev ) + { + if ( pdev->vpci && (pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf) ) + { + /* 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 pci_dev *pdev) { diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h index c55c45f7a1..7d30fbdd28 100644 --- a/xen/include/xen/vpci.h +++ b/xen/include/xen/vpci.h @@ -286,6 +286,7 @@ static inline bool __must_check vpci_process_pending(struct vcpu *v) /* Notify vPCI that device is assigned/de-assigned to/from guest. */ int vpci_assign_device(struct pci_dev *pdev); #define vpci_deassign_device vpci_remove_device +bool vpci_translate_virtual_device(struct domain *d, pci_sbdf_t *sbdf); #else static inline int vpci_assign_device(struct pci_dev *pdev) { @@ -295,6 +296,12 @@ static inline int vpci_assign_device(struct pci_dev *pdev) static inline void vpci_deassign_device(struct pci_dev *pdev) { }; + +static inline bool vpci_translate_virtual_device(struct domain *d, + pci_sbdf_t *sbdf) +{ + return false; +} #endif #endif