With SVM_SEV_FEAT_DEBUG_SWAP enabled, the VM should not get #VC events
for DR7 read/write which it rather avoided.
Signed-off-by: Alexey Kardashevskiy <[email protected]>
---
arch/x86/kernel/sev.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index a428c62330d3..4e91b9f8742c 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -1618,6 +1618,9 @@ static enum es_result vc_handle_dr7_write(struct ghcb *ghcb,
long val, *reg = vc_insn_get_rm(ctxt);
enum es_result ret;
+ if ((sev_status >> 2) & SVM_SEV_FEAT_DEBUG_SWAP)
+ return ES_VMM_ERROR;
+
if (!reg)
return ES_DECODE_FAILED;
@@ -1655,6 +1658,9 @@ static enum es_result vc_handle_dr7_read(struct ghcb *ghcb,
struct sev_es_runtime_data *data = this_cpu_read(runtime_data);
long *reg = vc_insn_get_rm(ctxt);
+ if ((sev_status >> 2) & SVM_SEV_FEAT_DEBUG_SWAP)
+ return ES_VMM_ERROR;
+
if (!reg)
return ES_DECODE_FAILED;
--
2.38.1
On Thu, Dec 01, 2022, Alexey Kardashevskiy wrote:
> With SVM_SEV_FEAT_DEBUG_SWAP enabled, the VM should not get #VC events
> for DR7 read/write which it rather avoided.
>
> Signed-off-by: Alexey Kardashevskiy <[email protected]>
> ---
> arch/x86/kernel/sev.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index a428c62330d3..4e91b9f8742c 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -1618,6 +1618,9 @@ static enum es_result vc_handle_dr7_write(struct ghcb *ghcb,
> long val, *reg = vc_insn_get_rm(ctxt);
> enum es_result ret;
>
> + if ((sev_status >> 2) & SVM_SEV_FEAT_DEBUG_SWAP)
Probably high time to add a helper/macro to convert the SEV_STATUS to the SEV_FEATURES
field.
> + return ES_VMM_ERROR;
> +
> if (!reg)
> return ES_DECODE_FAILED;
>
> @@ -1655,6 +1658,9 @@ static enum es_result vc_handle_dr7_read(struct ghcb *ghcb,
> struct sev_es_runtime_data *data = this_cpu_read(runtime_data);
> long *reg = vc_insn_get_rm(ctxt);
>
> + if ((sev_status >> 2) & SVM_SEV_FEAT_DEBUG_SWAP)
> + return ES_VMM_ERROR;
> +
> if (!reg)
> return ES_DECODE_FAILED;
>
> --
> 2.38.1
>
On Thu, Dec 01, 2022 at 05:38:33PM +0000, Sean Christopherson wrote:
> Probably high time to add a helper/macro to convert the SEV_STATUS to
> the SEV_FEATURES field.
Nah, there's a couple of
MSR_AMD64_SEV*
defines in arch/x86/include/asm/msr-index.h.
Bit 5 should simply be added there.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Dec 07, 2022, Borislav Petkov wrote:
> On Thu, Dec 01, 2022 at 05:38:33PM +0000, Sean Christopherson wrote:
> > Probably high time to add a helper/macro to convert the SEV_STATUS to
> > the SEV_FEATURES field.
>
> Nah, there's a couple of
>
> MSR_AMD64_SEV*
>
> defines in arch/x86/include/asm/msr-index.h.
>
> Bit 5 should simply be added there.
Ah, yeah, that's much better.
On 8/12/22 06:07, Sean Christopherson wrote:
> On Wed, Dec 07, 2022, Borislav Petkov wrote:
>> On Thu, Dec 01, 2022 at 05:38:33PM +0000, Sean Christopherson wrote:
>>> Probably high time to add a helper/macro to convert the SEV_STATUS to
>>> the SEV_FEATURES field.
>>
>> Nah, there's a couple of
>>
>> MSR_AMD64_SEV*
>>
>> defines in arch/x86/include/asm/msr-index.h.
>>
>> Bit 5 should simply be added there.
>
> Ah, yeah, that's much better.
Sorry, I am not following. How is moving the bit makes
SEV_STATUS_TO_FEATURES() not needed?
When I am setting it in VMSA SEV_FEATURES - it is a bit 5.
Inside a SEV VM, it is SEV_STATUS MSR and there it is bit 7. Mentioned
MSR_AMD64_SEV* are SEV_STATUS MSR bits.
Since the current patch is bad, I'd rather define the bit twice then:
arch/x86/include/asm/msr-index.h:
#define MSR_AMD64_SEV_FEAT_DEBUG_SWAP BIT_ULL(7)
arch/x86/include/asm/svm.h
#define SVM_SEV_FEAT_DEBUG_SWAP BIT(5)
as nothing really says that SEV_FEATURES is always going to be
SEV_STATUS>>2, even though it is now.
Soooo what is acceptable solution here? Thanks,
--
Alexey
On Thu, Dec 08, 2022 at 06:14:01PM +1100, Alexey Kardashevskiy wrote:
> Sorry, I am not following. How is moving the bit makes
> SEV_STATUS_TO_FEATURES() not needed?
>
> When I am setting it in VMSA SEV_FEATURES - it is a bit 5.
>
> Inside a SEV VM, it is SEV_STATUS MSR and there it is bit 7. Mentioned
> MSR_AMD64_SEV* are SEV_STATUS MSR bits.
>
> Since the current patch is bad, I'd rather define the bit twice then:
Yes.
> arch/x86/include/asm/msr-index.h:
> #define MSR_AMD64_SEV_FEAT_DEBUG_SWAP BIT_ULL(7)
>
> arch/x86/include/asm/svm.h
> #define SVM_SEV_FEAT_DEBUG_SWAP BIT(5)
>
> as nothing really says that SEV_FEATURES is always going to be
> SEV_STATUS>>2, even though it is now.
>
> Soooo what is acceptable solution here? Thanks,
Right, so since you're testing against sev_status which is a copy of
MSR_AMD64_SEV, then you use bit definitions which are for that MSR as
documented in the respective PPR section for "MSRC001_0131 [SEV Status]
(Core::X86::Msr::SEV_Status)"
When you're setting the VMSA's SEV_FEATURES field, then you need a
different define, ofc.
This also automatically takes care of SEV_FEATURES not being tied to
SEV_STATUS >> 2 forever, as you say.
So yes, do the twice thing.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette