On 7/19/21 12:18 PM, Sean Christopherson wrote:
>>
>> Okay, I will add helper to make things easier. One case where we will
>> need to directly call the rmpupdate() is during the LAUNCH_UPDATE
>> command. In that case the page is private and its immutable bit is also
>> set. This is because the firmware makes change to the page, and we are
>> required to set the immutable bit before the call.
>
> Or do "int rmp_make_firmware(u64 pfn, bool immutable)"?
>
That's not what we need.
We need 'rmp_make_private() + immutable' all in one RMPUPDATE. Here is
the snippet from SNP_LAUNCH_UPDATE.
+ /* Transition the page state to pre-guest */
+ memset(&e, 0, sizeof(e));
+ e.assigned = 1;
+ e.gpa = gpa;
+ e.asid = sev_get_asid(kvm);
+ e.immutable = true;
+ e.pagesize = X86_TO_RMP_PG_LEVEL(level);
+ ret = rmpupdate(inpages[i], &e);
thanks
On Mon, Jul 19, 2021, Brijesh Singh wrote:
>
> On 7/19/21 12:18 PM, Sean Christopherson wrote:
> > >
> > > Okay, I will add helper to make things easier. One case where we will
> > > need to directly call the rmpupdate() is during the LAUNCH_UPDATE
> > > command. In that case the page is private and its immutable bit is also
> > > set. This is because the firmware makes change to the page, and we are
> > > required to set the immutable bit before the call.
> >
> > Or do "int rmp_make_firmware(u64 pfn, bool immutable)"?
>
> That's not what we need.
>
> We need 'rmp_make_private() + immutable' all in one RMPUPDATE. Here is the
> snippet from SNP_LAUNCH_UPDATE.
Ah, not firmwrare, gotcha. But we can still use a helper, e.g. an inner
double-underscore helper, __rmp_make_private().
On Mon, Jul 19, 2021, Sean Christopherson wrote:
> On Mon, Jul 19, 2021, Brijesh Singh wrote:
> >
> > On 7/19/21 12:18 PM, Sean Christopherson wrote:
> > > >
> > > > Okay, I will add helper to make things easier. One case where we will
> > > > need to directly call the rmpupdate() is during the LAUNCH_UPDATE
> > > > command. In that case the page is private and its immutable bit is also
> > > > set. This is because the firmware makes change to the page, and we are
> > > > required to set the immutable bit before the call.
> > >
> > > Or do "int rmp_make_firmware(u64 pfn, bool immutable)"?
> >
> > That's not what we need.
> >
> > We need 'rmp_make_private() + immutable' all in one RMPUPDATE. Here is the
> > snippet from SNP_LAUNCH_UPDATE.
>
> Ah, not firmwrare, gotcha. But we can still use a helper, e.g. an inner
> double-underscore helper, __rmp_make_private().
Hmm, looking at it again, I think I also got confused by the comment for the VMSA
page:
/* Transition the VMSA page to a firmware state. */
e.assigned = 1;
e.immutable = 1;
e.asid = sev->asid;
e.gpa = -1;
e.pagesize = RMP_PG_SIZE_4K;
Unlike __snp_alloc_firmware_pages() in the CCP code, the VMSA is associated with
the guest's ASID, just not a GPA. I.e. the VMSA is more of a specialized guest
private page, as opposed to a dedicated firmware page. I.e. a __rmp_make_private()
and/or rmp_make_private_immutable() definitely seems like a good idea.
On 7/19/21 2:03 PM, Sean Christopherson wrote:
> On Mon, Jul 19, 2021, Brijesh Singh wrote:
>>
>> On 7/19/21 12:18 PM, Sean Christopherson wrote:
>>>>
>>>> Okay, I will add helper to make things easier. One case where we will
>>>> need to directly call the rmpupdate() is during the LAUNCH_UPDATE
>>>> command. In that case the page is private and its immutable bit is also
>>>> set. This is because the firmware makes change to the page, and we are
>>>> required to set the immutable bit before the call.
>>>
>>> Or do "int rmp_make_firmware(u64 pfn, bool immutable)"?
>>
>> That's not what we need.
>>
>> We need 'rmp_make_private() + immutable' all in one RMPUPDATE. Here is the
>> snippet from SNP_LAUNCH_UPDATE.
>
> Ah, not firmwrare, gotcha. But we can still use a helper, e.g. an inner
> double-underscore helper, __rmp_make_private().
>
In that case we are basically passing the all the fields defined in the
'struct rmpupdate' as individual arguments. How about something like this:
* core kernel exports the rmpupdate()
* the include/linux/sev.h header file defines the helper functions
int rmp_make_private(u64 pfn, u64 gpa, int psize, int asid)
int rmp_make_firmware(u64 pfn, int psize);
int rmp_make_shared(u64 pfn, int psize);
In most of the case above 3 helpers are good. If driver finds that the
above helper does not fit its need (such as SNP_LAUNCH_UPDATE) then call
the rmpupdate() without going through the helper.
-Brijesh
On Mon, Jul 19, 2021, Brijesh Singh wrote:
>
> On 7/19/21 2:03 PM, Sean Christopherson wrote:
> > On Mon, Jul 19, 2021, Brijesh Singh wrote:
> > Ah, not firmwrare, gotcha. But we can still use a helper, e.g. an inner
> > double-underscore helper, __rmp_make_private().
>
> In that case we are basically passing the all the fields defined in the
> 'struct rmpupdate' as individual arguments.
Yes, but (a) not _all_ fields, (b) it would allow hiding "struct rmpupdate", and
(c) this is much friendlier to readers:
__rmp_make_private(pfn, gpa, PG_LEVEL_4K, svm->asid, true);
than:
rmpupdate(&rmpupdate);
For the former, I can see in a single line of code that KVM is creating a 4k
private, immutable guest page. With the latter, I need to go hunt down all code
that modifies rmpupdate to understand what the code is doing.
> How about something like this:
>
> * core kernel exports the rmpupdate()
> * the include/linux/sev.h header file defines the helper functions
>
> int rmp_make_private(u64 pfn, u64 gpa, int psize, int asid)
I think we'll want s/psize/level, i.e. make it more obvious clear that the input
is PG_LEVEL_*.
On 7/20/21 11:40 AM, Sean Christopherson wrote:
> On Mon, Jul 19, 2021, Brijesh Singh wrote:
>>
>> On 7/19/21 2:03 PM, Sean Christopherson wrote:
>>> On Mon, Jul 19, 2021, Brijesh Singh wrote:
>>> Ah, not firmwrare, gotcha. But we can still use a helper, e.g. an inner
>>> double-underscore helper, __rmp_make_private().
>>
>> In that case we are basically passing the all the fields defined in the
>> 'struct rmpupdate' as individual arguments.
>
> Yes, but (a) not _all_ fields, (b) it would allow hiding "struct rmpupdate", and
> (c) this is much friendlier to readers:
>
> __rmp_make_private(pfn, gpa, PG_LEVEL_4K, svm->asid, true);
>
> than:
>
> rmpupdate(&rmpupdate);
>
Ok.
> For the former, I can see in a single line of code that KVM is creating a 4k
> private, immutable guest page. With the latter, I need to go hunt down all code
> that modifies rmpupdate to understand what the code is doing.
>
>> How about something like this:
>>
>> * core kernel exports the rmpupdate()
>> * the include/linux/sev.h header file defines the helper functions
>>
>> int rmp_make_private(u64 pfn, u64 gpa, int psize, int asid)
>
> I think we'll want s/psize/level, i.e. make it more obvious clear that the input
> is PG_LEVEL_*.
>
ok, I will stick to x86 PG_LEVEL_*
thanks