[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 11/11] xen/arm: translate virtual PCI bus topology for guests
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |