Message-ID:
 <SN6PR02MB4157B333B126909197590DE0D4E32@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Tue, 14 May 2024 21:34:59 +0000
From: Michael Kelley <mhklinux@...look.com>
To: "Suthikulpanit, Suravee" <suravee.suthikulpanit@....com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"iommu@...ts.linux.dev" <iommu@...ts.linux.dev>, "joro@...tes.org"
	<joro@...tes.org>
CC: "thomas.lendacky@....com" <thomas.lendacky@....com>,
	"vasant.hegde@....com" <vasant.hegde@....com>, "michael.roth@....com"
	<michael.roth@....com>, "jon.grimm@....com" <jon.grimm@....com>,
	"rientjes@...gle.com" <rientjes@...gle.com>
Subject: RE: [PATCH 0/9] iommu/amd: Add AMD IOMMU emulation support for
 SEV-SNP guest kernel

From: Suthikulpanit, Suravee <suravee.suthikulpanit@....com> Sent: Tuesday, May 14, 2024 12:02 PM
> 
> On 5/14/2024 3:05 AM, Michael Kelley wrote:
> > From: Suravee Suthikulpanit<suravee.suthikulpanit@....com>  Sent: Tuesday, April
> 30, 2024 8:24 AM
> >> To boot a VM w/ x2APIC ID > 255, guest interrupt remapping emulation
> >> is required.
> >
> > Top-level question:  Is there a reason the MSI extended destination ID mechanism is
> > insufficient to avoid the need for interrupt remapping?  (see function pointer
> > "msi_ext_dest_id").  I'm unclear on whether it is or not. If it is not sufficient, perhaps
> > you could explain why.
> 
> In case of running a Linux VM w/ QEMU/KVM as hypervisor, the
> qemu-system-x86_64 option kvm-msi-ext-dest-id=on would allow booting the
> VM w/ x2APIC ID > 255. However, for other hypervisor, it might not
> support this feature.

Two observations:
1) KVM, Hyper-V, and Xen all have support for MSI extended destination ID.
Is it reasonable to make it a requirement that any hypervisor supporting
SEV/SEV-ES/SEV-SNP guests must also support MSI extended destination ID
if the guest is to run with more than 255 vCPUs?  With that requirement,
you don't need any interrupt remapping or IOMMU emulation.

2) It would help to be more explicit about the basic premise of this patch
set.  I *think* the idea is that KVM/QEMU already supports an emulated
AMD IOMMU in a normal VM.  That emulation depends on QEMU having
access to data structures in guest memory (just like a physical AMD IOMMU
would).  But with SEV/SEV-ES/SEV-SNP, guest memory is encrypted and
QEMU doesn't have the access.  So this patch set changes the IOMMU
related data structures to be allocated in decrypted guest memory.  Of
course, as Jason has pointed out, this would seem to open the guest to
security risks from a compromised hypervisor/VMM, negating what
SEV* is trying to provide as guest confidentiality.

My #1 above might be a lot less risky from a security perspective.

> 
> >> For SEV guest, this can be achieved using an emulated
> >> AMD IOMMU.
> > You've used "SEV" here and in several other places.  I think you intend this to be
> > the more specific "SEV-SNP", and exclude SEV and SEV-ES. For avoid any confusion,
> > I'd suggest using "SEV-SNP" throughout if that's what you mean.
> 
> Actually, The CC_ATTR_GUEST_MEM_ENCRYPT attribute is true for all SEV
> guests, so this will enable IOMMU emulation for all SEV guests.
> 

If that's the case, I'd suggest updating the Subject: line of this cover letter
to not be specific to SEV-SNP, and to call out in the text that's your intent
for the patch set to work for all SEV* flavors.  Also, the commit messages
throughout the patch set sometimes reference "SNP" and sometimes "SEV".
That's confusing if your intent is applicability to all SEV* flavors.

Michael

Powered by blists - more mailing lists