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

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

On 1/13/23 14:12, Jan Beulich wrote:
> On 13.01.2023 12:55, Xenia Ragiadakou wrote:
> > 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:
> > > > > --- 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?
> For a non-existent (or disabled) IOMMU the value of this boolean simply
> is irrelevant. Or to put it in other words, when there's no active
> IOMMU, it doesn't matter whether it would actually snoop.
The variable iommu_snoop is initialized to true.
Also, the above change does not prevent it from being overwritten 
through the cmdline iommu param in iommu_setup().
I m afraid I still cannot understand why the change above is needed.

> > Also, in vmx_do_resume(), iommu_snoop is used without checking if the
> > iommu is enabled.
> It only looks to be - a domain having a PCI device implies it having
> IOMMU enabled for it. And indeed in that case we'd like to avoid the
> effort for domains which have IOMMU support enabled for them, but which
> have no devices assigned to them.
>
> Jan
--
Xenia