Re: [PATCH v5 03/14] vpci: move lock outside of struct vpci
Hi, Roger! On 31.01.22 10:56, Roger Pau Monné wrote: > On Fri, Jan 28, 2022 at 02:15:08PM +, Oleksandr Andrushchenko wrote: >> Hi, Roger, Jan! >> >> On 13.01.22 10:58, Roger Pau Monné wrote: >>> On Wed, Jan 12, 2022 at 04:52:51PM +0100, Jan Beulich wrote: On 12.01.2022 16:42, Roger Pau Monné wrote: > On Wed, Jan 12, 2022 at 03:57:36PM +0100, Jan Beulich wrote: >> On 25.11.2021 12:02, Oleksandr Andrushchenko wrote: >>> @@ -379,7 +396,6 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int >>> reg, unsigned int size) >>> >>>data = merge_result(data, tmp_data, size - data_offset, >>> data_offset); >>>} >>> -spin_unlock(&pdev->vpci->lock); >>> >>>return data & (0x >> (32 - 8 * size)); >>>} >> Here and ... >> >>> @@ -475,13 +498,12 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int >>> reg, unsigned int size, >>>break; >>>ASSERT(data_offset < size); >>>} >>> +spin_unlock(&pdev->vpci_lock); >>> >>>if ( data_offset < size ) >>>/* Tailing gap, write the remaining. */ >>>vpci_write_hw(sbdf, reg + data_offset, size - data_offset, >>> data >> (data_offset * 8)); >>> - >>> -spin_unlock(&pdev->vpci->lock); >>>} >> ... even more so here I'm not sure of the correctness of the moving >> you do: While pdev->vpci indeed doesn't get accessed, I wonder >> whether there wasn't an intention to avoid racing calls to >> vpci_{read,write}_hw() this way. In any event I think such movement >> would need justification in the description. > I agree about the need for justification in the commit message, or > even better this being split into a pre-patch, as it's not related to > the lock switching done here. > > I do think this is fine however, as racing calls to > vpci_{read,write}_hw() shouldn't be a problem. Those are just wrappers > around pci_conf_{read,write} functions, and the required locking (in > case of using the IO ports) is already taken care in > pci_conf_{read,write}. IOW you consider it acceptable for a guest (really: Dom0) read racing a write to read back only part of what was written (so far)? I would think individual multi-byte reads and writes should appear atomic to the guest. >>> We split 64bit writes into two 32bit ones without taking the lock for >>> the whole duration of the access, so it's already possible to see a >>> partially updated state as a result of a 64bit write. >>> >>> I'm going over the PCI(e) spec but I don't seem to find anything about >>> whether the ECAM is allowed to split memory transactions into multiple >>> Configuration Requests, and whether those could then interleave with >>> requests from a different CPU. >> So, with the above is it still fine for you to have the change as is or >> you want this optimization to go into a dedicated patch before this one? > The change seems slightly controversial, so I think it would be best > if it was split into a separate patch with a proper reasoning in the > commit message. Sure, will move into a dedicated patch then > > Thanks, Roger. Thank you, Oleksandr
Re: [PATCH v5 03/14] vpci: move lock outside of struct vpci
On Fri, Jan 28, 2022 at 02:15:08PM +, Oleksandr Andrushchenko wrote: > Hi, Roger, Jan! > > On 13.01.22 10:58, Roger Pau Monné wrote: > > On Wed, Jan 12, 2022 at 04:52:51PM +0100, Jan Beulich wrote: > >> On 12.01.2022 16:42, Roger Pau Monné wrote: > >>> On Wed, Jan 12, 2022 at 03:57:36PM +0100, Jan Beulich wrote: > On 25.11.2021 12:02, Oleksandr Andrushchenko wrote: > > @@ -379,7 +396,6 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int > > reg, unsigned int size) > > > > data = merge_result(data, tmp_data, size - data_offset, > > data_offset); > > } > > -spin_unlock(&pdev->vpci->lock); > > > > return data & (0x >> (32 - 8 * size)); > > } > Here and ... > > > @@ -475,13 +498,12 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int > > reg, unsigned int size, > > break; > > ASSERT(data_offset < size); > > } > > +spin_unlock(&pdev->vpci_lock); > > > > if ( data_offset < size ) > > /* Tailing gap, write the remaining. */ > > vpci_write_hw(sbdf, reg + data_offset, size - data_offset, > > data >> (data_offset * 8)); > > - > > -spin_unlock(&pdev->vpci->lock); > > } > ... even more so here I'm not sure of the correctness of the moving > you do: While pdev->vpci indeed doesn't get accessed, I wonder > whether there wasn't an intention to avoid racing calls to > vpci_{read,write}_hw() this way. In any event I think such movement > would need justification in the description. > >>> I agree about the need for justification in the commit message, or > >>> even better this being split into a pre-patch, as it's not related to > >>> the lock switching done here. > >>> > >>> I do think this is fine however, as racing calls to > >>> vpci_{read,write}_hw() shouldn't be a problem. Those are just wrappers > >>> around pci_conf_{read,write} functions, and the required locking (in > >>> case of using the IO ports) is already taken care in > >>> pci_conf_{read,write}. > >> IOW you consider it acceptable for a guest (really: Dom0) read racing > >> a write to read back only part of what was written (so far)? I would > >> think individual multi-byte reads and writes should appear atomic to > >> the guest. > > We split 64bit writes into two 32bit ones without taking the lock for > > the whole duration of the access, so it's already possible to see a > > partially updated state as a result of a 64bit write. > > > > I'm going over the PCI(e) spec but I don't seem to find anything about > > whether the ECAM is allowed to split memory transactions into multiple > > Configuration Requests, and whether those could then interleave with > > requests from a different CPU. > So, with the above is it still fine for you to have the change as is or > you want this optimization to go into a dedicated patch before this one? The change seems slightly controversial, so I think it would be best if it was split into a separate patch with a proper reasoning in the commit message. Thanks, Roger.
Re: [PATCH v5 03/14] vpci: move lock outside of struct vpci
Hi, Jan, Roger! On 26.01.22 13:13, Roger Pau Monné wrote: > On Wed, Jan 26, 2022 at 08:40:09AM +, Oleksandr Andrushchenko wrote: >> Hello, Roger, Jan! >> >> On 12.01.22 16:42, Jan Beulich wrote: >>> On 11.01.2022 16:17, Roger Pau Monné wrote: On Thu, Nov 25, 2021 at 01:02:40PM +0200, Oleksandr Andrushchenko wrote: > diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c > index 657697fe3406..ceaac4516ff8 100644 > --- a/xen/drivers/vpci/vpci.c > +++ b/xen/drivers/vpci/vpci.c > @@ -35,12 +35,10 @@ extern vpci_register_init_t *const > __start_vpci_array[]; >extern vpci_register_init_t *const __end_vpci_array[]; >#define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array) > > -void vpci_remove_device(struct pci_dev *pdev) > +static void vpci_remove_device_handlers_locked(struct pci_dev *pdev) >{ > -if ( !has_vpci(pdev->domain) ) > -return; > +ASSERT(spin_is_locked(&pdev->vpci_lock)); > > -spin_lock(&pdev->vpci->lock); >while ( !list_empty(&pdev->vpci->handlers) ) >{ >struct vpci_register *r = > list_first_entry(&pdev->vpci->handlers, > @@ -50,15 +48,33 @@ void vpci_remove_device(struct pci_dev *pdev) >list_del(&r->node); >xfree(r); >} > -spin_unlock(&pdev->vpci->lock); > +} > + > +void vpci_remove_device_locked(struct pci_dev *pdev) I think this could be static instead, as it's only used by vpci_remove_device and vpci_add_handlers which are local to the file. >> This is going to be used outside later on while processing pending mappings, >> so I think it is not worth it defining it static here and then removing the >> static >> key word later on: please see [PATCH v5 04/14] vpci: cancel pending >> map/unmap on vpci removal [1] > I have some comments there also, which might change the approach > you are using. > >>> Does the splitting out of vpci_remove_device_handlers_locked() belong in >>> this patch in the first place? There's no second caller being added, so >>> this looks to be an orthogonal adjustment. >> I think of it as a preparation for the upcoming code: although the reason >> for the >> change might not be immediately seen in this patch it is still in line with >> what >> happens next. > Right - it's generally best if the change is done together as the new > callers are added. Otherwise it's hard to understand why certain changes > are made, and you will likely get asked the same question on next > rounds. > > It's also possible that the code that requires this is changed in > further iterations so there's no longer a need for the splitting. Ok, sounds reasonable I will not split the functions without the obvious need > > Thanks, Roger. Thank you, Oleksandr
Re: [PATCH v5 03/14] vpci: move lock outside of struct vpci
Hi, Jan! On 12.01.22 16:57, Jan Beulich wrote: > On 25.11.2021 12:02, Oleksandr Andrushchenko wrote: >> @@ -68,12 +84,13 @@ int vpci_add_handlers(struct pci_dev *pdev) >> /* We should not get here twice for the same device. */ >> ASSERT(!pdev->vpci); >> >> -pdev->vpci = xzalloc(struct vpci); >> -if ( !pdev->vpci ) >> +vpci = xzalloc(struct vpci); >> +if ( !vpci ) >> return -ENOMEM; >> >> +spin_lock(&pdev->vpci_lock); >> +pdev->vpci = vpci; >> INIT_LIST_HEAD(&pdev->vpci->handlers); >> -spin_lock_init(&pdev->vpci->lock); > INIT_LIST_HEAD() can occur ahead of taking the lock, and can also act > on &vpci->handlers rather than &pdev->vpci->handlers. Yes, I will move it, good catch >> for ( i = 0; i < NUM_VPCI_INIT; i++ ) >> { >> @@ -83,7 +100,8 @@ int vpci_add_handlers(struct pci_dev *pdev) >> } > This loop wants to live in the locked region because you need to install > vpci into pdev->vpci up front, afaict. I wonder whether that couldn't > be relaxed, but perhaps that's an improvement that can be thought about > later. Ok, so I'll leave it as is > > The main reason I'm looking at this closely is because from the patch > title I didn't expect new locking regions to be introduced right here; > instead I did expect strictly a mechanical conversion. > >> @@ -152,8 +170,6 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t >> *read_handler, >> r->offset = offset; >> r->private = data; >> >> -spin_lock(&vpci->lock); > From the description I can't deduce why this lock is fine to go away > now, i.e. that all call sites have the lock now acquire earlier. > Therefore I'd expect at least an assertion to be left here ... > >> @@ -183,7 +197,6 @@ int vpci_remove_register(struct vpci *vpci, unsigned int >> offset, >> const struct vpci_register r = { .offset = offset, .size = size }; >> struct vpci_register *rm; >> >> -spin_lock(&vpci->lock); > ... and here. Previously the lock lived in struct vpci and now it lives in struct pci_dev which is not visible here, so: 1. we cannot take that lock here and do expect for it to be acquired outside 2. we cannot add an ASSERT here as we would need ASSERT(spin_is_locked(&pdev->vpci_lock)); and pdev is not here All the callers of the vpci_{add|remove}_register are REGISTER_VPCI_INIT functions which are called with &pdev->vpci_lock held. So, while I agree that it would be indeed a good check with ASSERT here, but adding an additional argument to the respective functions just for that might not be a good idea IMO I will describe this lock removal in the commit message Thank you, Oleksandr
Re: [PATCH v5 03/14] vpci: move lock outside of struct vpci
Hi, Roger, Jan! On 13.01.22 10:58, Roger Pau Monné wrote: > On Wed, Jan 12, 2022 at 04:52:51PM +0100, Jan Beulich wrote: >> On 12.01.2022 16:42, Roger Pau Monné wrote: >>> On Wed, Jan 12, 2022 at 03:57:36PM +0100, Jan Beulich wrote: On 25.11.2021 12:02, Oleksandr Andrushchenko wrote: > @@ -379,7 +396,6 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, > unsigned int size) > > data = merge_result(data, tmp_data, size - data_offset, > data_offset); > } > -spin_unlock(&pdev->vpci->lock); > > return data & (0x >> (32 - 8 * size)); > } Here and ... > @@ -475,13 +498,12 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, > unsigned int size, > break; > ASSERT(data_offset < size); > } > +spin_unlock(&pdev->vpci_lock); > > if ( data_offset < size ) > /* Tailing gap, write the remaining. */ > vpci_write_hw(sbdf, reg + data_offset, size - data_offset, > data >> (data_offset * 8)); > - > -spin_unlock(&pdev->vpci->lock); > } ... even more so here I'm not sure of the correctness of the moving you do: While pdev->vpci indeed doesn't get accessed, I wonder whether there wasn't an intention to avoid racing calls to vpci_{read,write}_hw() this way. In any event I think such movement would need justification in the description. >>> I agree about the need for justification in the commit message, or >>> even better this being split into a pre-patch, as it's not related to >>> the lock switching done here. >>> >>> I do think this is fine however, as racing calls to >>> vpci_{read,write}_hw() shouldn't be a problem. Those are just wrappers >>> around pci_conf_{read,write} functions, and the required locking (in >>> case of using the IO ports) is already taken care in >>> pci_conf_{read,write}. >> IOW you consider it acceptable for a guest (really: Dom0) read racing >> a write to read back only part of what was written (so far)? I would >> think individual multi-byte reads and writes should appear atomic to >> the guest. > We split 64bit writes into two 32bit ones without taking the lock for > the whole duration of the access, so it's already possible to see a > partially updated state as a result of a 64bit write. > > I'm going over the PCI(e) spec but I don't seem to find anything about > whether the ECAM is allowed to split memory transactions into multiple > Configuration Requests, and whether those could then interleave with > requests from a different CPU. So, with the above is it still fine for you to have the change as is or you want this optimization to go into a dedicated patch before this one? > > Thanks, Roger. Thank you, Oleksandr
Re: [PATCH v5 03/14] vpci: move lock outside of struct vpci
On Wed, Jan 26, 2022 at 08:40:09AM +, Oleksandr Andrushchenko wrote: > Hello, Roger, Jan! > > On 12.01.22 16:42, Jan Beulich wrote: > > On 11.01.2022 16:17, Roger Pau Monné wrote: > >> On Thu, Nov 25, 2021 at 01:02:40PM +0200, Oleksandr Andrushchenko wrote: > >>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c > >>> index 657697fe3406..ceaac4516ff8 100644 > >>> --- a/xen/drivers/vpci/vpci.c > >>> +++ b/xen/drivers/vpci/vpci.c > >>> @@ -35,12 +35,10 @@ extern vpci_register_init_t *const > >>> __start_vpci_array[]; > >>> extern vpci_register_init_t *const __end_vpci_array[]; > >>> #define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array) > >>> > >>> -void vpci_remove_device(struct pci_dev *pdev) > >>> +static void vpci_remove_device_handlers_locked(struct pci_dev *pdev) > >>> { > >>> -if ( !has_vpci(pdev->domain) ) > >>> -return; > >>> +ASSERT(spin_is_locked(&pdev->vpci_lock)); > >>> > >>> -spin_lock(&pdev->vpci->lock); > >>> while ( !list_empty(&pdev->vpci->handlers) ) > >>> { > >>> struct vpci_register *r = > >>> list_first_entry(&pdev->vpci->handlers, > >>> @@ -50,15 +48,33 @@ void vpci_remove_device(struct pci_dev *pdev) > >>> list_del(&r->node); > >>> xfree(r); > >>> } > >>> -spin_unlock(&pdev->vpci->lock); > >>> +} > >>> + > >>> +void vpci_remove_device_locked(struct pci_dev *pdev) > >> I think this could be static instead, as it's only used by > >> vpci_remove_device and vpci_add_handlers which are local to the > >> file. > This is going to be used outside later on while processing pending mappings, > so I think it is not worth it defining it static here and then removing the > static > key word later on: please see [PATCH v5 04/14] vpci: cancel pending map/unmap > on vpci removal [1] I have some comments there also, which might change the approach you are using. > > Does the splitting out of vpci_remove_device_handlers_locked() belong in > > this patch in the first place? There's no second caller being added, so > > this looks to be an orthogonal adjustment. > I think of it as a preparation for the upcoming code: although the reason for > the > change might not be immediately seen in this patch it is still in line with > what > happens next. Right - it's generally best if the change is done together as the new callers are added. Otherwise it's hard to understand why certain changes are made, and you will likely get asked the same question on next rounds. It's also possible that the code that requires this is changed in further iterations so there's no longer a need for the splitting. Thanks, Roger.
Re: [PATCH v5 03/14] vpci: move lock outside of struct vpci
Hello, Roger, Jan! On 12.01.22 16:42, Jan Beulich wrote: > On 11.01.2022 16:17, Roger Pau Monné wrote: >> On Thu, Nov 25, 2021 at 01:02:40PM +0200, Oleksandr Andrushchenko wrote: >>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c >>> index 657697fe3406..ceaac4516ff8 100644 >>> --- a/xen/drivers/vpci/vpci.c >>> +++ b/xen/drivers/vpci/vpci.c >>> @@ -35,12 +35,10 @@ extern vpci_register_init_t *const __start_vpci_array[]; >>> extern vpci_register_init_t *const __end_vpci_array[]; >>> #define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array) >>> >>> -void vpci_remove_device(struct pci_dev *pdev) >>> +static void vpci_remove_device_handlers_locked(struct pci_dev *pdev) >>> { >>> -if ( !has_vpci(pdev->domain) ) >>> -return; >>> +ASSERT(spin_is_locked(&pdev->vpci_lock)); >>> >>> -spin_lock(&pdev->vpci->lock); >>> while ( !list_empty(&pdev->vpci->handlers) ) >>> { >>> struct vpci_register *r = list_first_entry(&pdev->vpci->handlers, >>> @@ -50,15 +48,33 @@ void vpci_remove_device(struct pci_dev *pdev) >>> list_del(&r->node); >>> xfree(r); >>> } >>> -spin_unlock(&pdev->vpci->lock); >>> +} >>> + >>> +void vpci_remove_device_locked(struct pci_dev *pdev) >> I think this could be static instead, as it's only used by >> vpci_remove_device and vpci_add_handlers which are local to the >> file. This is going to be used outside later on while processing pending mappings, so I think it is not worth it defining it static here and then removing the static key word later on: please see [PATCH v5 04/14] vpci: cancel pending map/unmap on vpci removal [1] > Does the splitting out of vpci_remove_device_handlers_locked() belong in > this patch in the first place? There's no second caller being added, so > this looks to be an orthogonal adjustment. I think of it as a preparation for the upcoming code: although the reason for the change might not be immediately seen in this patch it is still in line with what happens next. So, I would prefer to keep the change as is: anyways the whole series should probably be committed as a single piece of work, so it won't look inconsistent then Thank you, Oleksandr > > Jan > [1] https://patchwork.kernel.org/project/xen-devel/patch/20211125110251.2877218-5-andr2...@gmail.com/
Re: [PATCH v5 03/14] vpci: move lock outside of struct vpci
On Wed, Jan 12, 2022 at 04:52:51PM +0100, Jan Beulich wrote: > On 12.01.2022 16:42, Roger Pau Monné wrote: > > On Wed, Jan 12, 2022 at 03:57:36PM +0100, Jan Beulich wrote: > >> On 25.11.2021 12:02, Oleksandr Andrushchenko wrote: > >>> @@ -379,7 +396,6 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, > >>> unsigned int size) > >>> > >>> data = merge_result(data, tmp_data, size - data_offset, > >>> data_offset); > >>> } > >>> -spin_unlock(&pdev->vpci->lock); > >>> > >>> return data & (0x >> (32 - 8 * size)); > >>> } > >> > >> Here and ... > >> > >>> @@ -475,13 +498,12 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, > >>> unsigned int size, > >>> break; > >>> ASSERT(data_offset < size); > >>> } > >>> +spin_unlock(&pdev->vpci_lock); > >>> > >>> if ( data_offset < size ) > >>> /* Tailing gap, write the remaining. */ > >>> vpci_write_hw(sbdf, reg + data_offset, size - data_offset, > >>>data >> (data_offset * 8)); > >>> - > >>> -spin_unlock(&pdev->vpci->lock); > >>> } > >> > >> ... even more so here I'm not sure of the correctness of the moving > >> you do: While pdev->vpci indeed doesn't get accessed, I wonder > >> whether there wasn't an intention to avoid racing calls to > >> vpci_{read,write}_hw() this way. In any event I think such movement > >> would need justification in the description. > > > > I agree about the need for justification in the commit message, or > > even better this being split into a pre-patch, as it's not related to > > the lock switching done here. > > > > I do think this is fine however, as racing calls to > > vpci_{read,write}_hw() shouldn't be a problem. Those are just wrappers > > around pci_conf_{read,write} functions, and the required locking (in > > case of using the IO ports) is already taken care in > > pci_conf_{read,write}. > > IOW you consider it acceptable for a guest (really: Dom0) read racing > a write to read back only part of what was written (so far)? I would > think individual multi-byte reads and writes should appear atomic to > the guest. We split 64bit writes into two 32bit ones without taking the lock for the whole duration of the access, so it's already possible to see a partially updated state as a result of a 64bit write. I'm going over the PCI(e) spec but I don't seem to find anything about whether the ECAM is allowed to split memory transactions into multiple Configuration Requests, and whether those could then interleave with requests from a different CPU. Thanks, Roger.
Re: [PATCH v5 03/14] vpci: move lock outside of struct vpci
On 12.01.2022 16:42, Roger Pau Monné wrote: > On Wed, Jan 12, 2022 at 03:57:36PM +0100, Jan Beulich wrote: >> On 25.11.2021 12:02, Oleksandr Andrushchenko wrote: >>> @@ -379,7 +396,6 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, >>> unsigned int size) >>> >>> data = merge_result(data, tmp_data, size - data_offset, >>> data_offset); >>> } >>> -spin_unlock(&pdev->vpci->lock); >>> >>> return data & (0x >> (32 - 8 * size)); >>> } >> >> Here and ... >> >>> @@ -475,13 +498,12 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, >>> unsigned int size, >>> break; >>> ASSERT(data_offset < size); >>> } >>> +spin_unlock(&pdev->vpci_lock); >>> >>> if ( data_offset < size ) >>> /* Tailing gap, write the remaining. */ >>> vpci_write_hw(sbdf, reg + data_offset, size - data_offset, >>>data >> (data_offset * 8)); >>> - >>> -spin_unlock(&pdev->vpci->lock); >>> } >> >> ... even more so here I'm not sure of the correctness of the moving >> you do: While pdev->vpci indeed doesn't get accessed, I wonder >> whether there wasn't an intention to avoid racing calls to >> vpci_{read,write}_hw() this way. In any event I think such movement >> would need justification in the description. > > I agree about the need for justification in the commit message, or > even better this being split into a pre-patch, as it's not related to > the lock switching done here. > > I do think this is fine however, as racing calls to > vpci_{read,write}_hw() shouldn't be a problem. Those are just wrappers > around pci_conf_{read,write} functions, and the required locking (in > case of using the IO ports) is already taken care in > pci_conf_{read,write}. IOW you consider it acceptable for a guest (really: Dom0) read racing a write to read back only part of what was written (so far)? I would think individual multi-byte reads and writes should appear atomic to the guest. Jan
Re: [PATCH v5 03/14] vpci: move lock outside of struct vpci
On Wed, Jan 12, 2022 at 03:57:36PM +0100, Jan Beulich wrote: > On 25.11.2021 12:02, Oleksandr Andrushchenko wrote: > > @@ -379,7 +396,6 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, > > unsigned int size) > > > > data = merge_result(data, tmp_data, size - data_offset, > > data_offset); > > } > > -spin_unlock(&pdev->vpci->lock); > > > > return data & (0x >> (32 - 8 * size)); > > } > > Here and ... > > > @@ -475,13 +498,12 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, > > unsigned int size, > > break; > > ASSERT(data_offset < size); > > } > > +spin_unlock(&pdev->vpci_lock); > > > > if ( data_offset < size ) > > /* Tailing gap, write the remaining. */ > > vpci_write_hw(sbdf, reg + data_offset, size - data_offset, > >data >> (data_offset * 8)); > > - > > -spin_unlock(&pdev->vpci->lock); > > } > > ... even more so here I'm not sure of the correctness of the moving > you do: While pdev->vpci indeed doesn't get accessed, I wonder > whether there wasn't an intention to avoid racing calls to > vpci_{read,write}_hw() this way. In any event I think such movement > would need justification in the description. I agree about the need for justification in the commit message, or even better this being split into a pre-patch, as it's not related to the lock switching done here. I do think this is fine however, as racing calls to vpci_{read,write}_hw() shouldn't be a problem. Those are just wrappers around pci_conf_{read,write} functions, and the required locking (in case of using the IO ports) is already taken care in pci_conf_{read,write}. Thanks, Roger.
Re: [PATCH v5 03/14] vpci: move lock outside of struct vpci
On 25.11.2021 12:02, Oleksandr Andrushchenko wrote: > @@ -68,12 +84,13 @@ int vpci_add_handlers(struct pci_dev *pdev) > /* We should not get here twice for the same device. */ > ASSERT(!pdev->vpci); > > -pdev->vpci = xzalloc(struct vpci); > -if ( !pdev->vpci ) > +vpci = xzalloc(struct vpci); > +if ( !vpci ) > return -ENOMEM; > > +spin_lock(&pdev->vpci_lock); > +pdev->vpci = vpci; > INIT_LIST_HEAD(&pdev->vpci->handlers); > -spin_lock_init(&pdev->vpci->lock); INIT_LIST_HEAD() can occur ahead of taking the lock, and can also act on &vpci->handlers rather than &pdev->vpci->handlers. > for ( i = 0; i < NUM_VPCI_INIT; i++ ) > { > @@ -83,7 +100,8 @@ int vpci_add_handlers(struct pci_dev *pdev) > } This loop wants to live in the locked region because you need to install vpci into pdev->vpci up front, afaict. I wonder whether that couldn't be relaxed, but perhaps that's an improvement that can be thought about later. The main reason I'm looking at this closely is because from the patch title I didn't expect new locking regions to be introduced right here; instead I did expect strictly a mechanical conversion. > @@ -152,8 +170,6 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t > *read_handler, > r->offset = offset; > r->private = data; > > -spin_lock(&vpci->lock); >From the description I can't deduce why this lock is fine to go away now, i.e. that all call sites have the lock now acquire earlier. Therefore I'd expect at least an assertion to be left here ... > @@ -183,7 +197,6 @@ int vpci_remove_register(struct vpci *vpci, unsigned int > offset, > const struct vpci_register r = { .offset = offset, .size = size }; > struct vpci_register *rm; > > -spin_lock(&vpci->lock); ... and here. > @@ -370,6 +386,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, > unsigned int size) > break; > ASSERT(data_offset < size); > } > +spin_unlock(&pdev->vpci_lock); > > if ( data_offset < size ) > { > @@ -379,7 +396,6 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, > unsigned int size) > > data = merge_result(data, tmp_data, size - data_offset, data_offset); > } > -spin_unlock(&pdev->vpci->lock); > > return data & (0x >> (32 - 8 * size)); > } Here and ... > @@ -475,13 +498,12 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, > unsigned int size, > break; > ASSERT(data_offset < size); > } > +spin_unlock(&pdev->vpci_lock); > > if ( data_offset < size ) > /* Tailing gap, write the remaining. */ > vpci_write_hw(sbdf, reg + data_offset, size - data_offset, >data >> (data_offset * 8)); > - > -spin_unlock(&pdev->vpci->lock); > } ... even more so here I'm not sure of the correctness of the moving you do: While pdev->vpci indeed doesn't get accessed, I wonder whether there wasn't an intention to avoid racing calls to vpci_{read,write}_hw() this way. In any event I think such movement would need justification in the description. Jan
Re: [PATCH v5 03/14] vpci: move lock outside of struct vpci
On 11.01.2022 16:17, Roger Pau Monné wrote: > On Thu, Nov 25, 2021 at 01:02:40PM +0200, Oleksandr Andrushchenko wrote: >> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c >> index 657697fe3406..ceaac4516ff8 100644 >> --- a/xen/drivers/vpci/vpci.c >> +++ b/xen/drivers/vpci/vpci.c >> @@ -35,12 +35,10 @@ extern vpci_register_init_t *const __start_vpci_array[]; >> extern vpci_register_init_t *const __end_vpci_array[]; >> #define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array) >> >> -void vpci_remove_device(struct pci_dev *pdev) >> +static void vpci_remove_device_handlers_locked(struct pci_dev *pdev) >> { >> -if ( !has_vpci(pdev->domain) ) >> -return; >> +ASSERT(spin_is_locked(&pdev->vpci_lock)); >> >> -spin_lock(&pdev->vpci->lock); >> while ( !list_empty(&pdev->vpci->handlers) ) >> { >> struct vpci_register *r = list_first_entry(&pdev->vpci->handlers, >> @@ -50,15 +48,33 @@ void vpci_remove_device(struct pci_dev *pdev) >> list_del(&r->node); >> xfree(r); >> } >> -spin_unlock(&pdev->vpci->lock); >> +} >> + >> +void vpci_remove_device_locked(struct pci_dev *pdev) > > I think this could be static instead, as it's only used by > vpci_remove_device and vpci_add_handlers which are local to the > file. Does the splitting out of vpci_remove_device_handlers_locked() belong in this patch in the first place? There's no second caller being added, so this looks to be an orthogonal adjustment. Jan
Re: [PATCH v5 03/14] vpci: move lock outside of struct vpci
On Thu, Nov 25, 2021 at 01:02:40PM +0200, Oleksandr Andrushchenko wrote: > diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c > index 657697fe3406..ceaac4516ff8 100644 > --- a/xen/drivers/vpci/vpci.c > +++ b/xen/drivers/vpci/vpci.c > @@ -35,12 +35,10 @@ extern vpci_register_init_t *const __start_vpci_array[]; > extern vpci_register_init_t *const __end_vpci_array[]; > #define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array) > > -void vpci_remove_device(struct pci_dev *pdev) > +static void vpci_remove_device_handlers_locked(struct pci_dev *pdev) > { > -if ( !has_vpci(pdev->domain) ) > -return; > +ASSERT(spin_is_locked(&pdev->vpci_lock)); > > -spin_lock(&pdev->vpci->lock); > while ( !list_empty(&pdev->vpci->handlers) ) > { > struct vpci_register *r = list_first_entry(&pdev->vpci->handlers, > @@ -50,15 +48,33 @@ void vpci_remove_device(struct pci_dev *pdev) > list_del(&r->node); > xfree(r); > } > -spin_unlock(&pdev->vpci->lock); > +} > + > +void vpci_remove_device_locked(struct pci_dev *pdev) I think this could be static instead, as it's only used by vpci_remove_device and vpci_add_handlers which are local to the file. Thanks, Roger.
[PATCH v5 03/14] vpci: move lock outside of struct vpci
From: Roger Pau Monne This way the lock can be used to check whether vpci is present, and removal can be performed while holding the lock, in order to make sure there are no accesses to the contents of the vpci struct. Previously removal could race with vpci_read for example, since the lock was dropped prior to freeing pdev->vpci. Signed-off-by: Roger Pau Monné Signed-off-by: Oleksandr Andrushchenko --- Cc: Andrew Cooper Cc: Ian Jackson Cc: Jan Beulich Cc: Julien Grall Cc: Stefano Stabellini --- New in v5 of this series: this is an updated version of the patch published at https://lore.kernel.org/xen-devel/20180717094830.54806-2-roger@citrix.com/ Changes since v2: - fixed pdev->vpci = xzalloc(struct vpci); under spin_lock (Jan) Changes since v1: - Assert that vpci_lock is locked in vpci_remove_device_locked. - Remove double newline. - Shrink critical section in vpci_{read/write}. --- tools/tests/vpci/emul.h | 5 ++- tools/tests/vpci/main.c | 4 +-- xen/arch/x86/hvm/vmsi.c | 8 ++--- xen/drivers/passthrough/pci.c | 1 + xen/drivers/vpci/header.c | 21 +++ xen/drivers/vpci/msi.c| 11 -- xen/drivers/vpci/msix.c | 8 ++--- xen/drivers/vpci/vpci.c | 68 +++ xen/include/xen/pci.h | 1 + xen/include/xen/vpci.h| 5 +-- 10 files changed, 85 insertions(+), 47 deletions(-) diff --git a/tools/tests/vpci/emul.h b/tools/tests/vpci/emul.h index 2e1d3057c9d8..d018fb5eef21 100644 --- a/tools/tests/vpci/emul.h +++ b/tools/tests/vpci/emul.h @@ -44,6 +44,7 @@ struct domain { }; struct pci_dev { +bool vpci_lock; struct vpci *vpci; }; @@ -53,10 +54,8 @@ struct vcpu }; extern const struct vcpu *current; -extern const struct pci_dev test_pdev; +extern struct pci_dev test_pdev; -typedef bool spinlock_t; -#define spin_lock_init(l) (*(l) = false) #define spin_lock(l) (*(l) = true) #define spin_unlock(l) (*(l) = false) diff --git a/tools/tests/vpci/main.c b/tools/tests/vpci/main.c index b9a0a6006bb9..26c95b08b6b1 100644 --- a/tools/tests/vpci/main.c +++ b/tools/tests/vpci/main.c @@ -23,7 +23,8 @@ static struct vpci vpci; const static struct domain d; -const struct pci_dev test_pdev = { +struct pci_dev test_pdev = { +.vpci_lock = false, .vpci = &vpci, }; @@ -158,7 +159,6 @@ main(int argc, char **argv) int rc; INIT_LIST_HEAD(&vpci.handlers); -spin_lock_init(&vpci.lock); VPCI_ADD_REG(vpci_read32, vpci_write32, 0, 4, r0); VPCI_READ_CHECK(0, 4, r0); diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c index 13e2a190b439..1f7a37f78264 100644 --- a/xen/arch/x86/hvm/vmsi.c +++ b/xen/arch/x86/hvm/vmsi.c @@ -910,14 +910,14 @@ int vpci_msix_arch_print(const struct vpci_msix *msix) { struct pci_dev *pdev = msix->pdev; -spin_unlock(&msix->pdev->vpci->lock); +spin_unlock(&msix->pdev->vpci_lock); process_pending_softirqs(); /* NB: we assume that pdev cannot go away for an alive domain. */ -if ( !pdev->vpci || !spin_trylock(&pdev->vpci->lock) ) +if ( !spin_trylock(&pdev->vpci_lock) ) return -EBUSY; -if ( pdev->vpci->msix != msix ) +if ( !pdev->vpci || pdev->vpci->msix != msix ) { -spin_unlock(&pdev->vpci->lock); +spin_unlock(&pdev->vpci_lock); return -EAGAIN; } } diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index a9d31293ac09..286808b25e65 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -328,6 +328,7 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn) *((u8*) &pdev->bus) = bus; *((u8*) &pdev->devfn) = devfn; pdev->domain = NULL; +spin_lock_init(&pdev->vpci_lock); arch_pci_init_pdev(pdev); diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c index 40ff79c33f8f..bd23c0274d48 100644 --- a/xen/drivers/vpci/header.c +++ b/xen/drivers/vpci/header.c @@ -142,12 +142,13 @@ bool vpci_process_pending(struct vcpu *v) if ( rc == -ERESTART ) return true; -spin_lock(&v->vpci.pdev->vpci->lock); -/* Disable memory decoding unconditionally on failure. */ -modify_decoding(v->vpci.pdev, -rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : v->vpci.cmd, -!rc && v->vpci.rom_only); -spin_unlock(&v->vpci.pdev->vpci->lock); +spin_lock(&v->vpci.pdev->vpci_lock); +if ( v->vpci.pdev->vpci ) +/* Disable memory decoding unconditionally on failure. */ +modify_decoding(v->vpci.pdev, +rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : v->vpci.cmd, +!rc && v->vpci.rom_only); +spin_unlock(&v->vpci.pdev->vpci_lock); rangeset_destroy(v-