| Message ID | 20211020062321.3581158-1-ltykernel@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | x86/sev-es: Expose __sev_es_ghcb_hv_call() to call ghcb hv call out of sev code | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, Oct 20, 2021 at 02:23:16AM -0400, Tianyu Lan wrote: > From: Tianyu Lan <Tianyu.Lan@microsoft.com> > > > Hyper-V also needs to call ghcb hv call to write/read MSR in Isolation VM. > So expose __sev_es_ghcb_hv_call() to call it in the Hyper-V code. > > Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com> > --- > arch/x86/include/asm/sev.h | 11 +++++++++++ > arch/x86/kernel/sev-shared.c | 24 +++++++++++++++++++----- > 2 files changed, 30 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h > index fa5cd05d3b5b..295c847c3cd4 100644 > --- a/arch/x86/include/asm/sev.h > +++ b/arch/x86/include/asm/sev.h > @@ -81,12 +81,23 @@ static __always_inline void sev_es_nmi_complete(void) > __sev_es_nmi_complete(); > } > extern int __init sev_es_efi_map_ghcbs(pgd_t *pgd); > +extern 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); You can do here: static inline enum es_result __sev_es_ghcb_hv_call(struct ghcb *ghcb, u64 exit_code, u64 exit_info_1, u64 exit_info_2) { return ES_VMM_ERROR; } > @@ -137,12 +141,22 @@ static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, > 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(); > > return verify_exception_info(ghcb, ctxt); > } > > +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) > +{ > + sev_es_wr_ghcb_msr(__pa(ghcb)); > + > + return __sev_es_ghcb_hv_call(ghcb, ctxt, exit_code, exit_info_1, > + exit_info_2); > +} Well, why does Hyper-V need this thing a bit differently, without the setting of the GHCB's physical address? What if another hypervisor does yet another SEV implementation and yet another HV call needs to be defined? If stuff is going to be exported to other users, then stuff better be defined properly so that it is used by multiple hypervisors.
On 10/20/2021 5:59 PM, Borislav Petkov wrote: > On Wed, Oct 20, 2021 at 02:23:16AM -0400, Tianyu Lan wrote: >> From: Tianyu Lan <Tianyu.Lan@microsoft.com> >> >> >> Hyper-V also needs to call ghcb hv call to write/read MSR in Isolation VM. >> So expose __sev_es_ghcb_hv_call() to call it in the Hyper-V code. >> >> Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com> >> --- >> arch/x86/include/asm/sev.h | 11 +++++++++++ >> arch/x86/kernel/sev-shared.c | 24 +++++++++++++++++++----- >> 2 files changed, 30 insertions(+), 5 deletions(-) >> >> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h >> index fa5cd05d3b5b..295c847c3cd4 100644 >> --- a/arch/x86/include/asm/sev.h >> +++ b/arch/x86/include/asm/sev.h >> @@ -81,12 +81,23 @@ static __always_inline void sev_es_nmi_complete(void) >> __sev_es_nmi_complete(); >> } >> extern int __init sev_es_efi_map_ghcbs(pgd_t *pgd); >> +extern 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); > > You can do here: > > static inline enum es_result > __sev_es_ghcb_hv_call(struct ghcb *ghcb, u64 exit_code, u64 exit_info_1, u64 exit_info_2) { return ES_VMM_ERROR; } > >> @@ -137,12 +141,22 @@ static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, >> 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(); >> >> return verify_exception_info(ghcb, ctxt); >> } >> >> +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) >> +{ >> + sev_es_wr_ghcb_msr(__pa(ghcb)); >> + >> + return __sev_es_ghcb_hv_call(ghcb, ctxt, exit_code, exit_info_1, >> + exit_info_2); >> +} > > Well, why does Hyper-V need this thing a bit differently, without the > setting of the GHCB's physical address? Hyper-V runs paravisor in guest VMPL0 which emulates some functions (e.g, timer, tsc, serial console and so on) via handling VC exception. GHCB pages are allocated and set up by the paravisor and report to Linux guest via MSR register.Hyper-V SEV implementation is unenlightened guest case which doesn't Linux doesn't handle VC and paravisor in the VMPL0 handle it. > > What if another hypervisor does yet another SEV implementation and yet > another HV call needs to be defined? In the early version, these ghcb operations are implemented in Hyper-V code and get comments to use existing code in the SEV ES as much as possible. So latter versions expose the API to re-use code. Current two cases: enlightened guest and un-enlightened guest. Tom and brjesh pushed enlightened case. Hyper-V is un-enlightened case and a paravisor runs in VMPL0 to handle VC to emulate devices inside VM. GHCB is allocated and set up by paravisor in the un-enlightened case. The new __sev_es_ghcb_hv_call() is to handle these two cases.
On Wed, Oct 20, 2021 at 08:39:59PM +0800, Tianyu Lan wrote: > Hyper-V runs paravisor in guest VMPL0 which emulates some functions > (e.g, timer, tsc, serial console and so on) via handling VC exception. > GHCB pages are allocated and set up by the paravisor and report to Linux > guest via MSR register.Hyper-V SEV implementation is unenlightened guest > case which doesn't Linux doesn't handle VC and paravisor in the VMPL0 > handle it. Aha, unenlightened. So why don't you export the original function by doing this (only partial diff to show intent only): --- diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c index f1d513897baf..bfe82f58508f 100644 --- a/arch/x86/kernel/sev-shared.c +++ b/arch/x86/kernel/sev-shared.c @@ -125,7 +125,7 @@ static enum es_result verify_exception_info(struct ghcb *ghcb, struct es_em_ctxt return ES_VMM_ERROR; } -static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, +static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, bool set_ghcb_msr, struct es_em_ctxt *ctxt, u64 exit_code, u64 exit_info_1, u64 exit_info_2) @@ -138,7 +138,14 @@ static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, 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)); + /* + * Hyper-V unenlightened guests use a paravisor for communicating and + * GHCB pages are being allocated by that paravisor which uses a + * different MSR and protocol. + */ + if (set_ghcb_msr) + sev_es_wr_ghcb_msr(__pa(ghcb)); + VMGEXIT(); return verify_exception_info(ghcb, ctxt);
On 10/20/21 8:39 AM, Borislav Petkov wrote: > On Wed, Oct 20, 2021 at 08:39:59PM +0800, Tianyu Lan wrote: >> Hyper-V runs paravisor in guest VMPL0 which emulates some functions >> (e.g, timer, tsc, serial console and so on) via handling VC exception. >> GHCB pages are allocated and set up by the paravisor and report to Linux >> guest via MSR register.Hyper-V SEV implementation is unenlightened guest >> case which doesn't Linux doesn't handle VC and paravisor in the VMPL0 >> handle it. > > Aha, unenlightened. > > So why don't you export the original function by doing this (only > partial diff to show intent only): > > --- > diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c > index f1d513897baf..bfe82f58508f 100644 > --- a/arch/x86/kernel/sev-shared.c > +++ b/arch/x86/kernel/sev-shared.c > @@ -125,7 +125,7 @@ static enum es_result verify_exception_info(struct ghcb *ghcb, struct es_em_ctxt > return ES_VMM_ERROR; > } > > -static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, > +static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, bool set_ghcb_msr, > struct es_em_ctxt *ctxt, > u64 exit_code, u64 exit_info_1, > u64 exit_info_2) > @@ -138,7 +138,14 @@ static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, > 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)); > + /* > + * Hyper-V unenlightened guests use a paravisor for communicating and > + * GHCB pages are being allocated by that paravisor which uses a > + * different MSR and protocol. Just to clarify the comment, the paravisor uses the same GHCB MSR and GHCB protocol, it just can't use __pa() to get the address of the GHCB. So I expect that the Hyper-V support sets the address properly before calling this function. Thanks, Tom > + */ > + if (set_ghcb_msr) > + sev_es_wr_ghcb_msr(__pa(ghcb)); > + > VMGEXIT(); > > return verify_exception_info(ghcb, ctxt); > >
On 10/20/2021 9:56 PM, Tom Lendacky wrote: > On 10/20/21 8:39 AM, Borislav Petkov wrote: >> On Wed, Oct 20, 2021 at 08:39:59PM +0800, Tianyu Lan wrote: >>> Hyper-V runs paravisor in guest VMPL0 which emulates some functions >>> (e.g, timer, tsc, serial console and so on) via handling VC exception. >>> GHCB pages are allocated and set up by the paravisor and report to Linux >>> guest via MSR register.Hyper-V SEV implementation is unenlightened guest >>> case which doesn't Linux doesn't handle VC and paravisor in the VMPL0 >>> handle it. >> >> Aha, unenlightened. >> >> So why don't you export the original function by doing this (only >> partial diff to show intent only): This follows Joreg's previous comment and I implemented similar version in the V! patchset([PATCH 05/13] HV: Add Write/Read MSR registers via ghcb page https://lkml.org/lkml/2021/7/28/668). "Instead, factor out a helper function which contains what Hyper-V needs and use that in sev_es_ghcb_hv_call() and Hyper-V code." https://lkml.org/lkml/2021/8/2/375 >> >> --- >> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c >> index f1d513897baf..bfe82f58508f 100644 >> --- a/arch/x86/kernel/sev-shared.c >> +++ b/arch/x86/kernel/sev-shared.c >> @@ -125,7 +125,7 @@ static enum es_result verify_exception_info(struct >> ghcb *ghcb, struct es_em_ctxt >> return ES_VMM_ERROR; >> } >> -static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, >> +static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, bool >> set_ghcb_msr, >> struct es_em_ctxt *ctxt, >> u64 exit_code, u64 exit_info_1, >> u64 exit_info_2) >> @@ -138,7 +138,14 @@ static enum es_result sev_es_ghcb_hv_call(struct >> ghcb *ghcb, >> 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)); >> + /* >> + * Hyper-V unenlightened guests use a paravisor for communicating >> and >> + * GHCB pages are being allocated by that paravisor which uses a >> + * different MSR and protocol. > > Just to clarify the comment, the paravisor uses the same GHCB MSR and > GHCB protocol, it just can't use __pa() to get the address of the GHCB. > So I expect that the Hyper-V support sets the address properly before > calling this function. > > Thanks, > Tom > >> + */ >> + if (set_ghcb_msr) >> + sev_es_wr_ghcb_msr(__pa(ghcb)); >> + >> VMGEXIT(); >> return verify_exception_info(ghcb, ctxt); >> >>
On Wed, Oct 20, 2021 at 10:23:06PM +0800, Tianyu Lan wrote: > This follows Joreg's previous comment and I implemented similar version in > the V! patchset([PATCH 05/13] HV: Add Write/Read MSR registers via ghcb page > https://lkml.org/lkml/2021/7/28/668). > "Instead, factor out a helper function which contains what Hyper-V needs and > use that in sev_es_ghcb_hv_call() and Hyper-V code." > > https://lkml.org/lkml/2021/8/2/375 If you wanna point to mails on a mailing list, you simply do https://lore.kernel.org/r/<Message-id> No need to use some random, unreliable web pages. As to Joerg's suggestion, in the version I'm seeing, you're checking the *context* - and the one you sent today, avoids the __pa(ghcb) MSR write. So which is it? Because your current version will look at the context too, see return verify_exception_info(ghcb, ctxt); at the end of the function. So is the issue what Tom said that "the paravisor uses the same GHCB MSR and GHCB protocol, it just can't use __pa() to get the address of the GHCB."? If that is the case and the only thing you want is to avoid the GHCB PA write, then, in the future, we might drop that MSR write altogether on the enlightened Linux guests too and then the same function will be used by your paravisor and the Linux guest. So please explain in detail what exactly you want to avoid from sev_es_ghcb_hv_call()'s current version and why. As I said before, I don't want to export any random details of the SEV implementation in the kernel without any justification for it. Thx.
On 10/20/2021 10:39 PM, Borislav Petkov wrote: > On Wed, Oct 20, 2021 at 10:23:06PM +0800, Tianyu Lan wrote: >> This follows Joreg's previous comment and I implemented similar version in >> the V! patchset([PATCH 05/13] HV: Add Write/Read MSR registers via ghcb page >> https://lkml.org/lkml/2021/7/28/668). >> "Instead, factor out a helper function which contains what Hyper-V needs and >> use that in sev_es_ghcb_hv_call() and Hyper-V code." >> >> https://lkml.org/lkml/2021/8/2/375 > > If you wanna point to mails on a mailing list, you simply do > > https://lore.kernel.org/r/<Message-id> > > No need to use some random, unreliable web pages. OK. Thanks for suggestion. > > As to Joerg's suggestion, in the version I'm seeing, you're checking the > *context* - and the one you sent today, avoids the __pa(ghcb) MSR write. > > So which is it? > > Because your current version will look at the context too, see > > return verify_exception_info(ghcb, ctxt); > > at the end of the function. Both old and new patches are to avoid setting GHCB page address via MSR. Paravisor is in charge of doing that and un-enlightened guest should not change it. The old one was in the patchset v1 "x86/Hyper-V: Add Hyper-V Isolation VM support". The patch I sent today is based on your clean up patch and for review first. It should be in patchset "x86/Hyper-V: Add Hyper-V Isolation VM support." > > So is the issue what Tom said that "the paravisor uses the same GHCB MSR > and GHCB protocol, it just can't use __pa() to get the address of the > GHCB."? Yes, hyper-V enables vTOM in the CVM and GHCB page PA reported by paravisor contains vTOM bit. We need to use memremap() to map ghcb page before accessing GHCB page. __pa() doesn't work for PA with vTOM bit. Otherwise, guest should not set GHCB page address and avoid conflict with paravisor. > > If that is the case and the only thing you want is to avoid the GHCB PA > write, then, in the future, we might drop that MSR write altogether on > the enlightened Linux guests too and then the same function will be used > by your paravisor and the Linux guest. Yes, this is the target of the patch. Can we put the change in the Hyper-V patchset? Other patch has been fully reviewed. Thanks.
On Wed, Oct 20, 2021 at 11:09:03PM +0800, Tianyu Lan wrote: > Yes, this is the target of the patch. Can we put the change in the > Hyper-V patchset? If you're asking about this version: https://lore.kernel.org/r/20211020062321.3581158-1-ltykernel@gmail.com then, no. I'd prefer if you did this: https://lore.kernel.org/r/YXAcGtxe08XiHBFH@zn.tnic for reasons which I already explained. Thx.
On 10/21/2021 12:24 AM, Borislav Petkov wrote: > On Wed, Oct 20, 2021 at 11:09:03PM +0800, Tianyu Lan wrote: >> Yes, this is the target of the patch. Can we put the change in the >> Hyper-V patchset? > > If you're asking about this version: > > https://lore.kernel.org/r/20211020062321.3581158-1-ltykernel@gmail.com > > then, no. I'd prefer if you did this: > > https://lore.kernel.org/r/YXAcGtxe08XiHBFH@zn.tnic > > for reasons which I already explained. > Thanks for your suggestion. I just sent out v8 version according to your guide. Please have a look. Thanks.
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h index fa5cd05d3b5b..295c847c3cd4 100644 --- a/arch/x86/include/asm/sev.h +++ b/arch/x86/include/asm/sev.h @@ -81,12 +81,23 @@ static __always_inline void sev_es_nmi_complete(void) __sev_es_nmi_complete(); } extern int __init sev_es_efi_map_ghcbs(pgd_t *pgd); +extern 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); #else static inline void sev_es_ist_enter(struct pt_regs *regs) { } static inline void sev_es_ist_exit(void) { } static inline int sev_es_setup_ap_jump_table(struct real_mode_header *rmh) { return 0; } static inline void sev_es_nmi_complete(void) { } static inline int sev_es_efi_map_ghcbs(pgd_t *pgd) { return 0; } +static inline enum es_result +__sev_es_ghcb_hv_call(struct ghcb *ghcb, + u64 exit_code, u64 exit_info_1, + u64 exit_info_2) +{ + return ES_VMM_ERROR; +} #endif #endif diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c index ea9abd69237e..08c97cb057fa 100644 --- a/arch/x86/kernel/sev-shared.c +++ b/arch/x86/kernel/sev-shared.c @@ -124,10 +124,14 @@ static enum es_result verify_exception_info(struct ghcb *ghcb, struct es_em_ctxt return ES_VMM_ERROR; } -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) +/* + * __sev_es_ghcb_hv_call() is also used in the other platform code(e.g + * Hyper-V). + */ +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) { /* Fill in protocol and format specifiers */ ghcb->protocol_version = GHCB_PROTOCOL_MAX; @@ -137,12 +141,22 @@ static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb, 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(); return verify_exception_info(ghcb, ctxt); } +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) +{ + sev_es_wr_ghcb_msr(__pa(ghcb)); + + return __sev_es_ghcb_hv_call(ghcb, ctxt, exit_code, exit_info_1, + exit_info_2); +} + /* * Boot VC Handler - This is the first VC handler during boot, there is no GHCB * page yet, so it only supports the MSR based communication with the