On Tue, Feb 01, 2022 at 08:56:49AM +0000, Oleksandr Andrushchenko wrote: > Hi, Roger! > > On 31.01.22 10:45, Oleksandr Andrushchenko wrote: > > Hi, Roger! > > > > On 13.01.22 13:40, Roger Pau Monné wrote: > >> On Thu, Nov 25, 2021 at 01:02:42PM +0200, Oleksandr Andrushchenko wrote: > >>> From: Oleksandr Andrushchenko <[email protected]> > >>> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT > >>> +/* Notify vPCI that device is assigned to guest. */ > >>> +int vpci_assign_device(struct domain *d, struct pci_dev *pdev) > >>> +{ > >>> + int rc; > >>> + > >>> + /* It only makes sense to assign for hwdom or guest domain. */ > >>> + if ( is_system_domain(d) || !has_vpci(d) ) > >>> + return 0; > >>> + > >>> + spin_lock(&pdev->vpci_lock); > >>> + rc = run_vpci_init(pdev); > >> Following my comment below, this will likely need to call > >> vpci_add_handlers in order to allocate the pdev->vpci field. > >> > >> It's not OK to carry the contents of pdev->vpci across domain > >> assignations, as the device should be reset, and thus the content of > >> pdev->vpci would be stale. > >> > >>> + spin_unlock(&pdev->vpci_lock); > >>> + if ( rc ) > >>> + vpci_deassign_device(d, pdev); > >>> + > >>> + return rc; > >>> +} > >>> + > >>> +/* Notify vPCI that device is de-assigned from guest. */ > >>> +int vpci_deassign_device(struct domain *d, struct pci_dev *pdev) > >>> +{ > >>> + /* It only makes sense to de-assign from hwdom or guest domain. */ > >>> + if ( is_system_domain(d) || !has_vpci(d) ) > >>> + return 0; > >>> + > >>> + spin_lock(&pdev->vpci_lock); > >>> + vpci_remove_device_handlers_locked(pdev); > >> You need to free the pdev->vpci structure on deassign. I would expect > >> the device to be reset on deassign, so keeping the pdev->vpci contents > >> would be wrong. > > Sure, I will re-allocate pdev->vpci then > After thinking a bit more on this I have realized that we cannot free > pdev->vpci on de-assign. The reason for that is the fact that vpci > structure contains vital data which is collected and managed at different > stages: for example, BAR types are collected while we run for the > hardware domain and in init_bars we collect the types of the BARS etc. > This is then used while assigning device to construct guest's representation > of the device. Freeing vpci will lead to that data is lost and the required > data is not populated into vpci. > So, it is no possible to free vpci structure and I am about to leave the > approach as it is.
We discussed this on IRC, and we have agreed that it's possible to free pdev->vpci on deassign since in any case we need to call init_bars (and other capability init functions) when the device is assigned to setup the register traps and fetch the required information in order to fill pdev->vpci. Roger.
