On 10.12.2021 14:51, Roger Pau Monné wrote: > On Fri, Sep 24, 2021 at 11:48:21AM +0200, Jan Beulich wrote: >> For vendor specific code to support superpages we need to be able to >> deal with a superpage mapping replacing an intermediate page table (or >> hierarchy thereof). Consequently an iommu_alloc_pgtable() counterpart is >> needed to free individual page tables while a domain is still alive. >> Since the freeing needs to be deferred until after a suitable IOTLB >> flush was performed, released page tables get queued for processing by a >> tasklet. >> >> Signed-off-by: Jan Beulich <[email protected]> >> --- >> I was considering whether to use a softirq-taklet instead. This would >> have the benefit of avoiding extra scheduling operations, but come with >> the risk of the freeing happening prematurely because of a >> process_pending_softirqs() somewhere. > > The main one that comes to mind would be the debug keys and it's usage > of process_pending_softirqs that could interfere with iommu unmaps, so > I guess if only for that reason it's best to run in idle vcpu context.
IOW you support the choice made. >> --- a/xen/drivers/passthrough/x86/iommu.c >> +++ b/xen/drivers/passthrough/x86/iommu.c >> @@ -12,6 +12,7 @@ >> * this program; If not, see <http://www.gnu.org/licenses/>. >> */ >> >> +#include <xen/cpu.h> >> #include <xen/sched.h> >> #include <xen/iommu.h> >> #include <xen/paging.h> >> @@ -463,6 +464,85 @@ struct page_info *iommu_alloc_pgtable(st >> return pg; >> } >> >> +/* >> + * Intermediate page tables which get replaced by large pages may only be >> + * freed after a suitable IOTLB flush. Hence such pages get queued on a >> + * per-CPU list, with a per-CPU tasklet processing the list on the >> assumption >> + * that the necessary IOTLB flush will have occurred by the time tasklets >> get >> + * to run. (List and tasklet being per-CPU has the benefit of accesses not >> + * requiring any locking.) >> + */ >> +static DEFINE_PER_CPU(struct page_list_head, free_pgt_list); >> +static DEFINE_PER_CPU(struct tasklet, free_pgt_tasklet); >> + >> +static void free_queued_pgtables(void *arg) >> +{ >> + struct page_list_head *list = arg; >> + struct page_info *pg; >> + >> + while ( (pg = page_list_remove_head(list)) ) >> + free_domheap_page(pg); > > Should you add a preempt check here to yield and schedule the tasklet > again, in order to be able to process any pending work? I did ask myself this question, yes, but ... > Maybe just calling process_pending_softirqs would be enough? ... I think I didn't consider this as a possible simpler variant (compared to re-scheduling the tasklet). Let me add such - I agree that this should be sufficient. Jan
