Re: [edk2-devel] [PATCH 3/4] OvmfPkg: create a SEV secret area in the AmdSev memfd

[jejb@linux.ibm.com]

On Thu, 2020-11-19 at 13:41 -0600, Brijesh Singh wrote:
> On 11/19/20 1:50 AM, Laszlo Ersek wrote:
> > On 11/18/20 21:23, James Bottomley wrote:
> > > On Mon, 2020-11-16 at 23:46 +0100, Laszlo Ersek wrote:
> > > > On 11/12/20 01:13, James Bottomley wrote:
> > > [...  I made all the changes above this]
> > > > > diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> > > > > b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> > > > > index 980e0138e7..7d3214e55d 100644
> > > > > --- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> > > > > +++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
> > > > > @@ -35,6 +35,8 @@ ALIGN   16
> > > > >  ;   the build time RIP value. The GUID must always be 48
> > > > > bytes
> > > > > from the
> > > > >  ;   end of the firmware.
> > > > >  ;
> > > > > +;   0xffffffc2 (-0x3e) - Base Location of the SEV Launch
> > > > > Secret
> > > > > +;   0xffffffc6 (-0x3a) - Size of SEV Launch Secret
> > > > >  ;   0xffffffca (-0x36) - IP value
> > > > >  ;   0xffffffcc (-0x34) - CS segment base [31:16]
> > > > >  ;   0xffffffce (-0x32) - Size of the SEV-ES reset block
> > > > > @@ -51,6 +53,8 @@ ALIGN   16
> > > > >  TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB
> > > > > 0
> > > > > 
> > > > >  sevEsResetBlockStart:
> > > > > +    DD      SEV_LAUNCH_SECRET_BASE
> > > > > +    DD      SEV_LAUNCH_SECRET_SIZE
> > > > >      DD      SEV_ES_AP_RESET_IP
> > > > >      DW      sevEsResetBlockEnd - sevEsResetBlockStart
> > > > >      DB      0xDE, 0x71, 0xF7, 0x00, 0x7E, 0x1A, 0xCB, 0x4F
> > > > (5) I'd prefer if we could introduce a new GUID-ed structure
> > > > for these new fields. The logic in QEMU should be extended to
> > > > start scanning at 4GB-48 for GUIDS. If the GUID is not
> > > > recognized, then terminate scanning. Otherwise, act upon the
> > > > GUID-ed structure found there as necessary, and then determine
> > > > the next GUID *candidate* location by subtracting the last
> > > > recognized GUID-ed structure's "size" field.
> > > So for this one, we can do it either way.  However, the current
> > > design of the sevEsRestBlock is (according to AMD) to allow the
> > > addition of SEV specific information.  Each piece of information
> > > is a specific offset from the GUID and the length of the
> > > structure can only grow, so the ordering is fixed once the info
> > > is added and you can tell if the section contains what you're
> > > looking for is present if the length covers it.
> > > 
> > > We can certainly move this to a fully GUID based system, which
> > > would allow us to have an unordered list rather than the strict
> > > definition the never decreasing length scheme allows, but if we
> > > do that, the length word above becomes redundant.
> > Well, GUIDed structs in UEFI/PI are sometimes permitted to grow
> > compatibily, and for that, either a revision field or a size field
> > is necessary / used. I kind of desire both here -- it makes sense
> > to extend (for example) the SEV-ES reset block with relevant
> > information, and to add other blocks of information (identified
> > with different GUIDs).
> > 
> > Basically I wouldn't want to finalize the SEV-ES AP reset block
> > just yet, *but* I also think this new information does not beloing
> > in the SEV-ES *AP reset block*. The new info is related to SEV-ES
> > alright, but not to the AP reset block, in my opinion. If you read
> > the larger context (the docs) in the assembly source around
> > "sevEsResetBlockStart", the launch secret just doesn't seem to fit
> > that.
> > 
> > > I don't have a huge preference for either mechanism ... they seem
> > > to work equally well, but everyone should agree before I replace
> > > the length based scheme. I agree we should all agree about it
> > > first.
> > 
> > And, to reiterate, I'd like to keep both the length fields and the
> > GUID-ed identification. In other words, a GUID should not imply an
> > exact struct size, just a minimum struct size.
> 
> I agree with the GUID based approach, it aligns well with the future
> needs. Looking forwardm we will need to reserve couple of pages
> (secret and cpuid) for the SNP. In my WIP patches I extended reset
> block to define a new GUID for those new fields.
> 
> https://github.com/AMDESE/ovmf/commit/87d47319411763d91219b377da709efdb057e662#diff-0ca7ec2856c316694c87b519c95db3270e0cac798eb09745cce167aad7f2d46dR28
> 
> And I am using this qemu patch to iterate through all the GUIDs and
> call the corresponding callbacks.
> 
> https://github.com/AMDESE/qemu/commit/16a1266353d372cbb7c1998f27081fb8aa4d31e9

OK, if that's not yet upstream, I think we should do this properly:
that means having a guid and length that identifies the entire table
and then all the incorporated guids and lengths.  That way we don't
have a double meaning for the reset block guid as both identifying the
start of the table and the reset vector data.  Also it means we don't
need a zero guid to signal the end of the table.  And also means the
reset block GUID doesn't have to always be present (if it got
deprecated for some reason).

However, the downside is that you'll have to pull out the table by this
new guid at 0xffffffd0 and its length and then iterate over the table
to find the reset block guid ... but that will make it very easy to add
the additional guids.

James