Re: [PATCH] iommu/amd: remove amd_iommu_snp_enable

Re: [PATCH] iommu/amd: remove amd_iommu_snp_enable

[Kim Phillips]

+Mike Roth, Ashish

On 8/31/23 7:31 AM, Christoph Hellwig wrote:
> amd_iommu_snp_enable is unused and has been since it was added in commit
> fb2accadaa94 ("iommu/amd: Introduce function to check and enable SNP").
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

It is used by the forthcoming host SNP support:

https://lore.kernel.org/lkml/20230612042559.375660-8-michael.roth@amd.com/

Thanks,

Kim

Re: [PATCH] iommu/amd: remove amd_iommu_snp_enable

[Christoph Hellwig]

On Thu, Aug 31, 2023 at 01:03:53PM -0500, Kim Phillips wrote:
> +Mike Roth, Ashish
>
> On 8/31/23 7:31 AM, Christoph Hellwig wrote:
>> amd_iommu_snp_enable is unused and has been since it was added in commit
>> fb2accadaa94 ("iommu/amd: Introduce function to check and enable SNP").
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> ---
>
> It is used by the forthcoming host SNP support:
>
> https://lore.kernel.org/lkml/20230612042559.375660-8-michael.roth@amd.com/

Then resend it with that support, but don't waste resources and everyones
time now.

Re: [PATCH] iommu/amd: remove amd_iommu_snp_enable

[Jason Gunthorpe]

On Fri, Sep 01, 2023 at 07:50:20AM +0200, Christoph Hellwig wrote:
> On Thu, Aug 31, 2023 at 01:03:53PM -0500, Kim Phillips wrote:
> > +Mike Roth, Ashish
> >
> > On 8/31/23 7:31 AM, Christoph Hellwig wrote:
> >> amd_iommu_snp_enable is unused and has been since it was added in commit
> >> fb2accadaa94 ("iommu/amd: Introduce function to check and enable SNP").
> >>
> >> Signed-off-by: Christoph Hellwig <hch@lst.de>
> >> ---
> >
> > It is used by the forthcoming host SNP support:
> >
> > https://lore.kernel.org/lkml/20230612042559.375660-8-michael.roth@amd.com/
> 
> Then resend it with that support, but don't waste resources and everyones
> time now.

+1

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

I've said this many times lately. There are other things in this
driver that have no upstream justification too, like nesting
"support".

Please organize this SNP support into series that makes sense and are
self complete :( I'm not sure a 51 patch series is a productive way to
approach this..

Jason

Re: [PATCH] iommu/amd: remove amd_iommu_snp_enable

[Suthikulpanit, Suravee]

On 9/6/2023 8:36 PM, Jason Gunthorpe wrote:
> On Fri, Sep 01, 2023 at 07:50:20AM +0200, Christoph Hellwig wrote:
>> On Thu, Aug 31, 2023 at 01:03:53PM -0500, Kim Phillips wrote:
>>> +Mike Roth, Ashish
>>>
>>> On 8/31/23 7:31 AM, Christoph Hellwig wrote:
>>>> amd_iommu_snp_enable is unused and has been since it was added in commit
>>>> fb2accadaa94 ("iommu/amd: Introduce function to check and enable SNP").
>>>>
>>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>>> ---
>>>
>>> It is used by the forthcoming host SNP support:
>>>
>>> https://lore.kernel.org/lkml/20230612042559.375660-8-michael.roth@amd.com/
>>
>> Then resend it with that support, but don't waste resources and everyones
>> time now.
> 
> +1
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Duly noted :)

The reason we introduced this change separately from the SNP series was 
because it is part of a different subsystem, which normally get pulled 
in separately. Unfortunately the main series takes a long time to get 
into upstream Linux kernel. So, this code appears as unused, which I can 
see how it can be confusing to other developers.

FYI: This is just removing a function to enable SNP support in AMD IOMMU 
driver. The underlying logic to enable the hardware still exist, which 
is part of the SNP feature enablement of the AMD IOMMU driver.

Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

> I've said this many times lately. There are other things in this
> driver that have no upstream justification too, like nesting
> "support".

Jason, there is no need to keep repeating and polluting this thread. I 
am happy to discuss and clarify any points of your concern on the 
"nesting support" in a separate discussion thread :)

> Please organize this SNP support into series that makes sense and are
> self complete :( I'm not sure a 51 patch series is a productive way to
> approach this..

We have discussed with Michael Roth (the author of the series "Add AMD 
Secure Nested Paging (SEV-SNP) Hypervisor Support"​), and he will start 
including the removed function in the next revision of his series.

Regards,
Suravee

Re: [PATCH] iommu/amd: remove amd_iommu_snp_enable

[Jason Gunthorpe]

On Thu, Sep 07, 2023 at 04:55:52PM +0700, Suthikulpanit, Suravee wrote:

> > I've said this many times lately. There are other things in this
> > driver that have no upstream justification too, like nesting
> > "support".
> 
> Jason, there is no need to keep repeating and polluting this thread. I am
> happy to discuss and clarify any points of your concern on the "nesting
> support" in a separate discussion thread :)

It is not polluting this thread. We are, again, asking AMD to follow
the rules please.

None of the other threads about the nesting has resulted in acceptance
that code needs to be removed as well. Go respond in one of those if
you have something to add.

Jason