[Xen-devel] [PATCH] AMD/IOMMU: correct handling when XT's prereq features are unavailable

[Xen-devel] [PATCH] AMD/IOMMU: correct handling when XT's prereq features are unavailable

[Jan Beulich]

We should neither cause IOMMU initialization as a whole to fail in this
case (we should still be able to bring up the system in non-x2APIC or
x2APIC physical mode), nor should the remainder of the function be
skipped (as the main part of it won't get entered a 2nd time) in such an
event. It is merely necessary for the function to indicate to the caller
(iov_supports_xt()) that setup failed as far as x2APIC is concerned.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -1364,6 +1364,7 @@ static int __init amd_iommu_prepare_one(
 int __init amd_iommu_prepare(bool xt)
 {
     struct amd_iommu *iommu;
+    bool no_xt = false;
     int rc = -ENODEV;
 
     BUG_ON( !iommu_found() );
@@ -1400,9 +1401,8 @@ int __init amd_iommu_prepare(bool xt)
         if ( rc )
             goto error_out;
 
-        rc = -ENODEV;
-        if ( xt && (!iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup) )
-            goto error_out;
+        if ( !iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup )
+            no_xt = true;
     }
 
     for_each_amd_iommu ( iommu )
@@ -1422,7 +1422,7 @@ int __init amd_iommu_prepare(bool xt)
         ivhd_type = 0;
     }
 
-    return rc;
+    return rc ?: xt && no_xt ? -ENODEV : 0;
 }
 
 int __init amd_iommu_init(bool xt)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] AMD/IOMMU: correct handling when XT's prereq features are unavailable

[Roger Pau Monné]

