* [PATCH v3 1/2] intel_iommu: Take the bql before registering a new address space @ 2025-04-23 5:38 CLEMENT MATHIEU--DRIF 2025-04-23 5:38 ` [PATCH v3 2/2] intel_iommu: Use BQL_LOCK_GUARD to manage cleanup automatically CLEMENT MATHIEU--DRIF 2025-04-23 6:00 ` [PATCH v3 1/2] intel_iommu: Take the bql before registering a new address space Michael S. Tsirkin 0 siblings, 2 replies; 6+ messages in thread From: CLEMENT MATHIEU--DRIF @ 2025-04-23 5:38 UTC (permalink / raw) To: qemu-devel@nongnu.org Cc: jasowang@redhat.com, zhenzhong.duan@intel.com, kevin.tian@intel.com, yi.l.liu@intel.com, peterx@redhat.com, mst@redhat.com, CLEMENT MATHIEU--DRIF Address space creation might end up being called without holding the bql as it is exposed through the IOMMU ops. Signed-off-by: Clement Mathieu--Drif <clement.mathieu--drif@eviden.com> --- hw/i386/intel_iommu.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index dffd7ee885..cc8c9857e1 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -4238,6 +4238,12 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, vtd_dev_as->context_cache_entry.context_cache_gen = 0; vtd_dev_as->iova_tree = iova_tree_new(); + /* + * memory_region_add_subregion_overlap requires the bql, + * make sure we own it. + */ + BQL_LOCK_GUARD(); + memory_region_init(&vtd_dev_as->root, OBJECT(s), name, UINT64_MAX); address_space_init(&vtd_dev_as->as, &vtd_dev_as->root, "vtd-root"); -- 2.49.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] intel_iommu: Use BQL_LOCK_GUARD to manage cleanup automatically 2025-04-23 5:38 [PATCH v3 1/2] intel_iommu: Take the bql before registering a new address space CLEMENT MATHIEU--DRIF @ 2025-04-23 5:38 ` CLEMENT MATHIEU--DRIF 2025-04-23 6:00 ` [PATCH v3 1/2] intel_iommu: Take the bql before registering a new address space Michael S. Tsirkin 1 sibling, 0 replies; 6+ messages in thread From: CLEMENT MATHIEU--DRIF @ 2025-04-23 5:38 UTC (permalink / raw) To: qemu-devel@nongnu.org Cc: jasowang@redhat.com, zhenzhong.duan@intel.com, kevin.tian@intel.com, yi.l.liu@intel.com, peterx@redhat.com, mst@redhat.com, CLEMENT MATHIEU--DRIF vtd_switch_address_space needs to take the BQL if not already held. Use BQL_LOCK_GUARD to make the iommu implementation more consistent. Signed-off-by: Clement Mathieu--Drif <clement.mathieu--drif@eviden.com> --- hw/i386/intel_iommu.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index cc8c9857e1..b7855f4b87 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -1728,8 +1728,6 @@ static bool vtd_as_pt_enabled(VTDAddressSpace *as) static bool vtd_switch_address_space(VTDAddressSpace *as) { bool use_iommu, pt; - /* Whether we need to take the BQL on our own */ - bool take_bql = !bql_locked(); assert(as); @@ -1746,9 +1744,7 @@ static bool vtd_switch_address_space(VTDAddressSpace *as) * from vtd_pt_enable_fast_path(). However the memory APIs need * it. We'd better make sure we have had it already, or, take it. */ - if (take_bql) { - bql_lock(); - } + BQL_LOCK_GUARD(); /* Turn off first then on the other */ if (use_iommu) { @@ -1801,10 +1797,6 @@ static bool vtd_switch_address_space(VTDAddressSpace *as) memory_region_set_enabled(&as->iommu_ir_fault, false); } - if (take_bql) { - bql_unlock(); - } - return use_iommu; } -- 2.49.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] intel_iommu: Take the bql before registering a new address space 2025-04-23 5:38 [PATCH v3 1/2] intel_iommu: Take the bql before registering a new address space CLEMENT MATHIEU--DRIF 2025-04-23 5:38 ` [PATCH v3 2/2] intel_iommu: Use BQL_LOCK_GUARD to manage cleanup automatically CLEMENT MATHIEU--DRIF @ 2025-04-23 6:00 ` Michael S. Tsirkin 2025-04-23 9:15 ` CLEMENT MATHIEU--DRIF 1 sibling, 1 reply; 6+ messages in thread From: Michael S. Tsirkin @ 2025-04-23 6:00 UTC (permalink / raw) To: CLEMENT MATHIEU--DRIF Cc: qemu-devel@nongnu.org, jasowang@redhat.com, zhenzhong.duan@intel.com, kevin.tian@intel.com, yi.l.liu@intel.com, peterx@redhat.com On Wed, Apr 23, 2025 at 05:38:20AM +0000, CLEMENT MATHIEU--DRIF wrote: > Address space creation might end up being called without holding the > bql as it is exposed through the IOMMU ops. > > Signed-off-by: Clement Mathieu--Drif <clement.mathieu--drif@eviden.com> > --- > hw/i386/intel_iommu.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index dffd7ee885..cc8c9857e1 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -4238,6 +4238,12 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, > vtd_dev_as->context_cache_entry.context_cache_gen = 0; > vtd_dev_as->iova_tree = iova_tree_new(); > > + /* > + * memory_region_add_subregion_overlap requires the bql, > + * make sure we own it. > + */ > + BQL_LOCK_GUARD(); > + > memory_region_init(&vtd_dev_as->root, OBJECT(s), name, UINT64_MAX); > address_space_init(&vtd_dev_as->as, &vtd_dev_as->root, "vtd-root"); Does not look like this addresses all races here: https://lore.kernel.org/all/8062d868-469f-4c1d-a071-099b8e18857c@redhat.com while this can be a separate patch on top, I'd rather we just address everything in a single patchset. > -- > 2.49.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] intel_iommu: Take the bql before registering a new address space 2025-04-23 6:00 ` [PATCH v3 1/2] intel_iommu: Take the bql before registering a new address space Michael S. Tsirkin @ 2025-04-23 9:15 ` CLEMENT MATHIEU--DRIF 2025-04-23 10:06 ` Michael S. Tsirkin 0 siblings, 1 reply; 6+ messages in thread From: CLEMENT MATHIEU--DRIF @ 2025-04-23 9:15 UTC (permalink / raw) To: Michael S. Tsirkin Cc: qemu-devel@nongnu.org, jasowang@redhat.com, zhenzhong.duan@intel.com, kevin.tian@intel.com, yi.l.liu@intel.com, peterx@redhat.com On 23/04/2025 8:00 am, Michael S. Tsirkin wrote: > Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe. > > > On Wed, Apr 23, 2025 at 05:38:20AM +0000, CLEMENT MATHIEU--DRIF wrote: >> Address space creation might end up being called without holding the >> bql as it is exposed through the IOMMU ops. >> >> Signed-off-by: Clement Mathieu--Drif <clement.mathieu--drif@eviden.com> >> --- >> hw/i386/intel_iommu.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >> index dffd7ee885..cc8c9857e1 100644 >> --- a/hw/i386/intel_iommu.c >> +++ b/hw/i386/intel_iommu.c >> @@ -4238,6 +4238,12 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, >> vtd_dev_as->context_cache_entry.context_cache_gen = 0; >> vtd_dev_as->iova_tree = iova_tree_new(); >> >> + /* >> + * memory_region_add_subregion_overlap requires the bql, >> + * make sure we own it. >> + */ >> + BQL_LOCK_GUARD(); >> + >> memory_region_init(&vtd_dev_as->root, OBJECT(s), name, UINT64_MAX); >> address_space_init(&vtd_dev_as->as, &vtd_dev_as->root, "vtd-root"); > > Does not look like this addresses all races here: > https://lore.kernel.org/all/8062d868-469f-4c1d-a071-099b8e18857c@redhat.com > > > while this can be a separate patch on top, I'd rather we just > address everything in a single patchset. Hi Michael, We only aim to fix the potential crash here. I saw Paolo's response and I know the race exists. I will send a patch set to fix it soon but are you sure both fixes must be in the same series? I think the nature is different. cmd > > >> -- >> 2.49.0 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] intel_iommu: Take the bql before registering a new address space 2025-04-23 9:15 ` CLEMENT MATHIEU--DRIF @ 2025-04-23 10:06 ` Michael S. Tsirkin 2025-04-23 10:09 ` CLEMENT MATHIEU--DRIF 0 siblings, 1 reply; 6+ messages in thread From: Michael S. Tsirkin @ 2025-04-23 10:06 UTC (permalink / raw) To: CLEMENT MATHIEU--DRIF Cc: qemu-devel@nongnu.org, jasowang@redhat.com, zhenzhong.duan@intel.com, kevin.tian@intel.com, yi.l.liu@intel.com, peterx@redhat.com On Wed, Apr 23, 2025 at 09:15:36AM +0000, CLEMENT MATHIEU--DRIF wrote: > > > On 23/04/2025 8:00 am, Michael S. Tsirkin wrote: > > Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe. > > > > > > On Wed, Apr 23, 2025 at 05:38:20AM +0000, CLEMENT MATHIEU--DRIF wrote: > >> Address space creation might end up being called without holding the > >> bql as it is exposed through the IOMMU ops. > >> > >> Signed-off-by: Clement Mathieu--Drif <clement.mathieu--drif@eviden.com> > >> --- > >> hw/i386/intel_iommu.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > >> index dffd7ee885..cc8c9857e1 100644 > >> --- a/hw/i386/intel_iommu.c > >> +++ b/hw/i386/intel_iommu.c > >> @@ -4238,6 +4238,12 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, > >> vtd_dev_as->context_cache_entry.context_cache_gen = 0; > >> vtd_dev_as->iova_tree = iova_tree_new(); > >> > >> + /* > >> + * memory_region_add_subregion_overlap requires the bql, > >> + * make sure we own it. > >> + */ > >> + BQL_LOCK_GUARD(); > >> + > >> memory_region_init(&vtd_dev_as->root, OBJECT(s), name, UINT64_MAX); > >> address_space_init(&vtd_dev_as->as, &vtd_dev_as->root, "vtd-root"); > > > > Does not look like this addresses all races here: > > https://lore.kernel.org/all/8062d868-469f-4c1d-a071-099b8e18857c@redhat.com > > > > > > while this can be a separate patch on top, I'd rather we just > > address everything in a single patchset. > > Hi Michael, > > We only aim to fix the potential crash here. > I saw Paolo's response and I know the race exists. I will send a patch > set to fix it soon but are you sure both fixes must be in the same > series? I think the nature is different. > > cmd If you have two races in the same function, fixing one can easily make another one occur more. Let's just fix it all please, I don't see any rush to apply a partial fix. -- MST ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] intel_iommu: Take the bql before registering a new address space 2025-04-23 10:06 ` Michael S. Tsirkin @ 2025-04-23 10:09 ` CLEMENT MATHIEU--DRIF 0 siblings, 0 replies; 6+ messages in thread From: CLEMENT MATHIEU--DRIF @ 2025-04-23 10:09 UTC (permalink / raw) To: Michael S. Tsirkin Cc: qemu-devel@nongnu.org, jasowang@redhat.com, zhenzhong.duan@intel.com, kevin.tian@intel.com, yi.l.liu@intel.com, peterx@redhat.com On 23/04/2025 12:06 pm, Michael S. Tsirkin wrote: > Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe. > > > On Wed, Apr 23, 2025 at 09:15:36AM +0000, CLEMENT MATHIEU--DRIF wrote: >> >> >> On 23/04/2025 8:00 am, Michael S. Tsirkin wrote: >>> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe. >>> >>> >>> On Wed, Apr 23, 2025 at 05:38:20AM +0000, CLEMENT MATHIEU--DRIF wrote: >>>> Address space creation might end up being called without holding the >>>> bql as it is exposed through the IOMMU ops. >>>> >>>> Signed-off-by: Clement Mathieu--Drif <clement.mathieu--drif@eviden.com> >>>> --- >>>> hw/i386/intel_iommu.c | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>>> index dffd7ee885..cc8c9857e1 100644 >>>> --- a/hw/i386/intel_iommu.c >>>> +++ b/hw/i386/intel_iommu.c >>>> @@ -4238,6 +4238,12 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, >>>> vtd_dev_as->context_cache_entry.context_cache_gen = 0; >>>> vtd_dev_as->iova_tree = iova_tree_new(); >>>> >>>> + /* >>>> + * memory_region_add_subregion_overlap requires the bql, >>>> + * make sure we own it. >>>> + */ >>>> + BQL_LOCK_GUARD(); >>>> + >>>> memory_region_init(&vtd_dev_as->root, OBJECT(s), name, UINT64_MAX); >>>> address_space_init(&vtd_dev_as->as, &vtd_dev_as->root, "vtd-root"); >>> >>> Does not look like this addresses all races here: >>> https://lore.kernel.org/all/8062d868-469f-4c1d-a071-099b8e18857c@redhat.com >>> >>> >>> while this can be a separate patch on top, I'd rather we just >>> address everything in a single patchset. >> >> Hi Michael, >> >> We only aim to fix the potential crash here. >> I saw Paolo's response and I know the race exists. I will send a patch >> set to fix it soon but are you sure both fixes must be in the same >> series? I think the nature is different. >> >> cmd > > If you have two races in the same function, fixing one can easily > make another one occur more. Let's just fix it all please, > I don't see any rush to apply a partial fix. > Fine, will do! > -- > MST > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-04-23 10:09 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-04-23 5:38 [PATCH v3 1/2] intel_iommu: Take the bql before registering a new address space CLEMENT MATHIEU--DRIF 2025-04-23 5:38 ` [PATCH v3 2/2] intel_iommu: Use BQL_LOCK_GUARD to manage cleanup automatically CLEMENT MATHIEU--DRIF 2025-04-23 6:00 ` [PATCH v3 1/2] intel_iommu: Take the bql before registering a new address space Michael S. Tsirkin 2025-04-23 9:15 ` CLEMENT MATHIEU--DRIF 2025-04-23 10:06 ` Michael S. Tsirkin 2025-04-23 10:09 ` CLEMENT MATHIEU--DRIF
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.