linux-kernel - Re: [PATCH v6 0/9] iommu/amd: Use 128-bit cmpxchg operation to update DTE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20241031113355.GC10193@nvidia.com>
Date: Thu, 31 Oct 2024 08:33:55 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: "Suthikulpanit, Suravee" <suravee.suthikulpanit@....com>
Cc: linux-kernel@...r.kernel.org, iommu@...ts.linux.dev, joro@...tes.org,
	robin.murphy@....com, vasant.hegde@....com, kevin.tian@...el.com,
	jon.grimm@....com, santosh.shukla@....com, pandoh@...gle.com,
	kumaranand@...gle.com
Subject: Re: [PATCH v6 0/9] iommu/amd: Use 128-bit cmpxchg operation to
 update DTE

On Thu, Oct 31, 2024 at 04:15:02PM +0700, Suthikulpanit, Suravee wrote:
> On 10/16/2024 9:22 PM, Jason Gunthorpe wrote:
> > 
> > ....
> > 
> > I wanted to see how far this was to being split up neatly like ARM is,
> > I came up with this, which seems pretty good to me. This would
> > probably be the next step to get to, then you'd lift the individual
> > set functions higher up the call chain into their respective attach
> > functions.
> 
> I like this idea and will look into adopting this code when I submit the
> nested translation stuff (right after this series) since it will affect the
> set_dte_entry().

Yes, I definitely want to see this kind of code structure before
nested translation.

> > static void set_dte_entry(struct amd_iommu *iommu,
> > 			  struct iommu_dev_data *dev_data)
> > {
> > 	u32 old_domid;
> > 	struct dev_table_entry new = {};
> > 	struct protection_domain *domain = dev_data->domain;
> > 	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
> > 	struct dev_table_entry *dte = &get_dev_table(iommu)[dev_data->devid];
> > 
> > 	make_clear_dte(dev_data, dte, &new);
> > 	if (gcr3_info && gcr3_info->gcr3_tbl)
> > 		set_dte_gcr3_table(iommu, dev_data, &new);
> > 	else if (domain->iop.mode == PAGE_MODE_NONE)
> > 		set_dte_identity(iommu, dev_data, &new);
> > 	else
> > 		set_dte_paging(iommu, dev_data, &new);
> 
> This will need to be change once we add nested translation support because
> we need to call both set_dte_paging() and set_dte_gcr3().

The idea would be to remove set_dte_entry() because the attach
functions just call their specific set_dte_xx() directly, like how arm
is structured.

That will make everything much clearer.

Then the nested attach function would call some set_dte_nested() and
it would use set_dte_paging() internally.

Getting to this level is necessary to get the hitless replace, which
is important..

I hope this series gets landed this cycle, next cycle you should try
to get to hitless replace on the domain path, including this stuff,
then adding the nested domain should be straightforward!

Jason