Messages in this thread | | | Date | Tue, 4 Oct 2022 14:06:47 -0500 | Subject | Re: [PATCH] KVM: SVM: Fix reserved fields of struct sev_es_save_area | From | Carlos Bilbao <> |
| |
On 10/4/22 13:51, Sean Christopherson wrote:
> On Tue, Oct 04, 2022, Carlos Bilbao wrote: >> On 10/4/22 11:29, Sean Christopherson wrote: >>> On Tue, Oct 04, 2022, Carlos Bilbao wrote: >>>> On 10/4/22 09:05, Carlos Bilbao wrote: >>>> >>>>> Reserved fields of struct sev_es_save_area are named by their order of >>>>> appearance, but right now they jump from reserved_5 to reserved_7. Rename >>>>> them with the correct order. >>>>> >>>>> Fixes: 6d3b3d34e39eb ("KVM: SVM: Update the SEV-ES save area mapping") >>>> Actually, there is no bug, so this Fix tag could go. Thanks!! >>> Fixes: is appropriate, if we think it's worth fixing. Personally, I don't think >>> it's worth the churn/effort to keep the reserved numbers accurate, e.g. if the >>> two bytes in reserved_1 are used, then every other field will need to be updated >>> just to accomodate a tiny change. We'll find ourselves in a similar situation if >>> field is added in the middle of reserved_3, >>> >>> If we really want to the number to have any kind of meaning without needing a pile >>> of churn for every update, the best idea I can think of is to name them reserved_<offset>. >>> That way only the affected reserved field needs to be modified when adding new >>> legal fields. But that has it's own flavor of maintenance burden as calculating >>> and verifying the offset is a waste of everyone's time. >>> >>> TL;DR: I vote to sweep this under the rug and live with arbitrary/bad numbers. >> Well, the discussion on what is the most appropriate way to name reserved >> fields is orthogonal to this patch, IMO. > It's not orthogonal, because how this "bug" is fixed determines the ongoing > maintenance cost. If we're going to deal with code churn to clean things up, then > we want to churn the code exactly once. If this was a one-line change then I > wouldn't care as much, but since it requires modifying half the reserved fields, > I'd rather we take an all-or-nothing approach. Makes sense. >> This change just follows the prior approach (reserved_x), but correctly. >> Keep in mind that the existence of reserved_{1,5} and reserved_{7,11} >> implies there's a reserved_6 (there isn't). Why knowingly keep something >> that's wrong, even if small? > Because the cost of maintaining the ordering far outweighs the benefits. It's > not just about this patch, it's about all the future patches and reviews that will > be needed to keep the ordering correct. On the benefits side, the fields are never > referenced and the names are effectively arbitrary, i.e. there's no real value in > keeping a sequence. > > A simple "fix" would be to add a comment that the names are arbitrary and have > no meaning. If really want to avoid ordering/skipping issues, then the we could > assign truly arbitrary names, e.g. something silly like this: > > diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h > index 0361626841bc..e55299a733b3 100644 > --- a/arch/x86/include/asm/svm.h > +++ b/arch/x86/include/asm/svm.h > @@ -332,7 +332,10 @@ struct vmcb_save_area { > u32 spec_ctrl; /* Guest version of SPEC_CTRL at 0x2E0 */ > } __packed; > > -/* Save area definition for SEV-ES and SEV-SNP guests */ > +/* > + * Save area definition for SEV-ES and SEV-SNP guests. Note the names of the > + * reserved fields are arbitrary and have no meaning. > + */
I'm thinking, if we add this note then there's really no need to change any field names.
> struct sev_es_save_area { > struct vmcb_seg es; > struct vmcb_seg cs; > @@ -349,12 +352,12 @@ struct sev_es_save_area { > u64 vmpl2_ssp; > u64 vmpl3_ssp; > u64 u_cet; > - u8 reserved_1[2]; > + u8 reserved_beef[2]; > u8 vmpl; > u8 cpl; > - u8 reserved_2[4]; > + u8 reserved_cafe[4]; > u64 efer; > - u8 reserved_3[104]; > + u8 reserved_feed[104]; > u64 xss; > u64 cr4; > u64 cr3; > @@ -371,7 +374,7 @@ struct sev_es_save_area { > u64 dr1_addr_mask; > u64 dr2_addr_mask; > u64 dr3_addr_mask; > - u8 reserved_4[24]; > + u8 reserved_fee[24]; > u64 rsp; > u64 s_cet; > u64 ssp; > @@ -386,21 +389,21 @@ struct sev_es_save_area { > u64 sysenter_esp; > u64 sysenter_eip; > u64 cr2; > - u8 reserved_5[32]; > + u8 reserved_deaf[32]; > u64 g_pat; > u64 dbgctl; > u64 br_from; > u64 br_to; > u64 last_excp_from; > u64 last_excp_to; > - u8 reserved_7[80]; > + u8 reserved_dead[80]; > u32 pkru; > - u8 reserved_8[20]; > - u64 reserved_9; /* rax already available at 0x01f8 */ > + u8 reserved_bed[20]; > + u64 reserved_bead; /* rax already available at 0x01f8 */ > u64 rcx; > u64 rdx; > u64 rbx; > - u64 reserved_10; /* rsp already available at 0x01d8 */ > + u64 reserved_cab; /* rsp already available at 0x01d8 */ > u64 rbp; > u64 rsi; > u64 rdi; > @@ -412,7 +415,7 @@ struct sev_es_save_area { > u64 r13; > u64 r14; > u64 r15; > - u8 reserved_11[16]; > + u8 reserved_face[16]; > u64 guest_exit_info_1; > u64 guest_exit_info_2; > u64 guest_exit_int_info; > @@ -425,7 +428,7 @@ struct sev_es_save_area { > u64 pcpu_id; > u64 event_inj; > u64 xcr0; > - u8 reserved_12[16]; > + u8 reserved_fade[16]; > > /* Floating point area */ > u64 x87_dp; > >
| |