[PATCH] iommu/amd: flush not present cache in iommu_map_page

[PATCH] iommu/amd: flush not present cache in iommu_map_page

[Tom Murphy]

check if there is a not-present cache present and flush it if there is.

Signed-off-by: Tom Murphy <tmurphy@arista.com>
---
 drivers/iommu/amd_iommu.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index f7cdd2ab7f11..8ef43224aae0 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1636,6 +1636,12 @@ static int iommu_map_page(struct protection_domain *dom,
 		pte[i] = __pte;
 
 	update_domain(dom);
+	if (!dom->updated) {
+		if (unlikely(amd_iommu_np_cache)) {
+			domain_flush_pages(dom, bus_addr, page_size);
+			domain_flush_complete(dom);
+		}
+	}
 
 	/* Everything flushed out, free pages now */
 	free_page_list(freelist);
-- 
2.17.1

Re: [PATCH] iommu/amd: flush not present cache in iommu_map_page

[Christoph Hellwig]

On Wed, Apr 24, 2019 at 03:18:59PM +0100, Tom Murphy via iommu wrote:
> check if there is a not-present cache present and flush it if there is.
> 
> Signed-off-by: Tom Murphy <tmurphy@arista.com>
> ---
>  drivers/iommu/amd_iommu.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index f7cdd2ab7f11..8ef43224aae0 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -1636,6 +1636,12 @@ static int iommu_map_page(struct protection_domain *dom,
>  		pte[i] = __pte;
>  
>  	update_domain(dom);
> +	if (!dom->updated) {
> +		if (unlikely(amd_iommu_np_cache)) {
> +			domain_flush_pages(dom, bus_addr, page_size);
> +			domain_flush_complete(dom);
> +		}
> +	}

The two conditions can go into one if statement to make this a little
more clear.

And I'd really like to understand the unlikely - amd_iommu_np_cache
is set based on a hardware capability, so it seems rather odd to mark
it unlikely.  Dynamic branch prediction really should do the right thing
here usually.

Re: [PATCH] iommu/amd: flush not present cache in iommu_map_page

[Tom Murphy]

>The two conditions can go into one if statement to make this a little
>more clear.
Ah, yeah of course

>And I'd really like to understand the unlikely - amd_iommu_np_cache
>is set based on a hardware capability, so it seems rather odd to mark
>it unlikely.  Dynamic branch prediction really should do the right thing
>here usually.

Here is the commit which added it without any explanation:
https://github.com/torvalds/linux/commit/270cab2426cdc6307725e4f1f46ecf8ab8e69193

should we remove it seen as there's no explanation given ?


On Wed, Apr 24, 2019 at 3:32 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Wed, Apr 24, 2019 at 03:18:59PM +0100, Tom Murphy via iommu wrote:
> > check if there is a not-present cache present and flush it if there is.
> >
> > Signed-off-by: Tom Murphy <tmurphy@arista.com>
> > ---
> >  drivers/iommu/amd_iommu.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> > index f7cdd2ab7f11..8ef43224aae0 100644
> > --- a/drivers/iommu/amd_iommu.c
> > +++ b/drivers/iommu/amd_iommu.c
> > @@ -1636,6 +1636,12 @@ static int iommu_map_page(struct protection_domain *dom,
> >               pte[i] = __pte;
> >
> >       update_domain(dom);
> > +     if (!dom->updated) {
> > +             if (unlikely(amd_iommu_np_cache)) {
> > +                     domain_flush_pages(dom, bus_addr, page_size);
> > +                     domain_flush_complete(dom);
> > +             }
> > +     }
>
> The two conditions can go into one if statement to make this a little
> more clear.
>
> And I'd really like to understand the unlikely - amd_iommu_np_cache
> is set based on a hardware capability, so it seems rather odd to mark
> it unlikely.  Dynamic branch prediction really should do the right thing
> here usually.

Re: [PATCH] iommu/amd: flush not present cache in iommu_map_page

[Christoph Hellwig]

On Wed, Apr 24, 2019 at 03:47:39PM +0100, Tom Murphy wrote:
> >And I'd really like to understand the unlikely - amd_iommu_np_cache
> >is set based on a hardware capability, so it seems rather odd to mark
> >it unlikely.  Dynamic branch prediction really should do the right thing
> >here usually.
> 
> Here is the commit which added it without any explanation:
> https://github.com/torvalds/linux/commit/270cab2426cdc6307725e4f1f46ecf8ab8e69193
> 
> should we remove it seen as there's no explanation given ?

I'd be tempted to do that.  But lets just ask Joerg if he has
any opinion..

Re: [PATCH] iommu/amd: flush not present cache in iommu_map_page

[Joerg Roedel]

On Wed, Apr 24, 2019 at 07:58:19AM -0700, Christoph Hellwig wrote:
> I'd be tempted to do that.  But lets just ask Joerg if he has
> any opinion..

The reason was that it is an unlikely path, as hardware implementations
are not allowed to set this bit. It is purely for emulated AMD IOMMUs.
I have not measured whether this annotation has any performance
benefit, but I find it more readable at least.

Regards,

	Joerg

PS: Why did you drop me from the Cc list of the previous replies?

Re: [PATCH] iommu/amd: flush not present cache in iommu_map_page

[Tom Murphy]

On Wed, Apr 24, 2019 at 4:55 PM Joerg Roedel <joro@8bytes.org> wrote:
>
> On Wed, Apr 24, 2019 at 07:58:19AM -0700, Christoph Hellwig wrote:
> > I'd be tempted to do that.  But lets just ask Joerg if he has
> > any opinion..
>
> The reason was that it is an unlikely path, as hardware implementations
> are not allowed to set this bit. It is purely for emulated AMD IOMMUs.
> I have not measured whether this annotation has any performance
> benefit, but I find it more readable at least.

In that case I will keep it in.

>
> Regards,
>
>         Joerg
>
> PS: Why did you drop me from the Cc list of the previous replies?