Messages in this thread | | | Subject | Re: [PATCH v2 4/7] KVM: SVM: hyper-v: Nested enlightenments in VMCB | From | Vineeth Pillai <> | Date | Fri, 16 Apr 2021 13:07:00 -0400 |
| |
On 4/16/2021 4:58 AM, Vitaly Kuznetsov wrote: > >> + >> +#if IS_ENABLED(CONFIG_HYPERV) >> +struct __packed hv_enlightenments { >> + struct __packed hv_enlightenments_control { >> + u32 nested_flush_hypercall:1; >> + u32 msr_bitmap:1; >> + u32 enlightened_npt_tlb: 1; >> + u32 reserved:29; >> + } hv_enlightenments_control; >> + u32 hv_vp_id; >> + u64 hv_vm_id; >> + u64 partition_assist_page; >> + u64 reserved; >> +}; > Enlightened VMCS seems to have the same part: > > struct { > u32 nested_flush_hypercall:1; > u32 msr_bitmap:1; > u32 reserved:30; > } __packed hv_enlightenments_control; > u32 hv_vp_id; > u64 hv_vm_id; > u64 partition_assist_page; > > Would it maybe make sense to unify these two (in case they are the same > thing in Hyper-V, of course)? They are very similar but, the individual bits are a bit different. SVM struct has an additional bit 'enlightened_npt_tlb'. There might be future changes as well if new enlightenments are designed for performance optimization. So I feel, we can have it as separate structs.
>> >> +#define VMCB_ALL_CLEAN_MASK ( \ >> + (1U << VMCB_INTERCEPTS) | (1U << VMCB_PERM_MAP) | \ >> + (1U << VMCB_ASID) | (1U << VMCB_INTR) | \ >> + (1U << VMCB_NPT) | (1U << VMCB_CR) | (1U << VMCB_DR) | \ >> + (1U << VMCB_DT) | (1U << VMCB_SEG) | (1U << VMCB_CR2) | \ >> + (1U << VMCB_LBR) | (1U << VMCB_AVIC) \ >> + ) > What if we preserve VMCB_DIRTY_MAX and drop this newly introduced > VMCB_ALL_CLEAN_MASK (which basically lists all the members of the enum > above)? '1 << VMCB_DIRTY_MAX' can still work. (If the 'VMCB_DIRTY_MAX' > name becomes misleading we can e.g. rename it to VMCB_NATIVE_DIRTY_MAX > or something but I'm not sure it's worth it)
I thought of keeping this code because, if we have non-contiguous bits in future, we would need this kinda logic anyways. But I get your point. Will revert this.
> >> +#if IS_ENABLED(CONFIG_HYPERV) >> +#define VMCB_HYPERV_CLEAN_MASK (1U << VMCB_HV_NESTED_ENLIGHTENMENTS) >> +#endif > VMCB_HYPERV_CLEAN_MASK is a single bit, why do we need it at all > (BIT(VMCB_HV_NESTED_ENLIGHTENMENTS) is not super long)
Agreed. Will change it in next revision.
Thanks, Vineeth
| |