Re: [PATCH 1/2] x86/shadow: sanitize iommu_snoop usage

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

(CC Paul as well)

On 1/13/23 11:53, Jan Beulich wrote:
> 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 <jbeulich@xxxxxxxx>
> > > ---
> > > 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.
I think I got confused... AFAIU with disabled iommu snooping cannot be 
enforced i.e iommu_snoop=true translates to snooping is enforced by the 
iommu (that's why we need to check that the iommu is enabled for the 
guest). So if the iommu is disabled how can iommu_snoop be true?
Also, in vmx_do_resume(), iommu_snoop is used without checking if the 
iommu is enabled.
--
Xenia