xen/drivers/passthrough/amd/iommu_init.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
IV bit shouldn't be set in DTE if interrupt remapping is not
enabled. This was traced to be a root cause behind assertion in
interrupt handling code on Lisbon.
Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
xen/drivers/passthrough/amd/iommu_init.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index 16e84d4..2b81e38 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -1279,7 +1279,7 @@ static int __init amd_iommu_setup_device_table(
for ( bdf = 0, size /= sizeof(*dt); bdf < size; ++bdf )
dt[bdf] = (struct amd_iommu_dte){
.v = true,
- .iv = true,
+ .iv = iommu_intremap,
};
}
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develOn 25.11.2019 22:05, Igor Druzhinin wrote:
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -1279,7 +1279,7 @@ static int __init amd_iommu_setup_device_table(
> for ( bdf = 0, size /= sizeof(*dt); bdf < size; ++bdf )
> dt[bdf] = (struct amd_iommu_dte){
> .v = true,
> - .iv = true,
> + .iv = iommu_intremap,
This was very intentionally "true", and ignoring "iommu_intremap":
We're _pre_-filling DTEs here. Their actual values will be
established by the loop further down in the function, and just
for those devices that actually exist. By unilaterally setting IV
here we make sure that all interrupt requests from devices we
don't recognize get blocked rather than allowed through in an
un-remapped fashion.
The question continues to be which specific DTE the loop below
may wrongly leave untouched. Even if the the IDE device of the
chipset has no MSI/MSI-X, amd_iommu_set_intremap_table() at
the bottom of the loop should still get invoked, and hence IV
should still get set to false there when !iommu_intremap. There's
further investigation necessary, I'm afraid.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 26/11/2019 08:42, Jan Beulich wrote:
> On 25.11.2019 22:05, Igor Druzhinin wrote:
>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>> @@ -1279,7 +1279,7 @@ static int __init amd_iommu_setup_device_table(
>> for ( bdf = 0, size /= sizeof(*dt); bdf < size; ++bdf )
>> dt[bdf] = (struct amd_iommu_dte){
>> .v = true,
>> - .iv = true,
>> + .iv = iommu_intremap,
> This was very intentionally "true", and ignoring "iommu_intremap":
Deliberate or not, it is a regression from 4.12.
Booting with iommu=no-intremap is a common debugging technique, and that
means no interrupt remapping anywhere in the system, even for
supposedly-unused DTEs.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 26.11.2019 13:25, Andrew Cooper wrote:
> On 26/11/2019 08:42, Jan Beulich wrote:
>> On 25.11.2019 22:05, Igor Druzhinin wrote:
>>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>>> @@ -1279,7 +1279,7 @@ static int __init amd_iommu_setup_device_table(
>>> for ( bdf = 0, size /= sizeof(*dt); bdf < size; ++bdf )
>>> dt[bdf] = (struct amd_iommu_dte){
>>> .v = true,
>>> - .iv = true,
>>> + .iv = iommu_intremap,
>> This was very intentionally "true", and ignoring "iommu_intremap":
>
> Deliberate or not, it is a regression from 4.12.
I accept it's a regression (which wants fixing), but I don't think
this is the way to address is. I could be convinced by good
arguments, though.
> Booting with iommu=no-intremap is a common debugging technique, and that
> means no interrupt remapping anywhere in the system, even for
> supposedly-unused DTEs.
Whether IV=1 or IV=0, there's no interrupt _remapping_ with this
option specified. There's some interrupt _blocking_, yes. It's
not immediately clear to me whether this is a good or a bad thing.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 26/11/2019 14:14, Jan Beulich wrote:
> On 26.11.2019 13:25, Andrew Cooper wrote:
>> On 26/11/2019 08:42, Jan Beulich wrote:
>>> On 25.11.2019 22:05, Igor Druzhinin wrote:
>>>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>>>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>>>> @@ -1279,7 +1279,7 @@ static int __init amd_iommu_setup_device_table(
>>>> for ( bdf = 0, size /= sizeof(*dt); bdf < size; ++bdf )
>>>> dt[bdf] = (struct amd_iommu_dte){
>>>> .v = true,
>>>> - .iv = true,
>>>> + .iv = iommu_intremap,
>>> This was very intentionally "true", and ignoring "iommu_intremap":
>>
>> Deliberate or not, it is a regression from 4.12.
>
> I accept it's a regression (which wants fixing), but I don't think
> this is the way to address is. I could be convinced by good
> arguments, though.
Do you have any suggestions how to address that?
>> Booting with iommu=no-intremap is a common debugging technique, and that
>> means no interrupt remapping anywhere in the system, even for
>> supposedly-unused DTEs.
>
> Whether IV=1 or IV=0, there's no interrupt _remapping_ with this
> option specified. There's some interrupt _blocking_, yes. It's
> not immediately clear to me whether this is a good or a bad thing.
From user point of view, if I supply "iommu=no-intremap" I'm not
expecting any interrupts in the system to be blocked either. And
as Andrew said we frequently use this option for debugging which
means we expect this functionality to be off completely.
Igor
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 26.11.2019 15:24, Igor Druzhinin wrote:
> On 26/11/2019 14:14, Jan Beulich wrote:
>> On 26.11.2019 13:25, Andrew Cooper wrote:
>>> On 26/11/2019 08:42, Jan Beulich wrote:
>>>> On 25.11.2019 22:05, Igor Druzhinin wrote:
>>>>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>>>>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>>>>> @@ -1279,7 +1279,7 @@ static int __init amd_iommu_setup_device_table(
>>>>> for ( bdf = 0, size /= sizeof(*dt); bdf < size; ++bdf )
>>>>> dt[bdf] = (struct amd_iommu_dte){
>>>>> .v = true,
>>>>> - .iv = true,
>>>>> + .iv = iommu_intremap,
>>>> This was very intentionally "true", and ignoring "iommu_intremap":
>>>
>>> Deliberate or not, it is a regression from 4.12.
>>
>> I accept it's a regression (which wants fixing), but I don't think
>> this is the way to address is. I could be convinced by good
>> arguments, though.
>
> Do you have any suggestions how to address that?
I'd like to reply in the other context, after a little more
thinking about the situation. I think I see an oversight of
mine.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 26/11/2019 14:14, Jan Beulich wrote:
> On 26.11.2019 13:25, Andrew Cooper wrote:
>> On 26/11/2019 08:42, Jan Beulich wrote:
>>> On 25.11.2019 22:05, Igor Druzhinin wrote:
>>>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>>>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>>>> @@ -1279,7 +1279,7 @@ static int __init amd_iommu_setup_device_table(
>>>> for ( bdf = 0, size /= sizeof(*dt); bdf < size; ++bdf )
>>>> dt[bdf] = (struct amd_iommu_dte){
>>>> .v = true,
>>>> - .iv = true,
>>>> + .iv = iommu_intremap,
>>> This was very intentionally "true", and ignoring "iommu_intremap":
>> Deliberate or not, it is a regression from 4.12.
> I accept it's a regression (which wants fixing), but I don't think
> this is the way to address is. I could be convinced by good
> arguments, though.
>
>> Booting with iommu=no-intremap is a common debugging technique, and that
>> means no interrupt remapping anywhere in the system, even for
>> supposedly-unused DTEs.
> Whether IV=1 or IV=0, there's no interrupt _remapping_ with this
> option specified. There's some interrupt _blocking_, yes. It's
> not immediately clear to me whether this is a good or a bad thing.
You're attempting to argue semantics. Blocking is a special case remapping.
"iommu=no-intremap" (for better or worse, naming wise) refers to the
interrupt mediation functionality in the IOMMU, and means "don't use any
of it". Any other behaviour is a regression.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 26.11.2019 15:34, Andrew Cooper wrote:
> On 26/11/2019 14:14, Jan Beulich wrote:
>> On 26.11.2019 13:25, Andrew Cooper wrote:
>>> On 26/11/2019 08:42, Jan Beulich wrote:
>>>> On 25.11.2019 22:05, Igor Druzhinin wrote:
>>>>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>>>>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>>>>> @@ -1279,7 +1279,7 @@ static int __init amd_iommu_setup_device_table(
>>>>> for ( bdf = 0, size /= sizeof(*dt); bdf < size; ++bdf )
>>>>> dt[bdf] = (struct amd_iommu_dte){
>>>>> .v = true,
>>>>> - .iv = true,
>>>>> + .iv = iommu_intremap,
>>>> This was very intentionally "true", and ignoring "iommu_intremap":
>>> Deliberate or not, it is a regression from 4.12.
>> I accept it's a regression (which wants fixing), but I don't think
>> this is the way to address is. I could be convinced by good
>> arguments, though.
>>
>>> Booting with iommu=no-intremap is a common debugging technique, and that
>>> means no interrupt remapping anywhere in the system, even for
>>> supposedly-unused DTEs.
>> Whether IV=1 or IV=0, there's no interrupt _remapping_ with this
>> option specified. There's some interrupt _blocking_, yes. It's
>> not immediately clear to me whether this is a good or a bad thing.
>
> You're attempting to argue semantics. Blocking is a special case remapping.
>
> "iommu=no-intremap" (for better or worse, naming wise) refers to the
> interrupt mediation functionality in the IOMMU, and means "don't use any
> of it". Any other behaviour is a regression.
I can accept this pov. Nevertheless I'd like to first see whether
we can't address the issue at hand with a less big hammer solution.
We can then always decide to still put in this change.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.