On Tue, Nov 24, 2020 at 07:16:29PM +0000, Sean Christopherson wrote:
> On Fri, Nov 13, 2020, David Rientjes wrote:
>
> >
> >
> > On Mon, 2 Nov 2020, Sean Christopherson wrote:
> >
> >
> >
> > > On Fri, Oct 02, 2020 at 01:48:10PM -0700, Vipin Sharma wrote:
> > >
> > > > On Fri, Sep 25, 2020 at 03:22:20PM -0700, Vipin Sharma wrote:
> > > >
> > > > > I agree with you that the abstract name is better than the concrete
> > > > >
> > > > > name, I also feel that we must provide HW extensions. Here is one
> > > > >
> > > > > approach:
> > > > >
> > > > >
> > > > >
> > > > > Cgroup name: cpu_encryption, encryption_slots, or memcrypt (open to
> > > > >
> > > > > suggestions)
> > > > >
> > > > >
> > > > >
> > > > > Control files: slots.{max, current, events}
> > > > >
> > >
> > >
> > > I don't particularly like the "slots" name, mostly because it could be
> > > confused
> > > with KVM's memslots. Maybe encryption_ids.ids.{max, current, events}? I
> > > don't
> > > love those names either, but "encryption" and "IDs" are the two obvious
> > >
> > > commonalities betwee TDX's encryption key IDs and SEV's encryption
> > > address
> > > space IDs.
> > >
> > >
> > >
> >
> >
> > Looping Janosch and Christian back into the thread.
> >
> >
> >
> > I interpret this suggestion as
> >
> > encryption.{sev,sev_es,keyids}.{max,current,events} for AMD and Intel
> >
>
> I think it makes sense to use encryption_ids instead of simply encryption,
> that
> way it's clear the cgroup is accounting ids as opposed to restricting what
> techs can be used on yes/no basis.
>
> > offerings, which was my thought on this as well.
> >
> >
> >
> > Certainly the kernel could provide a single interface for all of these and
> >
> > key value pairs depending on the underlying encryption technology but it
> >
> > seems to only introduce additional complexity in the kernel in string
> >
> > parsing that can otherwise be avoided. I think we all agree that a single
> >
> > interface for all encryption keys or one-value-per-file could be done in
> >
> > the kernel and handled by any userspace agent that is configuring these
> >
> > values.
> >
> >
> >
> > I think Vipin is adding a root level file that describes how many keys we
> >
> > have available on the platform for each technology. So I think this comes
> >
> > down to, for example, a single encryption.max file vs
> >
> > encryption.{sev,sev_es,keyid}.max. SEV and SEV-ES ASIDs are provisioned
> >
>
> Are you suggesting that the cgroup omit "current" and "events"? I agree
> there's
> no need to enumerate platform total, but not knowing how many of the allowed
> IDs
> have been allocated seems problematic.
>
We will be showing encryption_ids.{sev,sev_es}.{max,current}
I am inclined to not provide "events" as I am not using it, let me know
if this file is required, I can provide it then.
I will provide an encryption_ids.{sev,sev_es}.stat file, which shows
total available ids on the platform. This one will be useful for
scheduling jobs in the cloud infrastructure based on total supported
capacity.
> > separately so we treat them as their own resource here.
> >
> >
> >
> > So which is easier?
> >
> >
> >
> > $ cat encryption.sev.max
> >
> > 10
> >
> > $ echo -n 15 > encryption.sev.max
> >
> >
> >
> > or
> >
> >
> >
> > $ cat encryption.max
> >
> > sev 10
> >
> > sev_es 10
> >
> > keyid 0
> >
> > $ echo -n "sev 10" > encryption.max
> >
> >
> >
> > I would argue the former is simplest (always preferring
> >
> > one-value-per-file) and avoids any string parsing or resource controller
> >
> > lookups that need to match on that string in the kernel.
> >
>
> Ya, I prefer individual files as well.
>
> I don't think "keyid" is the best name for TDX, it doesn't leave any wiggle
> room
> if there are other flavors of key IDs on Intel platform, e.g. private vs.
> shared
> in the future. It's also inconsistent with the SEV names, e.g. "asid" isn't
> mentioned anywhere. And "keyid" sort of reads as "max key id", rather than
> "max
> number of keyids". Maybe "tdx_private", or simply "tdx"? Doesn't have to be
> solved now though, there's plenty of time before TDX will be upstream. :-)
>
> > The set of encryption.{sev,sev_es,keyid} files that exist would depend on
> >
> > CONFIG_CGROUP_ENCRYPTION and whether CONFIG_AMD_MEM_ENCRYPT or
> >
> > CONFIG_INTEL_TDX is configured. Both can be configured so we have all
> >
> > three files, but the root file will obviously indicate 0 keys available
> >
> > for one of them (can't run on AMD and Intel at the same time :).
> >
> >
> >
> > So I'm inclined to suggest that the one-value-per-file format is the ideal
> >
> > way to go unless there are objections to it.
