On 2/12/22 10:32 PM, Marc Orr wrote:
> On Wed, Feb 9, 2022 at 8:44 AM Varad Gautam <varad.gautam@xxxxxxxx> wrote:
>>
>> Using Linux's CPUID #VC processing logic.
>>
>> Signed-off-by: Varad Gautam <varad.gautam@xxxxxxxx>
>> ---
>> lib/x86/amd_sev_vc.c | 98 ++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 98 insertions(+)
>>
>> diff --git a/lib/x86/amd_sev_vc.c b/lib/x86/amd_sev_vc.c
>> index 142f2cd..9ee67c0 100644
>> --- a/lib/x86/amd_sev_vc.c
>> +++ b/lib/x86/amd_sev_vc.c
>> @@ -2,6 +2,7 @@
>>
>> #include "amd_sev.h"
>> #include "svm.h"
>> +#include "x86/xsave.h"
>>
>> extern phys_addr_t ghcb_addr;
>>
>> @@ -52,6 +53,100 @@ static void vc_finish_insn(struct es_em_ctxt *ctxt)
>> ctxt->regs->rip += ctxt->insn.length;
>> }
>>
>> +static inline u64 lower_bits(u64 val, unsigned int bits)
>> +{
>> + u64 mask = (1ULL << bits) - 1;
>> +
>> + return (val & mask);
>> +}
>
> This isn't used in this patch. I guess it ends up being used later, in
> path 9: "x86: AMD SEV-ES: Handle IOIO #VC". Let's introduce it there
> if we're going to put it in this file. Though, again, maybe it's worth
> creating a platform agnostic bit library, and put this and
> `_test_bit()` (introduced in a previous patch) there.
>
Ack, it makes sense to introduce it later (and at a different place).
>> +
>> +static inline void sev_es_wr_ghcb_msr(u64 val)
>> +{
>> + wrmsr(MSR_AMD64_SEV_ES_GHCB, val);
>> +}
>> +
>> +static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb,
>> + struct es_em_ctxt *ctxt,
>> + u64 exit_code, u64 exit_info_1,
>> + u64 exit_info_2)
>> +{
>> + enum es_result ret;
>> +
>> + /* Fill in protocol and format specifiers */
>> + ghcb->protocol_version = GHCB_PROTOCOL_MAX;
>> + ghcb->ghcb_usage = GHCB_DEFAULT_USAGE;
>> +
>> + ghcb_set_sw_exit_code(ghcb, exit_code);
>> + ghcb_set_sw_exit_info_1(ghcb, exit_info_1);
>> + ghcb_set_sw_exit_info_2(ghcb, exit_info_2);
>> +
>> + sev_es_wr_ghcb_msr(__pa(ghcb));
>> + VMGEXIT();
>> +
>> + if ((ghcb->save.sw_exit_info_1 & 0xffffffff) == 1) {
>> + u64 info = ghcb->save.sw_exit_info_2;
>> + unsigned long v;
>> +
>> + info = ghcb->save.sw_exit_info_2;
>
> This line seems redundant, since `info` is already initialized to this
> value when it's declared, two lines above. That being said, I see this
> is how the code is in Linux as well. I wonder if it was done like this
> on accident.
>
Nice catch, it seems so. It's harmless, but I will drop it in v3.
>> + v = info & SVM_EVTINJ_VEC_MASK;
>> +
>> + /* Check if exception information from hypervisor is sane. */
>> + if ((info & SVM_EVTINJ_VALID) &&
>> + ((v == GP_VECTOR) || (v == UD_VECTOR)) &&
>> + ((info & SVM_EVTINJ_TYPE_MASK) == SVM_EVTINJ_TYPE_EXEPT)) {
>> + ctxt->fi.vector = v;
>> + if (info & SVM_EVTINJ_VALID_ERR)
>> + ctxt->fi.error_code = info >> 32;
>> + ret = ES_EXCEPTION;
>> + } else {
>> + ret = ES_VMM_ERROR;
>> + }
>> + } else if (ghcb->save.sw_exit_info_1 & 0xffffffff) {
>> + ret = ES_VMM_ERROR;
>> + } else {
>> + ret = ES_OK;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static enum es_result vc_handle_cpuid(struct ghcb *ghcb,
>> + struct es_em_ctxt *ctxt)
>> +{
>> + struct ex_regs *regs = ctxt->regs;
>> + u32 cr4 = read_cr4();
>> + enum es_result ret;
>> +
>> + ghcb_set_rax(ghcb, regs->rax);
>> + ghcb_set_rcx(ghcb, regs->rcx);
>> +
>> + if (cr4 & X86_CR4_OSXSAVE) {
>> + /* Safe to read xcr0 */
>> + u64 xcr0;
>> + xgetbv_checking(XCR_XFEATURE_ENABLED_MASK, &xcr0);
>> + ghcb_set_xcr0(ghcb, xcr0);
>> + } else
>> + /* xgetbv will cause #GP - use reset value for xcr0 */
>> + ghcb_set_xcr0(ghcb, 1);
>
> nit: Consider adding curly braces to the else branch, so that it
> matches the if branch.
>
Will do.
>> +
>> + ret = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_CPUID, 0, 0);
>> + if (ret != ES_OK)
>> + return ret;
>> +
>> + if (!(ghcb_rax_is_valid(ghcb) &&
>> + ghcb_rbx_is_valid(ghcb) &&
>> + ghcb_rcx_is_valid(ghcb) &&
>> + ghcb_rdx_is_valid(ghcb)))
>> + return ES_VMM_ERROR;
>> +
>> + regs->rax = ghcb->save.rax;
>> + regs->rbx = ghcb->save.rbx;
>> + regs->rcx = ghcb->save.rcx;
>> + regs->rdx = ghcb->save.rdx;
>> +
>> + return ES_OK;
>> +}
>> +
>> static enum es_result vc_handle_exitcode(struct es_em_ctxt *ctxt,
>> struct ghcb *ghcb,
>> unsigned long exit_code)
>> @@ -59,6 +154,9 @@ static enum es_result vc_handle_exitcode(struct es_em_ctxt *ctxt,
>> enum es_result result;
>>
>> switch (exit_code) {
>> + case SVM_EXIT_CPUID:
>> + result = vc_handle_cpuid(ghcb, ctxt);
>> + break;
>> default:
>> /*
>> * Unexpected #VC exception
>> --
>> 2.32.0
>>
>