On Thu, Feb 27, 2020 at 03:34:48PM +0100, Jan Beulich wrote:
> We should neither cause IOMMU initialization as a whole to fail in this
> case (we should still be able to bring up the system in non-x2APIC or
> x2APIC physical mode), nor should the remainder of the function be
> skipped (as the main part of it won't get entered a 2nd time) in such an
> event. It is merely necessary for the function to indicate to the caller
> (iov_supports_xt()) that setup failed as far as x2APIC is concerned.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -1364,6 +1364,7 @@ static int __init amd_iommu_prepare_one(
>  int __init amd_iommu_prepare(bool xt)
>  {
>      struct amd_iommu *iommu;
> +    bool no_xt = false;
>      int rc = -ENODEV;
>  
>      BUG_ON( !iommu_found() );
> @@ -1400,9 +1401,8 @@ int __init amd_iommu_prepare(bool xt)
>          if ( rc )
>              goto error_out;
>  
> -        rc = -ENODEV;
> -        if ( xt && (!iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup) )
> -            goto error_out;
> +        if ( !iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup )
> +            no_xt = true;

Don't you need to also adjust the usage of xt in the
for_each_amd_iommu loop below, so that the control registers fields
get initialized properly?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] AMD/IOMMU: correct handling when XT's prereq features are unavailable

[Andrew Cooper]

On 27/02/2020 14:34, Jan Beulich wrote:
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -1364,6 +1364,7 @@ static int __init amd_iommu_prepare_one(
>  int __init amd_iommu_prepare(bool xt)
>  {
>      struct amd_iommu *iommu;
> +    bool no_xt = false;

I think the logic would be easier to follow if this was has_xt, with
inverted meaning.  However...

>      int rc = -ENODEV;
>  
>      BUG_ON( !iommu_found() );
> @@ -1400,9 +1401,8 @@ int __init amd_iommu_prepare(bool xt)
>          if ( rc )
>              goto error_out;
>  
> -        rc = -ENODEV;
> -        if ( xt && (!iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup) )
> -            goto error_out;
> +        if ( !iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup )
> +            no_xt = true;
>      }
>  
>      for_each_amd_iommu ( iommu )

... the contents of this loop depends on the early exit path you've just
deleted.

In the case of x2apic not being available, we'll still set {ga,xt}_en to
the caller requested value.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] AMD/IOMMU: correct handling when XT's prereq features are unavailable

[Jan Beulich]

On 27.02.2020 16:25, Andrew Cooper wrote:
> On 27/02/2020 14:34, Jan Beulich wrote:
>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>> @@ -1364,6 +1364,7 @@ static int __init amd_iommu_prepare_one(
>>  int __init amd_iommu_prepare(bool xt)
>>  {
>>      struct amd_iommu *iommu;
>> +    bool no_xt = false;
> 
> I think the logic would be easier to follow if this was has_xt, with
> inverted meaning.

I'm not fully convinced it'll make it easier, but I've switched
things around.

>  However...
> 
>>      int rc = -ENODEV;
>>  
>>      BUG_ON( !iommu_found() );
>> @@ -1400,9 +1401,8 @@ int __init amd_iommu_prepare(bool xt)
>>          if ( rc )
>>              goto error_out;
>>  
>> -        rc = -ENODEV;
>> -        if ( xt && (!iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup) )
>> -            goto error_out;
>> +        if ( !iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup )
>> +            no_xt = true;
>>      }
>>  
>>      for_each_amd_iommu ( iommu )
> 
> ... the contents of this loop depends on the early exit path you've just
> deleted.
> 
> In the case of x2apic not being available, we'll still set {ga,xt}_en to
> the caller requested value.

Oh, indeed (and Roger, thank you too for noticing this). In fact
it explains a hang later during boot that I did observe, and that
I meant to look into later. That said, interrupts for Dom0 still
don't seem to work quite right in this (partly broken) mode even
with this fixed (there are several "No irq handler for vector"
messages, and at the very least the disk doesn't work); I'll see
to find time to look into that as well, but I'm pretty convinced
it's an independent issue. (Seeing that interrupt remapping gets
enabled this way, and x2APIC is pre-enabled, I'm suspecting this
is another case where we need to force physical mode.)

Let me blame this on the cold I'm fighting, which isn't quite bad
enough to stay home, but which also isn't helpful. v2 coming ...

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] AMD/IOMMU: correct handling when XT's prereq features are unavailable

[Jan Beulich]

On 28.02.2020 09:14, Jan Beulich wrote:
> On 27.02.2020 16:25, Andrew Cooper wrote:
>> On 27/02/2020 14:34, Jan Beulich wrote:
>>> @@ -1400,9 +1401,8 @@ int __init amd_iommu_prepare(bool xt)
>>>          if ( rc )
>>>              goto error_out;
>>>  
>>> -        rc = -ENODEV;
>>> -        if ( xt && (!iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup) )
>>> -            goto error_out;
>>> +        if ( !iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup )
>>> +            no_xt = true;
>>>      }
>>>  
>>>      for_each_amd_iommu ( iommu )
>>
>> ... the contents of this loop depends on the early exit path you've just
>> deleted.
>>
>> In the case of x2apic not being available, we'll still set {ga,xt}_en to
>> the caller requested value.
> 
> Oh, indeed (and Roger, thank you too for noticing this). In fact
> it explains a hang later during boot that I did observe, and that
> I meant to look into later. That said, interrupts for Dom0 still
> don't seem to work quite right in this (partly broken) mode even
> with this fixed (there are several "No irq handler for vector"
> messages, and at the very least the disk doesn't work); I'll see
> to find time to look into that as well, but I'm pretty convinced
> it's an independent issue. (Seeing that interrupt remapping gets
> enabled this way, and x2APIC is pre-enabled, I'm suspecting this
> is another case where we need to force physical mode.)

And indeed x2apic_phys on the command line makes things work. Now
I'll have to figure a reasonable way when and how to communicate
the need to switch the mode. (Another potential issue I'm seeing is
that it may not be okay to bring up a CPU with APIC ID 0xff in
this case, and it pretty certainly would require further precautions
if we were to allow bringing up CPUs with even larger APIC IDs.)

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel