On 13/01/2023 8:47 am, Jan Beulich wrote:
As far as the subject goes, I really wouldn't call this "sanitise". The behaviour is crazy, and broken. "Make shadow consistent with how HAP works" feels somewhat better. > First of all the variable is meaningful only when an IOMMU is in use for > a guest. Qualify the check accordingly, like done elsewhere. Furthermore > the controlling command line option is supposed to take effect on VT-d > only. Since command line parsing happens before we know whether we're > going to use VT-d, force the variable back to set when instead running > with AMD IOMMU(s). > > Signed-off-by: Jan Beulich <jbeul...@suse.com> > --- > I was first considering to add the extra check to the outermost > enclosing if(), but I guess that would break the (questionable) case of > assigning MMIO ranges directly by address. The way it's done now also > better fits the existing checks, in particular the ones in p2m-ept.c. > > Note that the #ifndef is put there in anticipation of iommu_snoop > becoming a #define when !IOMMU_INTEL (see > https://lists.xen.org/archives/html/xen-devel/2023-01/msg00103.html > and replies). > > In _sh_propagate() I'm further puzzled: The iomem_access_permitted() > certainly suggests very bad things could happen if it returned false > (i.e. in the implicit "else" case). The assumption looks to be that no > bad "target_mfn" can make it there. But overall things might end up > looking more sane (and being cheaper) when simply using "mmio_mfn" > instead. That entire block looks suspect. For one, I can't see why the ASSERT() is correct; we have literally just (conditionally) added CACHE_ATTR to pass_thru_flags and pulled everything across from gflags into sflags. It can also half its number of external calls by rearranging the if/else chain and making better use of the type variable. > > --- a/xen/arch/x86/mm/shadow/multi.c > +++ b/xen/arch/x86/mm/shadow/multi.c Just out of context here is a comment which says VT-d but really means IOMMU. It probably wants adjusting in the context of this change. > @@ -571,7 +571,7 @@ _sh_propagate(struct vcpu *v, > gfn_to_paddr(target_gfn), > mfn_to_maddr(target_mfn), > X86_MT_UC); > - else if ( iommu_snoop ) > + else if ( is_iommu_enabled(d) && iommu_snoop ) > sflags |= pat_type_2_pte_flags(X86_MT_WB); Hmm... This is still one reasonably expensive nop; the PTE Flags for WB are 0. > else > sflags |= get_pat_flags(v, > --- a/xen/drivers/passthrough/x86/iommu.c > +++ b/xen/drivers/passthrough/x86/iommu.c > @@ -56,6 +56,13 @@ void __init acpi_iommu_init(void) > if ( !acpi_disabled ) > { > ret = acpi_dmar_init(); > + > +#ifndef iommu_snoop > + /* A command line override for snoop control affects VT-d only. */ > + if ( ret ) > + iommu_snoop = true; > +#endif I really don't think this is a good idea. If nothing else, you're reinforcing the notion that this logic is somehow acceptable. If instead the comment read something like: /* This logic is pretty bogus, but necessary for now. iommu_snoop as a control is only wired up for VT-d (which may be conditionally compiled out), and while AMD can control coherency, Xen forces coherent accesses unilaterally so iommu_snoop needs to report true on all AMD systems for logic elsewhere in Xen to behave correctly. */ That at least highlights that it is a giant bodge. ~Andrew
