Re: [PATCH] KVM: SVM: Fix reserved fields of struct sev_es_save_area

[Carlos Bilbao <carlos.bilbao@amd.com>]

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;
>
>