iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
From: Maxim Levitsky <mlevitsk@redhat.com>
To: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	 linux-kernel@vger.kernel.org
Cc: "open list:AMD IOMMU \(AMD-VI\)"
	<iommu@lists.linux-foundation.org>,
	Joao Martins <joao.m.martins@oracle.com>
Subject: Re: [PATCH] iommu/amd: fix interrupt remapping for avic
Date: Tue, 15 Sep 2020 14:25:01 +0300	[thread overview]
Message-ID: <04a8ab5cb1f6662f72bcad856da3415d6d9b2593.camel@redhat.com> (raw)
In-Reply-To: <60856c61-062b-8d92-e565-38bd00855228@amd.com>

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

  reply	other threads:[~2020-09-15 11:25 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-13 12:42 Maxim Levitsky
2020-09-14 14:48 ` Suravee Suthikulpanit
2020-09-15 11:25   ` Maxim Levitsky [this message]
2020-09-15 12:30     ` Suravee Suthikulpanit
2020-09-15 13:19       ` Joao Martins
2020-09-16 10:38         ` Suravee Suthikulpanit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=04a8ab5cb1f6662f72bcad856da3415d6d9b2593.camel@redhat.com \
    --to=mlevitsk@redhat.com \
    --cc=iommu@lists.linux-foundation.org \
    --cc=joao.m.martins@oracle.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=suravee.suthikulpanit@amd.com \
    --subject='Re: [PATCH] iommu/amd: fix interrupt remapping for avic' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).