[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] do not p2m_invalidate_root when iommu_use_hap_pt
On Tue, 3 Aug 2021, Julien Grall wrote: > Hi, > > Title: How about: > > "xen/arm: Do not invalidate the P2M when the PT is shared with the IOMMU" OK > On 03/08/2021 22:37, Stefano Stabellini wrote: > > Set/Way flushes never work correctly in a virtualized environment. > > > > Our current implementation is based on clearing the valid bit in the p2m > > pagetable to track guest memory accesses. This technique doesn't work > > when the IOMMU is enabled for the domain and the pagetable is shared > > between IOMMU and MMU because it triggers IOMMU faults. > > > > Specifically, p2m_invalidate_root causes IOMMU faults if > > iommu_use_hap_pt returns true for the domain. > > > > Add a check in vsysreg.c and vcpreg.c: if a set/way instruction is used > > and iommu_use_hap_pt returns true, rather than failing with obscure > > IOMMU faults, inject an undef exception straight away into the guest, > > and print a verbose error message to explain the problem. > > > > Also add an ASSERT in p2m_invalidate_root to make sure we don't > > inadvertently stumble across this problem again in the future. > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx> > > --- > > > > This patch is an improvement over the IOMMU faults. I don't know if we > > want to give users an option to get past these errors for development > > and debugging. > > > > We could add a Xen command line option to make Xen ignore Set/Way > > instructions (do nothing on trap). Or we could add an option to avoid > > trapping Set/Way instructions altogether (remove HCR_TSW). > > > > Both workarounds are obviously not correct and could lead to memory > > corruptions (the former) or bad interference between guests (the latter). > > My answer is similar to when you suggested to not trap the SMCs for all the > domains. Yes, it may allow a domain to boot but such option will not do a > favor to anyone because more weird behavior may happen. > > If there is a will to handle set/way when device is assigned, then we need to > add support for unsharing the page-tables or figure out a different way to > emulate set/way. > > > Either way, we can start with this patch. > > > > diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c > > index caf17174b8..125a9281fc 100644 > > --- a/xen/arch/arm/arm64/vsysreg.c > > +++ b/xen/arch/arm/arm64/vsysreg.c > > @@ -105,6 +105,13 @@ void do_sysreg(struct cpu_user_regs *regs, > > case HSR_SYSREG_DCISW: > > case HSR_SYSREG_DCCSW: > > case HSR_SYSREG_DCCISW: > > + if ( iommu_use_hap_pt(current->domain) ) > > + { > > + gdprintk(XENLOG_ERR, > > + "d%u uses set/way cache flushes with the IOMMU on. It > > cannot work. Replace set/way instructions with dc [ci]vac and retry. > > Injecting exception into the guest now.\n", > > This line would be far too long to print on the serial. I think you want to > add a few newline here. Fair enough but I'll try to keep most info on the same line because otherwise with a dom0less boot it can get confusing. I suggest: gprintk(XENLOG_ERR, "uses set/way cache flushes with the IOMMU on. It cannot work. Replace them with dc [ci]vac and retry.\n" "Injecting exception into the guest now.\n"); > > + current->domain->domain_id); > > Please use %pd. I realized I should use gprintk and not gdprintk, and also that either way the domain id gets printed automatically, so there is no need to add d%u. > > + return inject_undef_exception(regs, hsr); > > + } > > I would prefer if the undef is added in p2m_set_way_flush(). This will avoid > the duplication between the cpreg and sysreg code. Yes, I can do that > > if ( !hsr.sysreg.read ) > > p2m_set_way_flush(current); > > break; > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > > index d414c4feb9..240913d5ac 100644 > > --- a/xen/arch/arm/p2m.c > > +++ b/xen/arch/arm/p2m.c > > @@ -1171,6 +1171,9 @@ void p2m_invalidate_root(struct p2m_domain *p2m) > > { > > unsigned int i; > > + /* Clearing the valid bit causes IOMMU faults. */ > > How about moving this comment on top of the function and writing: > > "p2m_invalid_root() should not be called when the P2M is shared with the IOMMU > because it will cause IOMMU fault." > > So one doesn't need to read the invalidation to understand that the function > should not be called when the P2M is shared with the IOMMU. Sure > > + ASSERT(!iommu_use_hap_pt(p2m->domain)); > + > > p2m_write_lock(p2m); > > for ( i = 0; i < P2M_ROOT_LEVEL; i++ ) > > diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c > > index e3ce56d875..04b68f6901 100644 > > --- a/xen/arch/arm/vcpreg.c > > +++ b/xen/arch/arm/vcpreg.c > > @@ -231,6 +231,13 @@ void do_cp15_32(struct cpu_user_regs *regs, const union > > hsr hsr) > > case HSR_CPREG32(DCISW): > > case HSR_CPREG32(DCCSW): > > case HSR_CPREG32(DCCISW): > > + if ( iommu_use_hap_pt(current->domain) ) > > + { > > + gdprintk(XENLOG_ERR, > > + "d%u uses set/way cache flushes with the IOMMU on. It > > cannot work. Replace set/way instructions with dc [ci]vac and retry. > > Injecting exception into the guest now.\n", > > + current->domain->domain_id); > > + return inject_undef_exception(regs, hsr); > > + } > > if ( !cp32.read ) > > p2m_set_way_flush(current); > > break; > > > > Cheers, > > -- > Julien Grall >
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |