On 13.01.2023 10:34, Xenia Ragiadakou wrote: > > On 1/13/23 10:47, Jan Beulich wrote: >> 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. >> >> --- a/xen/arch/x86/mm/shadow/multi.c >> +++ b/xen/arch/x86/mm/shadow/multi.c >> @@ -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); >> 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 >> + > > Why here iommu_snoop is forced when iommu is not enabled? > This change is confusing because later on, in iommu_setup, iommu_snoop > will be set to false in the case of !iommu_enabled.
Counter question: Why is it being set to false there? I see no reason at all. On the same basis as here, I'd actually expect it to be set back to true in such a case. Which, however, would be a benign change now that all uses of the variable are properly qualified. Which in turn is why I thought I'd leave that other place alone. Jan
