[PATCH 0/2] x86/shadow: MMIO treatment

[PATCH 0/2] x86/shadow: MMIO treatment

[Jan Beulich]

While reviewing Xenia's change to iommu_snoop placement, I've
spotted a shortcoming in _sh_propagate(), fixing of which made
me notice (again) a 2nd kind of issue.

1: sanitize iommu_snoop usage
2: further correct MMIO handling in _sh_propagate()

Jan

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

[Jan Beulich]

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@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
+
         if ( ret == -ENODEV )
             ret = acpi_ivrs_init();
     }

[PATCH 2/2] x86/shadow: further correct MMIO handling in _sh_propagate()

[Jan Beulich]

While c61a6f74f80e ("x86: enforce consistent cachability of MMIO
mappings") correctly converted one !mfn_valid() check there, two others
were wrongly left untouched: Both cachability control and log-dirty
tracking ought to be uniformly handled/excluded for all (non-)MMIO
ranges, not just ones qualifiable by mfn_valid().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Note that this is orthogonal to there looking to be plans to undo other
aspects of said commit (XSA-154).

--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -543,8 +543,7 @@ _sh_propagate(struct vcpu *v,
      * caching attributes in the shadows to match what was asked for.
      */
     if ( (level == 1) && is_hvm_domain(d) &&
-         (!mfn_valid(target_mfn) ||
-          !is_special_page(mfn_to_page(target_mfn))) )
+         (mmio_mfn || !is_special_page(mfn_to_page(target_mfn))) )
     {
         int type;
 
@@ -655,8 +654,7 @@ _sh_propagate(struct vcpu *v,
      * (We handle log-dirty entirely inside the shadow code, without using the
      * p2m_ram_logdirty p2m type: only HAP uses that.)
      */
-    if ( level == 1 && unlikely(shadow_mode_log_dirty(d)) &&
-         mfn_valid(target_mfn) )
+    if ( level == 1 && unlikely(shadow_mode_log_dirty(d)) && !mmio_mfn )
     {
         if ( ft & FETCH_TYPE_WRITE )
             paging_mark_dirty(d, target_mfn);

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

[Xenia Ragiadakou]

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@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.

>           if ( ret == -ENODEV )
>               ret = acpi_ivrs_init();
>       }
> 

-- 
Xenia

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

[Jan Beulich]

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@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

Re: [PATCH 2/2] x86/shadow: further correct MMIO handling in _sh_propagate()

[Andrew Cooper]

On 13/01/2023 8:48 am, Jan Beulich wrote:
> While c61a6f74f80e ("x86: enforce consistent cachability of MMIO
> mappings") correctly converted one !mfn_valid() check there, two others
> were wrongly left untouched: Both cachability control and log-dirty
> tracking ought to be uniformly handled/excluded for all (non-)MMIO
> ranges, not just ones qualifiable by mfn_valid().
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[Jan Beulich]

(missed to CC Paul on the original submission)

On 13.01.2023 09: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@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
> +
>          if ( ret == -ENODEV )
>              ret = acpi_ivrs_init();
>      }
> 
> 

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

[Andrew Cooper]

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 <jbeulich@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

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

[Xenia Ragiadakou]

(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@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.

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

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

[Jan Beulich]

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.

> 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

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

[Xenia Ragiadakou]

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

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

[Jan Beulich]

On 13.01.2023 14:07, Xenia Ragiadakou wrote:
> 
> 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().

Command line parsing happens earlier (and in parse_iommu_param(), not in
iommu_setup()). iommu_setup() can further overwrite it on its error path,
but as said that's benign then.

> I m afraid I still cannot understand why the change above is needed.

When using an AMD IOMMU, with how things work right now the variable ought
to always be true (hence why I've suggested that when !INTEL_IOMMU, this
simply become a #define to true). See also Andrew's comments here and/or
on your patch.

Jan

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

[Xenia Ragiadakou]

On 1/13/23 15:24, Jan Beulich wrote:
> On 13.01.2023 14:07, Xenia Ragiadakou wrote:
>>
>> 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().
> 
> Command line parsing happens earlier (and in parse_iommu_param(), not in
> iommu_setup()). iommu_setup() can further overwrite it on its error path,
> but as said that's benign then.

You are right. I misunderstood.

> 
>> I m afraid I still cannot understand why the change above is needed.
> 
> When using an AMD IOMMU, with how things work right now the variable ought
> to always be true (hence why I've suggested that when !INTEL_IOMMU, this
> simply become a #define to true). See also Andrew's comments here and/or
> on your patch.

Ok I see, so this change is specific to AMD iommu and when iommu_snoop 
becomes a #define, this change won't be needed anymore, right?

-- 
Xenia

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

[Jan Beulich]

On 13.01.2023 14:53, Xenia Ragiadakou wrote:
> On 1/13/23 15:24, Jan Beulich wrote:
>> On 13.01.2023 14:07, Xenia Ragiadakou wrote:
>>> 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().
>>
>> Command line parsing happens earlier (and in parse_iommu_param(), not in
>> iommu_setup()). iommu_setup() can further overwrite it on its error path,
>> but as said that's benign then.
> 
> You are right. I misunderstood.
> 
>>
>>> I m afraid I still cannot understand why the change above is needed.
>>
>> When using an AMD IOMMU, with how things work right now the variable ought
>> to always be true (hence why I've suggested that when !INTEL_IOMMU, this
>> simply become a #define to true). See also Andrew's comments here and/or
>> on your patch.
> 
> Ok I see, so this change is specific to AMD iommu and when iommu_snoop 
> becomes a #define, this change won't be needed anymore, right?

Well the (source) code change will still be needed; it'll simply be
compiled out for the case where iommu_snoop is a #define (which it
looks like we agree it will be when !INTEL_IOMMU).

Jan

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

[Xenia Ragiadakou]

On 1/13/23 16:22, Jan Beulich wrote:
> On 13.01.2023 14:53, Xenia Ragiadakou wrote:
>> On 1/13/23 15:24, Jan Beulich wrote:
>>> On 13.01.2023 14:07, Xenia Ragiadakou wrote:
>>>> 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().
>>>
>>> Command line parsing happens earlier (and in parse_iommu_param(), not in
>>> iommu_setup()). iommu_setup() can further overwrite it on its error path,
>>> but as said that's benign then.
>>
>> You are right. I misunderstood.
>>
>>>
>>>> I m afraid I still cannot understand why the change above is needed.
>>>
>>> When using an AMD IOMMU, with how things work right now the variable ought
>>> to always be true (hence why I've suggested that when !INTEL_IOMMU, this
>>> simply become a #define to true). See also Andrew's comments here and/or
>>> on your patch.
>>
>> Ok I see, so this change is specific to AMD iommu and when iommu_snoop
>> becomes a #define, this change won't be needed anymore, right?
> 
> Well the (source) code change will still be needed; it'll simply be
> compiled out for the case where iommu_snoop is a #define (which it
> looks like we agree it will be when !INTEL_IOMMU).

Yes. Actually, I was thinking to move the setup of iommu_snoop out of 
the X86 #ifdef and to make it depend only on INTEL_IOMMU since for !X86 
only matters to be defined.

-- 
Xenia

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

[Jan Beulich]

On 13.01.2023 12:55, Andrew Cooper wrote:
> On 13/01/2023 8:47 am, Jan Beulich wrote:
>> 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.

That is for !shadow_mode_refcounts() domains, i.e. PV, whereas the
outermost conditional here limits things to HVM. Using different
predicates of course obfuscates this some, but bringing those two
closer together (perhaps even merging them) didn't look reasonable
to do right here.

> It can also half its number of external calls by rearranging the if/else
> chain and making better use of the type variable.

I did actually spend quite a bit of time to see whether I could figure
a valid way of re-arranging the order, but in the end for every
transformation I found a reason why it wouldn't be valid. So I'm
curious what valid simplification(s) you see.

>> --- 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.

I was pondering that when making the patch, but wasn't sure about making
such a not directly related adjustment right here. Now that you ask for
it, I've done so. I've also removed the "and device assigned" part.

>> @@ -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.

Right, but besides being unrelated to the patch (there's a following
"else", so the condition cannot be purged altogether) I would wonder
if we really want to bake in more PAT layout <-> PTE dependencies.

>> --- 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. */

I've extended the comment to this:

        /*
         * As long as there's no per-domain snoop control, and as long as on
         * AMD we uniformly force coherent accesses, a possible command line
         * override should affect VT-d only.
         */

Jan

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

[Andrew Cooper]

On 16/01/2023 8:56 am, Jan Beulich wrote:
> On 13.01.2023 12:55, Andrew Cooper wrote:
>> On 13/01/2023 8:47 am, Jan Beulich wrote:
>>> 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.
> That is for !shadow_mode_refcounts() domains, i.e. PV, whereas the
> outermost conditional here limits things to HVM. Using different
> predicates of course obfuscates this some, but bringing those two
> closer together (perhaps even merging them) didn't look reasonable
> to do right here.

Ah, that bit.  Also further obfuscated by partial nested !'s.

I doubt Shadow has seen anything beyond token testing in combination
with PCI Passthrough.  It certainly saw no testing under XenServer.

>> It can also half its number of external calls by rearranging the if/else
>> chain and making better use of the type variable.
> I did actually spend quite a bit of time to see whether I could figure
> a valid way of re-arranging the order, but in the end for every
> transformation I found a reason why it wouldn't be valid. So I'm
> curious what valid simplification(s) you see.

Well, the first two calls calls to pat_type_2_pte_flags() can be merged
quite easily, but I was also thinking in terms of future where coherency
handling was working in a more sane way.

>>> @@ -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.
> Right, but besides being unrelated to the patch (there's a following
> "else", so the condition cannot be purged altogether) I would wonder
> if we really want to bake in more PAT layout <-> PTE dependencies.

I'm not advocating for more assumption about PAT <-> PTE layout, but it
would be nice if the NOPs were actually NOPs.

I submitted a patch which makes pat_type_2_pte_flags() marginally less
expensive, but there's still massive savings to be made here.  Because
XEN's PAT is compile time constant, this inverse can be too.

>
>>> --- 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. */
> I've extended the comment to this:
>
>         /*
>          * As long as there's no per-domain snoop control, and as long as on
>          * AMD we uniformly force coherent accesses, a possible command line
>          * override should affect VT-d only.
>          */

Better.  I suppose my displeasure of this can live on list...

~Andrew