* [PATCH] iommu/amd: fix interrupt remapping for avic
@ 2020-09-13 12:42 ` Maxim Levitsky
0 siblings, 0 replies; 12+ messages in thread
From: Maxim Levitsky @ 2020-09-13 12:42 UTC (permalink / raw)
To: linux-kernel
Cc: Suravee Suthikulpanit, open list:AMD IOMMU (AMD-VI),
Joao Martins, Joerg Roedel, Maxim Levitsky
Commit e52d58d54a32 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE")
accidentally removed an assumption that modify_irte_ga always set the valid bit
and amd_iommu_activate_guest_mode relied on that.
Side effect of this is that on my machine, VFIO based VMs with AVIC enabled
would eventually crash and show IOMMU errors like that:
AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0055 address=0xfffffffdf8000000 flags=0x0008]
Fixes: e52d58d54a321 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE")
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
drivers/iommu/amd/iommu.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 07ae8b93887e5..aff4cc1869356 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -3853,6 +3853,7 @@ int amd_iommu_activate_guest_mode(void *data)
entry->hi.fields.ga_root_ptr = ir_data->ga_root_ptr;
entry->hi.fields.vector = ir_data->ga_vector;
entry->lo.fields_vapic.ga_tag = ir_data->ga_tag;
+ entry->lo.fields_remap.valid = 1;
return modify_irte_ga(ir_data->irq_2_irte.devid,
ir_data->irq_2_irte.index, entry, ir_data);
--
2.26.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH] iommu/amd: fix interrupt remapping for avic @ 2020-09-13 12:42 ` Maxim Levitsky 0 siblings, 0 replies; 12+ messages in thread From: Maxim Levitsky @ 2020-09-13 12:42 UTC (permalink / raw) To: linux-kernel; +Cc: open list:AMD IOMMU AMD-VI, Maxim Levitsky, Joao Martins Commit e52d58d54a32 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE") accidentally removed an assumption that modify_irte_ga always set the valid bit and amd_iommu_activate_guest_mode relied on that. Side effect of this is that on my machine, VFIO based VMs with AVIC enabled would eventually crash and show IOMMU errors like that: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0055 address=0xfffffffdf8000000 flags=0x0008] Fixes: e52d58d54a321 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE") Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- drivers/iommu/amd/iommu.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c index 07ae8b93887e5..aff4cc1869356 100644 --- a/drivers/iommu/amd/iommu.c +++ b/drivers/iommu/amd/iommu.c @@ -3853,6 +3853,7 @@ int amd_iommu_activate_guest_mode(void *data) entry->hi.fields.ga_root_ptr = ir_data->ga_root_ptr; entry->hi.fields.vector = ir_data->ga_vector; entry->lo.fields_vapic.ga_tag = ir_data->ga_tag; + entry->lo.fields_remap.valid = 1; return modify_irte_ga(ir_data->irq_2_irte.devid, ir_data->irq_2_irte.index, entry, ir_data); -- 2.26.2 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] iommu/amd: fix interrupt remapping for avic 2020-09-13 12:42 ` Maxim Levitsky @ 2020-09-14 14:48 ` Suravee Suthikulpanit -1 siblings, 0 replies; 12+ messages in thread From: Suravee Suthikulpanit @ 2020-09-14 14:48 UTC (permalink / raw) To: Maxim Levitsky, linux-kernel Cc: open list:AMD IOMMU (AMD-VI), Joao Martins, Joerg Roedel Maxim, On 9/13/2020 7:42 PM, Maxim Levitsky wrote: > Commit e52d58d54a32 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE") > accidentally removed an assumption that modify_irte_ga always set the valid bit > and amd_iommu_activate_guest_mode relied on that. > > Side effect of this is that on my machine, VFIO based VMs with AVIC enabled > would eventually crash and show IOMMU errors like that: > > AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0055 address=0xfffffffdf8000000 flags=0x0008] > > Fixes: e52d58d54a321 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE") > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > drivers/iommu/amd/iommu.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c > index 07ae8b93887e5..aff4cc1869356 100644 > --- a/drivers/iommu/amd/iommu.c > +++ b/drivers/iommu/amd/iommu.c > @@ -3853,6 +3853,7 @@ int amd_iommu_activate_guest_mode(void *data) > entry->hi.fields.ga_root_ptr = ir_data->ga_root_ptr; > entry->hi.fields.vector = ir_data->ga_vector; > entry->lo.fields_vapic.ga_tag = ir_data->ga_tag; > + entry->lo.fields_remap.valid = 1; > > return modify_irte_ga(ir_data->irq_2_irte.devid, > ir_data->irq_2_irte.index, entry, ir_data); > Could you please try with the following patch instead? --- a/drivers/iommu/amd/iommu.c +++ b/drivers/iommu/amd/iommu.c @@ -3840,14 +3840,18 @@ int amd_iommu_activate_guest_mode(void *data) { struct amd_ir_data *ir_data = (struct amd_ir_data *)data; struct irte_ga *entry = (struct irte_ga *) ir_data->entry; + u64 valid; if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || !entry || entry->lo.fields_vapic.guest_mode) return 0; + valid = entry->lo.fields_vapic.valid; + entry->lo.val = 0; entry->hi.val = 0; + entry->lo.fields_vapic.valid = valid; entry->lo.fields_vapic.guest_mode = 1; entry->lo.fields_vapic.ga_log_intr = 1; entry->hi.fields.ga_root_ptr = ir_data->ga_root_ptr; @@ -3864,12 +3868,14 @@ int amd_iommu_deactivate_guest_mode(void *data) struct amd_ir_data *ir_data = (struct amd_ir_data *)data; struct irte_ga *entry = (struct irte_ga *) ir_data->entry; struct irq_cfg *cfg = ir_data->cfg; - u64 valid = entry->lo.fields_remap.valid; + u64 valid; if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || !entry || !entry->lo.fields_vapic.guest_mode) return 0; + valid = entry->lo.fields_remap.valid; + entry->lo.val = 0; entry->hi.val = 0; -- Thanks, Suravee ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] iommu/amd: fix interrupt remapping for avic @ 2020-09-14 14:48 ` Suravee Suthikulpanit 0 siblings, 0 replies; 12+ messages in thread From: Suravee Suthikulpanit @ 2020-09-14 14:48 UTC (permalink / raw) To: Maxim Levitsky, linux-kernel; +Cc: open list:AMD IOMMU (AMD-VI), Joao Martins Maxim, On 9/13/2020 7:42 PM, Maxim Levitsky wrote: > Commit e52d58d54a32 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE") > accidentally removed an assumption that modify_irte_ga always set the valid bit > and amd_iommu_activate_guest_mode relied on that. > > Side effect of this is that on my machine, VFIO based VMs with AVIC enabled > would eventually crash and show IOMMU errors like that: > > AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0055 address=0xfffffffdf8000000 flags=0x0008] > > Fixes: e52d58d54a321 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE") > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > drivers/iommu/amd/iommu.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c > index 07ae8b93887e5..aff4cc1869356 100644 > --- a/drivers/iommu/amd/iommu.c > +++ b/drivers/iommu/amd/iommu.c > @@ -3853,6 +3853,7 @@ int amd_iommu_activate_guest_mode(void *data) > entry->hi.fields.ga_root_ptr = ir_data->ga_root_ptr; > entry->hi.fields.vector = ir_data->ga_vector; > entry->lo.fields_vapic.ga_tag = ir_data->ga_tag; > + entry->lo.fields_remap.valid = 1; > > return modify_irte_ga(ir_data->irq_2_irte.devid, > ir_data->irq_2_irte.index, entry, ir_data); > Could you please try with the following patch instead? --- a/drivers/iommu/amd/iommu.c +++ b/drivers/iommu/amd/iommu.c @@ -3840,14 +3840,18 @@ int amd_iommu_activate_guest_mode(void *data) { struct amd_ir_data *ir_data = (struct amd_ir_data *)data; struct irte_ga *entry = (struct irte_ga *) ir_data->entry; + u64 valid; if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || !entry || entry->lo.fields_vapic.guest_mode) return 0; + valid = entry->lo.fields_vapic.valid; + entry->lo.val = 0; entry->hi.val = 0; + entry->lo.fields_vapic.valid = valid; entry->lo.fields_vapic.guest_mode = 1; entry->lo.fields_vapic.ga_log_intr = 1; entry->hi.fields.ga_root_ptr = ir_data->ga_root_ptr; @@ -3864,12 +3868,14 @@ int amd_iommu_deactivate_guest_mode(void *data) struct amd_ir_data *ir_data = (struct amd_ir_data *)data; struct irte_ga *entry = (struct irte_ga *) ir_data->entry; struct irq_cfg *cfg = ir_data->cfg; - u64 valid = entry->lo.fields_remap.valid; + u64 valid; if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || !entry || !entry->lo.fields_vapic.guest_mode) return 0; + valid = entry->lo.fields_remap.valid; + entry->lo.val = 0; entry->hi.val = 0; -- Thanks, Suravee _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] iommu/amd: fix interrupt remapping for avic 2020-09-14 14:48 ` Suravee Suthikulpanit @ 2020-09-15 11:25 ` Maxim Levitsky -1 siblings, 0 replies; 12+ messages in thread From: Maxim Levitsky @ 2020-09-15 11:25 UTC (permalink / raw) To: Suravee Suthikulpanit, linux-kernel Cc: open list:AMD IOMMU (AMD-VI), Joao Martins, Joerg Roedel On Mon, 2020-09-14 at 21:48 +0700, Suravee Suthikulpanit wrote: > Maxim, > > On 9/13/2020 7:42 PM, Maxim Levitsky wrote: > > Commit e52d58d54a32 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE") > > accidentally removed an assumption that modify_irte_ga always set the valid bit > > and amd_iommu_activate_guest_mode relied on that. > > > > Side effect of this is that on my machine, VFIO based VMs with AVIC enabled > > would eventually crash and show IOMMU errors like that: > > > > AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0055 address=0xfffffffdf8000000 flags=0x0008] > > > > Fixes: e52d58d54a321 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE") > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > drivers/iommu/amd/iommu.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c > > index 07ae8b93887e5..aff4cc1869356 100644 > > --- a/drivers/iommu/amd/iommu.c > > +++ b/drivers/iommu/amd/iommu.c > > @@ -3853,6 +3853,7 @@ int amd_iommu_activate_guest_mode(void *data) > > entry->hi.fields.ga_root_ptr = ir_data->ga_root_ptr; > > entry->hi.fields.vector = ir_data->ga_vector; > > entry->lo.fields_vapic.ga_tag = ir_data->ga_tag; > > + entry->lo.fields_remap.valid = 1; > > > > return modify_irte_ga(ir_data->irq_2_irte.devid, > > ir_data->irq_2_irte.index, entry, ir_data); > > > > Could you please try with the following patch instead? > > --- a/drivers/iommu/amd/iommu.c > +++ b/drivers/iommu/amd/iommu.c > @@ -3840,14 +3840,18 @@ int amd_iommu_activate_guest_mode(void *data) > { > struct amd_ir_data *ir_data = (struct amd_ir_data *)data; > struct irte_ga *entry = (struct irte_ga *) ir_data->entry; > + u64 valid; > > if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || > !entry || entry->lo.fields_vapic.guest_mode) > return 0; > > + valid = entry->lo.fields_vapic.valid; > + > entry->lo.val = 0; > entry->hi.val = 0; > > + entry->lo.fields_vapic.valid = valid; > entry->lo.fields_vapic.guest_mode = 1; > entry->lo.fields_vapic.ga_log_intr = 1; > entry->hi.fields.ga_root_ptr = ir_data->ga_root_ptr; > @@ -3864,12 +3868,14 @@ int amd_iommu_deactivate_guest_mode(void *data) > struct amd_ir_data *ir_data = (struct amd_ir_data *)data; > struct irte_ga *entry = (struct irte_ga *) ir_data->entry; > struct irq_cfg *cfg = ir_data->cfg; > - u64 valid = entry->lo.fields_remap.valid; > + u64 valid; > > if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || > !entry || !entry->lo.fields_vapic.guest_mode) > return 0; > > + valid = entry->lo.fields_remap.valid; > + > entry->lo.val = 0; > entry->hi.val = 0; I see. I based my approach on the fact that valid bit was set always to true anyway before, plus that amd_iommu_activate_guest_mode should be really only called when someone activates a valid interrupt remapping entry, but IMHO the approach of preserving the valid bit is safer anyway. It works on my system (I applied the patch manually, since either your or my email client, seems to mangle the patch) Thanks, Best regards, Maxim Levitsky > -- > > Thanks, > Suravee > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] iommu/amd: fix interrupt remapping for avic @ 2020-09-15 11:25 ` Maxim Levitsky 0 siblings, 0 replies; 12+ messages in thread From: Maxim Levitsky @ 2020-09-15 11:25 UTC (permalink / raw) To: Suravee Suthikulpanit, linux-kernel Cc: open list:AMD IOMMU (AMD-VI), Joao Martins On Mon, 2020-09-14 at 21:48 +0700, Suravee Suthikulpanit wrote: > Maxim, > > On 9/13/2020 7:42 PM, Maxim Levitsky wrote: > > Commit e52d58d54a32 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE") > > accidentally removed an assumption that modify_irte_ga always set the valid bit > > and amd_iommu_activate_guest_mode relied on that. > > > > Side effect of this is that on my machine, VFIO based VMs with AVIC enabled > > would eventually crash and show IOMMU errors like that: > > > > AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0055 address=0xfffffffdf8000000 flags=0x0008] > > > > Fixes: e52d58d54a321 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE") > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > drivers/iommu/amd/iommu.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c > > index 07ae8b93887e5..aff4cc1869356 100644 > > --- a/drivers/iommu/amd/iommu.c > > +++ b/drivers/iommu/amd/iommu.c > > @@ -3853,6 +3853,7 @@ int amd_iommu_activate_guest_mode(void *data) > > entry->hi.fields.ga_root_ptr = ir_data->ga_root_ptr; > > entry->hi.fields.vector = ir_data->ga_vector; > > entry->lo.fields_vapic.ga_tag = ir_data->ga_tag; > > + entry->lo.fields_remap.valid = 1; > > > > return modify_irte_ga(ir_data->irq_2_irte.devid, > > ir_data->irq_2_irte.index, entry, ir_data); > > > > Could you please try with the following patch instead? > > --- a/drivers/iommu/amd/iommu.c > +++ b/drivers/iommu/amd/iommu.c > @@ -3840,14 +3840,18 @@ int amd_iommu_activate_guest_mode(void *data) > { > struct amd_ir_data *ir_data = (struct amd_ir_data *)data; > struct irte_ga *entry = (struct irte_ga *) ir_data->entry; > + u64 valid; > > if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || > !entry || entry->lo.fields_vapic.guest_mode) > return 0; > > + valid = entry->lo.fields_vapic.valid; > + > entry->lo.val = 0; > entry->hi.val = 0; > > + entry->lo.fields_vapic.valid = valid; > entry->lo.fields_vapic.guest_mode = 1; > entry->lo.fields_vapic.ga_log_intr = 1; > entry->hi.fields.ga_root_ptr = ir_data->ga_root_ptr; > @@ -3864,12 +3868,14 @@ int amd_iommu_deactivate_guest_mode(void *data) > struct amd_ir_data *ir_data = (struct amd_ir_data *)data; > struct irte_ga *entry = (struct irte_ga *) ir_data->entry; > struct irq_cfg *cfg = ir_data->cfg; > - u64 valid = entry->lo.fields_remap.valid; > + u64 valid; > > if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || > !entry || !entry->lo.fields_vapic.guest_mode) > return 0; > > + valid = entry->lo.fields_remap.valid; > + > entry->lo.val = 0; > entry->hi.val = 0; I see. I based my approach on the fact that valid bit was set always to true anyway before, plus that amd_iommu_activate_guest_mode should be really only called when someone activates a valid interrupt remapping entry, but IMHO the approach of preserving the valid bit is safer anyway. It works on my system (I applied the patch manually, since either your or my email client, seems to mangle the patch) Thanks, Best regards, Maxim Levitsky > -- > > Thanks, > Suravee > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] iommu/amd: fix interrupt remapping for avic 2020-09-15 11:25 ` Maxim Levitsky @ 2020-09-15 12:30 ` Suravee Suthikulpanit -1 siblings, 0 replies; 12+ messages in thread From: Suravee Suthikulpanit @ 2020-09-15 12:30 UTC (permalink / raw) To: Maxim Levitsky, linux-kernel Cc: open list:AMD IOMMU (AMD-VI), Joao Martins, Joerg Roedel, Grimm, Jon On 9/15/20 6:25 PM, Maxim Levitsky wrote: > On Mon, 2020-09-14 at 21:48 +0700, Suravee Suthikulpanit wrote: >> Maxim, >> >> On 9/13/2020 7:42 PM, Maxim Levitsky wrote: >>> Commit e52d58d54a32 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE") >>> accidentally removed an assumption that modify_irte_ga always set the valid bit >>> and amd_iommu_activate_guest_mode relied on that. >>> >>> Side effect of this is that on my machine, VFIO based VMs with AVIC enabled >>> would eventually crash and show IOMMU errors like that: >>> >>> AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0055 address=0xfffffffdf8000000 flags=0x0008] >>> >>> Fixes: e52d58d54a321 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE") >>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> >>> --- >>> drivers/iommu/amd/iommu.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c >>> index 07ae8b93887e5..aff4cc1869356 100644 >>> --- a/drivers/iommu/amd/iommu.c >>> +++ b/drivers/iommu/amd/iommu.c >>> @@ -3853,6 +3853,7 @@ int amd_iommu_activate_guest_mode(void *data) >>> entry->hi.fields.ga_root_ptr = ir_data->ga_root_ptr; >>> entry->hi.fields.vector = ir_data->ga_vector; >>> entry->lo.fields_vapic.ga_tag = ir_data->ga_tag; >>> + entry->lo.fields_remap.valid = 1; >>> >>> return modify_irte_ga(ir_data->irq_2_irte.devid, >>> ir_data->irq_2_irte.index, entry, ir_data); >>> >> >> Could you please try with the following patch instead? >> >> --- a/drivers/iommu/amd/iommu.c >> +++ b/drivers/iommu/amd/iommu.c >> @@ -3840,14 +3840,18 @@ int amd_iommu_activate_guest_mode(void *data) >> { >> struct amd_ir_data *ir_data = (struct amd_ir_data *)data; >> struct irte_ga *entry = (struct irte_ga *) ir_data->entry; >> + u64 valid; >> >> if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || >> !entry || entry->lo.fields_vapic.guest_mode) >> return 0; >> >> + valid = entry->lo.fields_vapic.valid; >> + >> entry->lo.val = 0; >> entry->hi.val = 0; >> >> + entry->lo.fields_vapic.valid = valid; >> entry->lo.fields_vapic.guest_mode = 1; >> entry->lo.fields_vapic.ga_log_intr = 1; >> entry->hi.fields.ga_root_ptr = ir_data->ga_root_ptr; >> @@ -3864,12 +3868,14 @@ int amd_iommu_deactivate_guest_mode(void *data) >> struct amd_ir_data *ir_data = (struct amd_ir_data *)data; >> struct irte_ga *entry = (struct irte_ga *) ir_data->entry; >> struct irq_cfg *cfg = ir_data->cfg; >> - u64 valid = entry->lo.fields_remap.valid; >> + u64 valid; >> >> if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || >> !entry || !entry->lo.fields_vapic.guest_mode) >> return 0; >> >> + valid = entry->lo.fields_remap.valid; >> + >> entry->lo.val = 0; >> entry->hi.val = 0; > I see. I based my approach on the fact that valid bit was > set always to true anyway before, plus that amd_iommu_activate_guest_mode > should be really only called when someone activates a valid interrupt remapping > entry, but IMHO the approach of preserving the valid bit is safer anyway. > > It works on my system (I applied the patch manually, since either your or my email client, > seems to mangle the patch) > > Thanks, > Best regards, > Maxim Levitsky > > Sorry for the mangled patch. I'll submit the patch w/ your information. Thanks for your help reporting, debugging, and testing the patch. Sincerely, Suravee >> -- > >> >> Thanks, >> Suravee >> > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] iommu/amd: fix interrupt remapping for avic @ 2020-09-15 12:30 ` Suravee Suthikulpanit 0 siblings, 0 replies; 12+ messages in thread From: Suravee Suthikulpanit @ 2020-09-15 12:30 UTC (permalink / raw) To: Maxim Levitsky, linux-kernel Cc: open list:AMD IOMMU (AMD-VI), Grimm, Jon, Joao Martins On 9/15/20 6:25 PM, Maxim Levitsky wrote: > On Mon, 2020-09-14 at 21:48 +0700, Suravee Suthikulpanit wrote: >> Maxim, >> >> On 9/13/2020 7:42 PM, Maxim Levitsky wrote: >>> Commit e52d58d54a32 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE") >>> accidentally removed an assumption that modify_irte_ga always set the valid bit >>> and amd_iommu_activate_guest_mode relied on that. >>> >>> Side effect of this is that on my machine, VFIO based VMs with AVIC enabled >>> would eventually crash and show IOMMU errors like that: >>> >>> AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0055 address=0xfffffffdf8000000 flags=0x0008] >>> >>> Fixes: e52d58d54a321 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE") >>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> >>> --- >>> drivers/iommu/amd/iommu.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c >>> index 07ae8b93887e5..aff4cc1869356 100644 >>> --- a/drivers/iommu/amd/iommu.c >>> +++ b/drivers/iommu/amd/iommu.c >>> @@ -3853,6 +3853,7 @@ int amd_iommu_activate_guest_mode(void *data) >>> entry->hi.fields.ga_root_ptr = ir_data->ga_root_ptr; >>> entry->hi.fields.vector = ir_data->ga_vector; >>> entry->lo.fields_vapic.ga_tag = ir_data->ga_tag; >>> + entry->lo.fields_remap.valid = 1; >>> >>> return modify_irte_ga(ir_data->irq_2_irte.devid, >>> ir_data->irq_2_irte.index, entry, ir_data); >>> >> >> Could you please try with the following patch instead? >> >> --- a/drivers/iommu/amd/iommu.c >> +++ b/drivers/iommu/amd/iommu.c >> @@ -3840,14 +3840,18 @@ int amd_iommu_activate_guest_mode(void *data) >> { >> struct amd_ir_data *ir_data = (struct amd_ir_data *)data; >> struct irte_ga *entry = (struct irte_ga *) ir_data->entry; >> + u64 valid; >> >> if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || >> !entry || entry->lo.fields_vapic.guest_mode) >> return 0; >> >> + valid = entry->lo.fields_vapic.valid; >> + >> entry->lo.val = 0; >> entry->hi.val = 0; >> >> + entry->lo.fields_vapic.valid = valid; >> entry->lo.fields_vapic.guest_mode = 1; >> entry->lo.fields_vapic.ga_log_intr = 1; >> entry->hi.fields.ga_root_ptr = ir_data->ga_root_ptr; >> @@ -3864,12 +3868,14 @@ int amd_iommu_deactivate_guest_mode(void *data) >> struct amd_ir_data *ir_data = (struct amd_ir_data *)data; >> struct irte_ga *entry = (struct irte_ga *) ir_data->entry; >> struct irq_cfg *cfg = ir_data->cfg; >> - u64 valid = entry->lo.fields_remap.valid; >> + u64 valid; >> >> if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || >> !entry || !entry->lo.fields_vapic.guest_mode) >> return 0; >> >> + valid = entry->lo.fields_remap.valid; >> + >> entry->lo.val = 0; >> entry->hi.val = 0; > I see. I based my approach on the fact that valid bit was > set always to true anyway before, plus that amd_iommu_activate_guest_mode > should be really only called when someone activates a valid interrupt remapping > entry, but IMHO the approach of preserving the valid bit is safer anyway. > > It works on my system (I applied the patch manually, since either your or my email client, > seems to mangle the patch) > > Thanks, > Best regards, > Maxim Levitsky > > Sorry for the mangled patch. I'll submit the patch w/ your information. Thanks for your help reporting, debugging, and testing the patch. Sincerely, Suravee >> -- > >> >> Thanks, >> Suravee >> > > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] iommu/amd: fix interrupt remapping for avic 2020-09-15 12:30 ` Suravee Suthikulpanit @ 2020-09-15 13:19 ` Joao Martins -1 siblings, 0 replies; 12+ messages in thread From: Joao Martins @ 2020-09-15 13:19 UTC (permalink / raw) To: Suravee Suthikulpanit Cc: Maxim Levitsky, linux-kernel, iommu, Joerg Roedel, Grimm, Jon On 9/15/20 1:30 PM, Suravee Suthikulpanit wrote: > On 9/15/20 6:25 PM, Maxim Levitsky wrote: >> On Mon, 2020-09-14 at 21:48 +0700, Suravee Suthikulpanit wrote: >>> Could you please try with the following patch instead? >>> >>> --- a/drivers/iommu/amd/iommu.c >>> +++ b/drivers/iommu/amd/iommu.c >>> @@ -3840,14 +3840,18 @@ int amd_iommu_activate_guest_mode(void *data) >>> { >>> struct amd_ir_data *ir_data = (struct amd_ir_data *)data; >>> struct irte_ga *entry = (struct irte_ga *) ir_data->entry; >>> + u64 valid; >>> >>> if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || >>> !entry || entry->lo.fields_vapic.guest_mode) >>> return 0; >>> >>> + valid = entry->lo.fields_vapic.valid; >>> + >>> entry->lo.val = 0; >>> entry->hi.val = 0; >>> >>> + entry->lo.fields_vapic.valid = valid; >>> entry->lo.fields_vapic.guest_mode = 1; >>> entry->lo.fields_vapic.ga_log_intr = 1; >>> entry->hi.fields.ga_root_ptr = ir_data->ga_root_ptr; >>> @@ -3864,12 +3868,14 @@ int amd_iommu_deactivate_guest_mode(void *data) >>> struct amd_ir_data *ir_data = (struct amd_ir_data *)data; >>> struct irte_ga *entry = (struct irte_ga *) ir_data->entry; >>> struct irq_cfg *cfg = ir_data->cfg; >>> - u64 valid = entry->lo.fields_remap.valid; >>> + u64 valid; >>> >>> if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || >>> !entry || !entry->lo.fields_vapic.guest_mode) >>> return 0; >>> >>> + valid = entry->lo.fields_remap.valid; >>> + >>> entry->lo.val = 0; >>> entry->hi.val = 0; >> I see. I based my approach on the fact that valid bit was >> set always to true anyway before, plus that amd_iommu_activate_guest_mode >> should be really only called when someone activates a valid interrupt remapping >> entry, but IMHO the approach of preserving the valid bit is safer anyway. >> >> It works on my system (I applied the patch manually, since either your or my email client, >> seems to mangle the patch) >> > > Sorry for the mangled patch. I'll submit the patch w/ your information. Thanks for your help reporting, debugging, and > testing the patch. > I assume you're only doing the valid bit preservation in amd_iommu_activate_guest_mode() ? The null deref fix in amd_iommu_deactivate_guest_mode() was fixed elsewhere[0], or are you planning on merging both changes like the diff you attached? Asking also because commit 26e495f341 ("iommu/amd: Restore IRTE.RemapEn bit after programming IRTE") was added in v5.4 and v5.8 stable trees but the v5.4 backport didn't include e52d58d54a321 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE"). Joao [0] https://lore.kernel.org/linux-iommu/20200910171621.12879-1-joao.m.martins@oracle.com/ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] iommu/amd: fix interrupt remapping for avic @ 2020-09-15 13:19 ` Joao Martins 0 siblings, 0 replies; 12+ messages in thread From: Joao Martins @ 2020-09-15 13:19 UTC (permalink / raw) To: Suravee Suthikulpanit; +Cc: iommu, Grimm, Jon, linux-kernel, Maxim Levitsky On 9/15/20 1:30 PM, Suravee Suthikulpanit wrote: > On 9/15/20 6:25 PM, Maxim Levitsky wrote: >> On Mon, 2020-09-14 at 21:48 +0700, Suravee Suthikulpanit wrote: >>> Could you please try with the following patch instead? >>> >>> --- a/drivers/iommu/amd/iommu.c >>> +++ b/drivers/iommu/amd/iommu.c >>> @@ -3840,14 +3840,18 @@ int amd_iommu_activate_guest_mode(void *data) >>> { >>> struct amd_ir_data *ir_data = (struct amd_ir_data *)data; >>> struct irte_ga *entry = (struct irte_ga *) ir_data->entry; >>> + u64 valid; >>> >>> if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || >>> !entry || entry->lo.fields_vapic.guest_mode) >>> return 0; >>> >>> + valid = entry->lo.fields_vapic.valid; >>> + >>> entry->lo.val = 0; >>> entry->hi.val = 0; >>> >>> + entry->lo.fields_vapic.valid = valid; >>> entry->lo.fields_vapic.guest_mode = 1; >>> entry->lo.fields_vapic.ga_log_intr = 1; >>> entry->hi.fields.ga_root_ptr = ir_data->ga_root_ptr; >>> @@ -3864,12 +3868,14 @@ int amd_iommu_deactivate_guest_mode(void *data) >>> struct amd_ir_data *ir_data = (struct amd_ir_data *)data; >>> struct irte_ga *entry = (struct irte_ga *) ir_data->entry; >>> struct irq_cfg *cfg = ir_data->cfg; >>> - u64 valid = entry->lo.fields_remap.valid; >>> + u64 valid; >>> >>> if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || >>> !entry || !entry->lo.fields_vapic.guest_mode) >>> return 0; >>> >>> + valid = entry->lo.fields_remap.valid; >>> + >>> entry->lo.val = 0; >>> entry->hi.val = 0; >> I see. I based my approach on the fact that valid bit was >> set always to true anyway before, plus that amd_iommu_activate_guest_mode >> should be really only called when someone activates a valid interrupt remapping >> entry, but IMHO the approach of preserving the valid bit is safer anyway. >> >> It works on my system (I applied the patch manually, since either your or my email client, >> seems to mangle the patch) >> > > Sorry for the mangled patch. I'll submit the patch w/ your information. Thanks for your help reporting, debugging, and > testing the patch. > I assume you're only doing the valid bit preservation in amd_iommu_activate_guest_mode() ? The null deref fix in amd_iommu_deactivate_guest_mode() was fixed elsewhere[0], or are you planning on merging both changes like the diff you attached? Asking also because commit 26e495f341 ("iommu/amd: Restore IRTE.RemapEn bit after programming IRTE") was added in v5.4 and v5.8 stable trees but the v5.4 backport didn't include e52d58d54a321 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE"). Joao [0] https://lore.kernel.org/linux-iommu/20200910171621.12879-1-joao.m.martins@oracle.com/ _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] iommu/amd: fix interrupt remapping for avic 2020-09-15 13:19 ` Joao Martins @ 2020-09-16 10:38 ` Suravee Suthikulpanit -1 siblings, 0 replies; 12+ messages in thread From: Suravee Suthikulpanit @ 2020-09-16 10:38 UTC (permalink / raw) To: Joao Martins Cc: Maxim Levitsky, linux-kernel, iommu, Joerg Roedel, Grimm, Jon On 9/15/20 8:19 PM, Joao Martins wrote: > On 9/15/20 1:30 PM, Suravee Suthikulpanit wrote: >> On 9/15/20 6:25 PM, Maxim Levitsky wrote: >>> On Mon, 2020-09-14 at 21:48 +0700, Suravee Suthikulpanit wrote: >>>> Could you please try with the following patch instead? >>>> >>>> --- a/drivers/iommu/amd/iommu.c >>>> +++ b/drivers/iommu/amd/iommu.c >>>> @@ -3840,14 +3840,18 @@ int amd_iommu_activate_guest_mode(void *data) >>>> { >>>> struct amd_ir_data *ir_data = (struct amd_ir_data *)data; >>>> struct irte_ga *entry = (struct irte_ga *) ir_data->entry; >>>> + u64 valid; >>>> >>>> if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || >>>> !entry || entry->lo.fields_vapic.guest_mode) >>>> return 0; >>>> >>>> + valid = entry->lo.fields_vapic.valid; >>>> + >>>> entry->lo.val = 0; >>>> entry->hi.val = 0; >>>> >>>> + entry->lo.fields_vapic.valid = valid; >>>> entry->lo.fields_vapic.guest_mode = 1; >>>> entry->lo.fields_vapic.ga_log_intr = 1; >>>> entry->hi.fields.ga_root_ptr = ir_data->ga_root_ptr; >>>> @@ -3864,12 +3868,14 @@ int amd_iommu_deactivate_guest_mode(void *data) >>>> struct amd_ir_data *ir_data = (struct amd_ir_data *)data; >>>> struct irte_ga *entry = (struct irte_ga *) ir_data->entry; >>>> struct irq_cfg *cfg = ir_data->cfg; >>>> - u64 valid = entry->lo.fields_remap.valid; >>>> + u64 valid; >>>> >>>> if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || >>>> !entry || !entry->lo.fields_vapic.guest_mode) >>>> return 0; >>>> >>>> + valid = entry->lo.fields_remap.valid; >>>> + >>>> entry->lo.val = 0; >>>> entry->hi.val = 0; >>> I see. I based my approach on the fact that valid bit was >>> set always to true anyway before, plus that amd_iommu_activate_guest_mode >>> should be really only called when someone activates a valid interrupt remapping >>> entry, but IMHO the approach of preserving the valid bit is safer anyway. >>> >>> It works on my system (I applied the patch manually, since either your or my email client, >>> seems to mangle the patch) >>> >> >> Sorry for the mangled patch. I'll submit the patch w/ your information. Thanks for your help reporting, debugging, and >> testing the patch. >> > I assume you're only doing the valid bit preservation in amd_iommu_activate_guest_mode() ? > The null deref fix in amd_iommu_deactivate_guest_mode() was fixed elsewhere[0], or are you > planning on merging both changes like the diff you attached? I am planning to send a separate patch just for amd_iommu_activate_guest_mode(). > Asking also because commit 26e495f341 ("iommu/amd: Restore IRTE.RemapEn bit after > programming IRTE") was added in v5.4 and v5.8 stable trees but the v5.4 backport didn't > include e52d58d54a321 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE"). We should probably backport the e52d58d54a321 along with the fixes in amd_iommu_activate_guest_mode() and amd_iommu_deactivate_guest_mode(). I'll work with the community to get these back-ported. Thanks, Suravee ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] iommu/amd: fix interrupt remapping for avic @ 2020-09-16 10:38 ` Suravee Suthikulpanit 0 siblings, 0 replies; 12+ messages in thread From: Suravee Suthikulpanit @ 2020-09-16 10:38 UTC (permalink / raw) To: Joao Martins; +Cc: iommu, Grimm, Jon, linux-kernel, Maxim Levitsky On 9/15/20 8:19 PM, Joao Martins wrote: > On 9/15/20 1:30 PM, Suravee Suthikulpanit wrote: >> On 9/15/20 6:25 PM, Maxim Levitsky wrote: >>> On Mon, 2020-09-14 at 21:48 +0700, Suravee Suthikulpanit wrote: >>>> Could you please try with the following patch instead? >>>> >>>> --- a/drivers/iommu/amd/iommu.c >>>> +++ b/drivers/iommu/amd/iommu.c >>>> @@ -3840,14 +3840,18 @@ int amd_iommu_activate_guest_mode(void *data) >>>> { >>>> struct amd_ir_data *ir_data = (struct amd_ir_data *)data; >>>> struct irte_ga *entry = (struct irte_ga *) ir_data->entry; >>>> + u64 valid; >>>> >>>> if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || >>>> !entry || entry->lo.fields_vapic.guest_mode) >>>> return 0; >>>> >>>> + valid = entry->lo.fields_vapic.valid; >>>> + >>>> entry->lo.val = 0; >>>> entry->hi.val = 0; >>>> >>>> + entry->lo.fields_vapic.valid = valid; >>>> entry->lo.fields_vapic.guest_mode = 1; >>>> entry->lo.fields_vapic.ga_log_intr = 1; >>>> entry->hi.fields.ga_root_ptr = ir_data->ga_root_ptr; >>>> @@ -3864,12 +3868,14 @@ int amd_iommu_deactivate_guest_mode(void *data) >>>> struct amd_ir_data *ir_data = (struct amd_ir_data *)data; >>>> struct irte_ga *entry = (struct irte_ga *) ir_data->entry; >>>> struct irq_cfg *cfg = ir_data->cfg; >>>> - u64 valid = entry->lo.fields_remap.valid; >>>> + u64 valid; >>>> >>>> if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || >>>> !entry || !entry->lo.fields_vapic.guest_mode) >>>> return 0; >>>> >>>> + valid = entry->lo.fields_remap.valid; >>>> + >>>> entry->lo.val = 0; >>>> entry->hi.val = 0; >>> I see. I based my approach on the fact that valid bit was >>> set always to true anyway before, plus that amd_iommu_activate_guest_mode >>> should be really only called when someone activates a valid interrupt remapping >>> entry, but IMHO the approach of preserving the valid bit is safer anyway. >>> >>> It works on my system (I applied the patch manually, since either your or my email client, >>> seems to mangle the patch) >>> >> >> Sorry for the mangled patch. I'll submit the patch w/ your information. Thanks for your help reporting, debugging, and >> testing the patch. >> > I assume you're only doing the valid bit preservation in amd_iommu_activate_guest_mode() ? > The null deref fix in amd_iommu_deactivate_guest_mode() was fixed elsewhere[0], or are you > planning on merging both changes like the diff you attached? I am planning to send a separate patch just for amd_iommu_activate_guest_mode(). > Asking also because commit 26e495f341 ("iommu/amd: Restore IRTE.RemapEn bit after > programming IRTE") was added in v5.4 and v5.8 stable trees but the v5.4 backport didn't > include e52d58d54a321 ("iommu/amd: Use cmpxchg_double() when updating 128-bit IRTE"). We should probably backport the e52d58d54a321 along with the fixes in amd_iommu_activate_guest_mode() and amd_iommu_deactivate_guest_mode(). I'll work with the community to get these back-ported. Thanks, Suravee _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-09-16 21:08 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-13 12:42 [PATCH] iommu/amd: fix interrupt remapping for avic Maxim Levitsky 2020-09-13 12:42 ` Maxim Levitsky 2020-09-14 14:48 ` Suravee Suthikulpanit 2020-09-14 14:48 ` Suravee Suthikulpanit 2020-09-15 11:25 ` Maxim Levitsky 2020-09-15 11:25 ` Maxim Levitsky 2020-09-15 12:30 ` Suravee Suthikulpanit 2020-09-15 12:30 ` Suravee Suthikulpanit 2020-09-15 13:19 ` Joao Martins 2020-09-15 13:19 ` Joao Martins 2020-09-16 10:38 ` Suravee Suthikulpanit 2020-09-16 10:38 ` Suravee Suthikulpanit
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.