Message ID | 20211105065629.940943-12-andr2000@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | PCI devices passthrough on Arm, part 3 | expand |
On 05.11.2021 07:56, Oleksandr Andrushchenko wrote: > --- 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; Nit: Indentation. > @@ -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; Again. > @@ -172,10 +175,37 @@ REGISTER_VPCI_INIT(vpci_add_virtual_device, VPCI_PRIORITY_MIDDLE); > static void vpci_remove_virtual_device(struct domain *d, > const struct pci_dev *pdev) > { > + ASSERT(pcidevs_locked()); > + > clear_bit(pdev->vpci->guest_sbdf.dev, &d->vpci_dev_assigned_map); > 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(struct domain *d, pci_sbdf_t *sbdf) const struct domain *d ? > +{ > + const struct pci_dev *pdev; > + bool found = false; > + > + pcidevs_lock(); > + for_each_pdev( d, pdev ) > + { > + if ( pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf ) > + { > + /* Replace virtual SBDF with the physical one. */ > + *sbdf = pdev->sbdf; > + found = true; > + break; > + } > + } > + pcidevs_unlock(); I think the description wants to at least mention that in principle this is too coarse grained a lock, providing justification for why it is deemed good enough nevertheless. (Personally, as expressed before, I don't think the lock should be used here, but as long as Roger agrees with you, you're fine.) Jan
On 08.11.21 13:10, Jan Beulich wrote: > On 05.11.2021 07:56, Oleksandr Andrushchenko wrote: >> --- 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; > Nit: Indentation. Ouch, sure > >> @@ -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; > Again. Will fix > >> @@ -172,10 +175,37 @@ REGISTER_VPCI_INIT(vpci_add_virtual_device, VPCI_PRIORITY_MIDDLE); >> static void vpci_remove_virtual_device(struct domain *d, >> const struct pci_dev *pdev) >> { >> + ASSERT(pcidevs_locked()); >> + >> clear_bit(pdev->vpci->guest_sbdf.dev, &d->vpci_dev_assigned_map); >> 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(struct domain *d, pci_sbdf_t *sbdf) > const struct domain *d ? Will change > >> +{ >> + const struct pci_dev *pdev; >> + bool found = false; >> + >> + pcidevs_lock(); >> + for_each_pdev( d, pdev ) >> + { >> + if ( pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf ) >> + { >> + /* Replace virtual SBDF with the physical one. */ >> + *sbdf = pdev->sbdf; >> + found = true; >> + break; >> + } >> + } >> + pcidevs_unlock(); > I think the description wants to at least mention that in principle > this is too coarse grained a lock, providing justification for why > it is deemed good enough nevertheless. (Personally, as expressed > before, I don't think the lock should be used here, but as long as > Roger agrees with you, you're fine.) Yes, makes sense > > Jan > Thank you, Oleksandr
On Mon, Nov 08, 2021 at 11:16:42AM +0000, Oleksandr Andrushchenko wrote: > > > On 08.11.21 13:10, Jan Beulich wrote: > > On 05.11.2021 07:56, Oleksandr Andrushchenko wrote: > >> --- 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; > > Nit: Indentation. > Ouch, sure > > > >> @@ -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; > > Again. > Will fix > > > >> @@ -172,10 +175,37 @@ REGISTER_VPCI_INIT(vpci_add_virtual_device, VPCI_PRIORITY_MIDDLE); > >> static void vpci_remove_virtual_device(struct domain *d, > >> const struct pci_dev *pdev) > >> { > >> + ASSERT(pcidevs_locked()); > >> + > >> clear_bit(pdev->vpci->guest_sbdf.dev, &d->vpci_dev_assigned_map); > >> 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(struct domain *d, pci_sbdf_t *sbdf) > > const struct domain *d ? > Will change > > > >> +{ > >> + const struct pci_dev *pdev; > >> + bool found = false; > >> + > >> + pcidevs_lock(); > >> + for_each_pdev( d, pdev ) > >> + { > >> + if ( pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf ) > >> + { > >> + /* Replace virtual SBDF with the physical one. */ > >> + *sbdf = pdev->sbdf; > >> + found = true; > >> + break; > >> + } > >> + } > >> + pcidevs_unlock(); > > I think the description wants to at least mention that in principle > > this is too coarse grained a lock, providing justification for why > > it is deemed good enough nevertheless. (Personally, as expressed > > before, I don't think the lock should be used here, but as long as > > Roger agrees with you, you're fine.) > Yes, makes sense Seeing as we don't take the lock in vpci_{read,write} I'm not sure we need it here either then. Since on Arm you will add devices to the guest at runtime (ie: while there could already be PCI accesses), have you seen issues with not taking the lock here? I think the whole pcidevs locking needs to be clarified, as it's currently a mess. If you want to take it here that's fine, but overall there are issues in other places that would make removing a device at runtime not reliable. Thanks, Roger.
On 08.11.21 16:23, Roger Pau Monné wrote: > On Mon, Nov 08, 2021 at 11:16:42AM +0000, Oleksandr Andrushchenko wrote: >> >> On 08.11.21 13:10, Jan Beulich wrote: >>> On 05.11.2021 07:56, Oleksandr Andrushchenko wrote: >>>> --- 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; >>> Nit: Indentation. >> Ouch, sure >>>> @@ -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; >>> Again. >> Will fix >>>> @@ -172,10 +175,37 @@ REGISTER_VPCI_INIT(vpci_add_virtual_device, VPCI_PRIORITY_MIDDLE); >>>> static void vpci_remove_virtual_device(struct domain *d, >>>> const struct pci_dev *pdev) >>>> { >>>> + ASSERT(pcidevs_locked()); >>>> + >>>> clear_bit(pdev->vpci->guest_sbdf.dev, &d->vpci_dev_assigned_map); >>>> 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(struct domain *d, pci_sbdf_t *sbdf) >>> const struct domain *d ? >> Will change >>>> +{ >>>> + const struct pci_dev *pdev; >>>> + bool found = false; >>>> + >>>> + pcidevs_lock(); >>>> + for_each_pdev( d, pdev ) >>>> + { >>>> + if ( pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf ) >>>> + { >>>> + /* Replace virtual SBDF with the physical one. */ >>>> + *sbdf = pdev->sbdf; >>>> + found = true; >>>> + break; >>>> + } >>>> + } >>>> + pcidevs_unlock(); >>> I think the description wants to at least mention that in principle >>> this is too coarse grained a lock, providing justification for why >>> it is deemed good enough nevertheless. (Personally, as expressed >>> before, I don't think the lock should be used here, but as long as >>> Roger agrees with you, you're fine.) >> Yes, makes sense > Seeing as we don't take the lock in vpci_{read,write} I'm not sure we > need it here either then. Yes, I was not feeling confident while adding locking > Since on Arm you will add devices to the guest at runtime (ie: while > there could already be PCI accesses), have you seen issues with not > taking the lock here? No, I didn't. Neither I am aware of Arm had problems But this could just mean that we were lucky not to step on it > > I think the whole pcidevs locking needs to be clarified, as it's > currently a mess. Agree > If you want to take it here that's fine, but overall > there are issues in other places that would make removing a device at > runtime not reliable. So, what's the decision? I would leave the locks where I put them, so at least this part won't need fixes. > > Thanks, Roger. > Thank you, Oleksandr
On 08.11.21 17:28, Oleksandr Andrushchenko wrote: > > On 08.11.21 16:23, Roger Pau Monné wrote: >> On Mon, Nov 08, 2021 at 11:16:42AM +0000, Oleksandr Andrushchenko wrote: >>> On 08.11.21 13:10, Jan Beulich wrote: >>>> On 05.11.2021 07:56, Oleksandr Andrushchenko wrote: >>>>> --- 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; >>>> Nit: Indentation. >>> Ouch, sure >>>>> @@ -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; >>>> Again. >>> Will fix >>>>> @@ -172,10 +175,37 @@ REGISTER_VPCI_INIT(vpci_add_virtual_device, VPCI_PRIORITY_MIDDLE); >>>>> static void vpci_remove_virtual_device(struct domain *d, >>>>> const struct pci_dev *pdev) >>>>> { >>>>> + ASSERT(pcidevs_locked()); >>>>> + >>>>> clear_bit(pdev->vpci->guest_sbdf.dev, &d->vpci_dev_assigned_map); >>>>> 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(struct domain *d, pci_sbdf_t *sbdf) >>>> const struct domain *d ? >>> Will change >>>>> +{ >>>>> + const struct pci_dev *pdev; >>>>> + bool found = false; >>>>> + >>>>> + pcidevs_lock(); >>>>> + for_each_pdev( d, pdev ) >>>>> + { >>>>> + if ( pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf ) >>>>> + { >>>>> + /* Replace virtual SBDF with the physical one. */ >>>>> + *sbdf = pdev->sbdf; >>>>> + found = true; >>>>> + break; >>>>> + } >>>>> + } >>>>> + pcidevs_unlock(); >>>> I think the description wants to at least mention that in principle >>>> this is too coarse grained a lock, providing justification for why >>>> it is deemed good enough nevertheless. (Personally, as expressed >>>> before, I don't think the lock should be used here, but as long as >>>> Roger agrees with you, you're fine.) >>> Yes, makes sense >> Seeing as we don't take the lock in vpci_{read,write} I'm not sure we >> need it here either then. > Yes, I was not feeling confident while adding locking >> Since on Arm you will add devices to the guest at runtime (ie: while >> there could already be PCI accesses), have you seen issues with not >> taking the lock here? > No, I didn't. Neither I am aware of Arm had problems > But this could just mean that we were lucky not to step on it >> I think the whole pcidevs locking needs to be clarified, as it's >> currently a mess. > Agree >> If you want to take it here that's fine, but overall >> there are issues in other places that would make removing a device at >> runtime not reliable. > So, what's the decision? I would leave the locks where I put them, > so at least this part won't need fixes. As I am about to use the lock outside vpci struct in v5 all these go away >> Thanks, Roger. >> > Thank you, > Oleksandr
diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c index 5a6ebd8b9868..6a37f770f8f0 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 6657d236dc1a..cb0bde35b6a6 100644 --- a/xen/drivers/vpci/vpci.c +++ b/xen/drivers/vpci/vpci.c @@ -94,6 +94,7 @@ int vpci_add_handlers(struct pci_dev *pdev) /* We should not get here twice for the same device. */ ASSERT(!pdev->vpci); + ASSERT(pcidevs_locked()); pdev->vpci = xzalloc(struct vpci); if ( !pdev->vpci ) @@ -134,6 +135,8 @@ int vpci_add_virtual_device(struct pci_dev *pdev) pci_sbdf_t sbdf; unsigned long new_dev_number; + ASSERT(pcidevs_locked()); + /* * Each PCI bus supports 32 devices/slots at max or up to 256 when * there are multi-function ones which are not yet supported. @@ -172,10 +175,37 @@ REGISTER_VPCI_INIT(vpci_add_virtual_device, VPCI_PRIORITY_MIDDLE); static void vpci_remove_virtual_device(struct domain *d, const struct pci_dev *pdev) { + ASSERT(pcidevs_locked()); + clear_bit(pdev->vpci->guest_sbdf.dev, &d->vpci_dev_assigned_map); 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(struct domain *d, pci_sbdf_t *sbdf) +{ + const struct pci_dev *pdev; + bool found = false; + + pcidevs_lock(); + for_each_pdev( d, pdev ) + { + if ( pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf ) + { + /* Replace virtual SBDF with the physical one. */ + *sbdf = pdev->sbdf; + found = true; + break; + } + } + pcidevs_unlock(); + + return found; +} + /* 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 9cc7071bc0af..d5765301e442 100644 --- a/xen/include/xen/vpci.h +++ b/xen/include/xen/vpci.h @@ -274,6 +274,7 @@ static inline void vpci_cancel_pending(const 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(struct domain *d, pci_sbdf_t *sbdf); #else static inline int vpci_assign_device(struct domain *d, struct pci_dev *pdev) {