> -----Original Message----- > From: Jan Beulich <[email protected]> > Sent: 15 January 2021 11:07 > To: Julien Grall <[email protected]>; Paul Durrant <[email protected]> > Cc: [email protected]; Julien Grall <[email protected]>; > [email protected]; Andrew > Cooper <[email protected]> > Subject: Re: [PATCH for-4.15 2/4] xen/iommu: x86: Free the IOMMU page-tables > with the pgtables.lock > held > > On 14.01.2021 20:19, Julien Grall wrote: > > > > > > On 23/12/2020 14:01, Julien Grall wrote: > >> Hi Jan, > >> > >> On 23/12/2020 13:48, Jan Beulich wrote: > >>> On 22.12.2020 16:43, Julien Grall wrote: > >>>> From: Julien Grall <[email protected]> > >>>> > >>>> The pgtables.lock is protecting access to the page list pgtables.list. > >>>> However, iommu_free_pgtables() will not held it. I guess it was assumed > >>>> that page-tables cannot be allocated while the domain is dying. > >>>> > >>>> Unfortunately, there is no guarantee that iommu_map() will not be > >>>> called while a domain is dying (it looks like to be possible from > >>>> XEN_DOMCTL_memory_mapping). > >>> > >>> I'd rather disallow any new allocations for a dying domain, not > >>> the least because ... > >> > >> Patch #4 will disallow such allocation. However... > > > > It turns out that I can't disallow it because there is at least one call > > of iommu_map() while an HVM domain is destroyed: > > > > (XEN) [<ffff82d04026e399>] R iommu_map+0xf2/0x171 > > (XEN) [<ffff82d04026e43e>] F iommu_legacy_map+0x26/0x63 > > (XEN) [<ffff82d040302a42>] F > > arch/x86/mm/p2m-ept.c#ept_set_entry+0x6b2/0x730 > > (XEN) [<ffff82d0402f761d>] F p2m_set_entry+0x91/0x128 > > (XEN) [<ffff82d0402f8643>] F guest_physmap_add_entry+0x3d7/0x4e0 > > (XEN) [<ffff82d0402f87cb>] F guest_physmap_add_page+0x7f/0xfe > > (XEN) [<ffff82d0402ba0a2>] F > > arch/x86/hvm/ioreq.c#hvm_add_ioreq_gfn+0x6f/0x8d > > (XEN) [<ffff82d0402ba0f9>] F > > arch/x86/hvm/ioreq.c#hvm_ioreq_server_disable+0x39/0x4f > > (XEN) [<ffff82d0402bb50e>] F hvm_destroy_all_ioreq_servers+0x6f/0x9b > > (XEN) [<ffff82d0402afc15>] F > > hvm_domain_relinquish_resources+0x3e/0x92 > > (XEN) [<ffff82d04031f278>] F domain_relinquish_resources+0x355/0x372 > > (XEN) [<ffff82d040205dd4>] F domain_kill+0xc7/0x150 > > (XEN) [<ffff82d04023baf0>] F do_domctl+0xba7/0x1a09 > > (XEN) [<ffff82d040312c8f>] F pv_hypercall+0x2f0/0x55f > > (XEN) [<ffff82d040391432>] F lstar_enter+0x112/0x120 > > > > This one resulted to a domain crash rather than a clean destruction. > > A domain crash despite the domain already getting cleaned up is > something we may at least want to consider doing better: There > already is a !d->is_shutting_down conditional printk() there. > What would people think about avoiding the domain_crash() call > in similar ways? (It could even be considered to make > domain_crash() itself behave like this, but such a step may make > it necessary to first audit all use sites.) > > > I would still like to disallow page-table allocation, so I am think to > > ignore any request in iommu_map() if the domain is dying. > > Ignoring requests there seems fragile to me. Paul - what are your > thoughts about bailing early from hvm_add_ioreq_gfn() when the > domain is dying? >
I think that's ok. Really, the only reason for putting the pages back in the P2M is to allow migration to work so if the domain is dying then we don't really care do we? Paul > Jan
