| Message ID | 20240215113128.275608-7-nikunj@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Secure TSC support for SNP guests | expand |
On Thu, Feb 15, 2024 at 05:01:18PM +0530, Nikunj A Dadhania wrote: > SNP command mutex is used to serialize the shared buffer access, command > handling and message sequence number races. Move the SNP guest command > mutex out of the sev guest driver and provide accessors to sev-guest And why in the hell are we doing this? Always, *ALWAYS* make sure a patch's commit message answers *why* a change is done. This doesn't explain why so I'm reading "just because" and "just because" doesn't fly. > driver. Remove multiple lockdep check in sev-guest driver, next patch adds > a single lockdep check in snp_send_guest_request(). The concept of "next patch" is meaningless once the patch is in git.
On 4/22/2024 6:30 PM, Borislav Petkov wrote: > On Thu, Feb 15, 2024 at 05:01:18PM +0530, Nikunj A Dadhania wrote: >> SNP command mutex is used to serialize the shared buffer access, command >> handling and message sequence number races. Move the SNP guest command >> mutex out of the sev guest driver and provide accessors to sev-guest > > And why in the hell are we doing this? SNP guest messaging will be moving as part of sev.c, and Secure TSC code will use this mutex. > Always, *ALWAYS* make sure a patch's commit message answers *why* > a change is done. This doesn't explain why so I'm reading "just because" > and "just because" doesn't fly. Sure, will change. > >> driver. Remove multiple lockdep check in sev-guest driver, next patch adds >> a single lockdep check in snp_send_guest_request(). > > The concept of "next patch" is meaningless once the patch is in git. Sure. As direct access to the mutex was not available now, I had removed lockdep check here and documented that lockdep gets added at later point. Regards Nikunj
On Tue, Apr 23, 2024 at 09:52:41AM +0530, Nikunj A. Dadhania wrote: > SNP guest messaging will be moving as part of sev.c, and Secure TSC code > will use this mutex. No, this is all backwards. You have a *static* function in sev-guest - snp_guest_ioctl- which takes an exported lock - snp_guest_cmd_lock - in order to synchronize with other callers which are only in that same sev-guest driver. Why do you even need the guest messaging in sev.c? I guess this: "Many of the required functions are implemented in the sev-guest driver and therefore not available at early boot." But then your API is misdesigned: the lock should be private to sev.c and none of the callers should pay attention to grabbing it - the callers simply call the functions and underneath the locking works automatically for them - they don't care. Just like any other shared resource, users see only the API they call and the actual synchronization is done behind the scenes. Sounds like you need to go back to the drawing board and think how this thing should look like. And when you have it, make sure to explain the commit messages *why* it is done this way. Thx.
On 4/23/2024 3:58 PM, Borislav Petkov wrote: > On Tue, Apr 23, 2024 at 09:52:41AM +0530, Nikunj A. Dadhania wrote: >> SNP guest messaging will be moving as part of sev.c, and Secure TSC code >> will use this mutex. > > No, this is all backwards. > > You have a *static* function in sev-guest - snp_guest_ioctl- which takes > an exported lock - snp_guest_cmd_lock - in order to synchronize with > other callers which are only in that same sev-guest driver. > > Why do you even need the guest messaging in sev.c? > > I guess this: "Many of the required functions are implemented in the > sev-guest driver and therefore not available at early boot." Yes. > > But then your API is misdesigned: the lock should be private to sev.c > and none of the callers should pay attention to grabbing it - the > callers simply call the functions and underneath the locking works > automatically for them - they don't care. Just like any other shared > resource, users see only the API they call and the actual > synchronization is done behind the scenes. > > Sounds like you need to go back to the drawing board and think how this > thing should look like. Something like below ? snp_guest_ioctl() -> get_report()/get_derived_key()/get_ext_report() -> snp_send_guest_request() snp_guest_cmd_lock(); ... snp_guest_cmd_lock(); With this the cmd_lock will be private to sev.c and lock/unlock function doesn't need to be exported. Regards Nikunj
On Tue, Apr 23, 2024 at 04:12:00PM +0530, Nikunj A. Dadhania wrote: > Something like below ? > > snp_guest_ioctl() > -> get_report()/get_derived_key()/get_ext_report() > -> snp_send_guest_request() > snp_guest_cmd_lock(); > ... > snp_guest_cmd_lock(); > > With this the cmd_lock will be private to sev.c and lock/unlock function > doesn't need to be exported. Yes, something like that.
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h index e4f52a606487..8578b33d8fc4 100644 --- a/arch/x86/include/asm/sev.h +++ b/arch/x86/include/asm/sev.h @@ -295,6 +295,8 @@ void snp_accept_memory(phys_addr_t start, phys_addr_t end); u64 snp_get_unsupported_features(u64 status); u64 sev_get_status(void); void kdump_sev_callback(void); +void snp_guest_cmd_lock(void); +void snp_guest_cmd_unlock(void); #else static inline void sev_es_ist_enter(struct pt_regs *regs) { } static inline void sev_es_ist_exit(void) { } @@ -325,6 +327,8 @@ static inline void snp_accept_memory(phys_addr_t start, phys_addr_t end) { } static inline u64 snp_get_unsupported_features(u64 status) { return 0; } static inline u64 sev_get_status(void) { return 0; } static inline void kdump_sev_callback(void) { } +static inline void snp_guest_cmd_lock(void) { } +static inline void snp_guest_cmd_unlock(void) { } #endif #ifdef CONFIG_KVM_AMD_SEV diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c index eda43c35a9f2..bc4a705d989c 100644 --- a/arch/x86/kernel/sev.c +++ b/arch/x86/kernel/sev.c @@ -940,6 +940,21 @@ static void snp_cleanup_vmsa(struct sev_es_save_area *vmsa) free_page((unsigned long)vmsa); } +/* SNP Guest command mutex to serialize the shared buffer access and command handling. */ +static DEFINE_MUTEX(snp_guest_cmd_mutex); + +void snp_guest_cmd_lock(void) +{ + mutex_lock(&snp_guest_cmd_mutex); +} +EXPORT_SYMBOL_GPL(snp_guest_cmd_lock); + +void snp_guest_cmd_unlock(void) +{ + mutex_unlock(&snp_guest_cmd_mutex); +} +EXPORT_SYMBOL_GPL(snp_guest_cmd_unlock); + static int wakeup_cpu_via_vmgexit(u32 apic_id, unsigned long start_ip) { struct sev_es_save_area *cur_vmsa, *vmsa; diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c index 646eb215f3c7..ba9ffaee647c 100644 --- a/drivers/virt/coco/sev-guest/sev-guest.c +++ b/drivers/virt/coco/sev-guest/sev-guest.c @@ -62,9 +62,6 @@ static u32 vmpck_id; module_param(vmpck_id, uint, 0444); MODULE_PARM_DESC(vmpck_id, "The VMPCK ID to use when communicating with the PSP."); -/* Mutex to serialize the shared buffer access and command handling. */ -static DEFINE_MUTEX(snp_cmd_mutex); - static inline u8 *snp_get_vmpck(struct snp_guest_dev *snp_dev) { return snp_dev->layout->vmpck0 + snp_dev->vmpck_id * VMPCK_KEY_LEN; @@ -114,8 +111,6 @@ static inline u64 __snp_get_msg_seqno(struct snp_guest_dev *snp_dev) u32 *os_area_msg_seqno = snp_get_os_area_msg_seqno(snp_dev); u64 count; - lockdep_assert_held(&snp_cmd_mutex); - /* Read the current message sequence counter from secrets pages */ count = *os_area_msg_seqno; @@ -408,8 +403,6 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io struct snp_report_resp *report_resp; int rc, resp_len; - lockdep_assert_held(&snp_cmd_mutex); - if (!arg->req_data || !arg->resp_data) return -EINVAL; @@ -456,8 +449,6 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque /* Response data is 64 bytes and max authsize for GCM is 16 bytes. */ u8 buf[64 + 16]; - lockdep_assert_held(&snp_cmd_mutex); - if (!arg->req_data || !arg->resp_data) return -EINVAL; @@ -508,8 +499,6 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques sockptr_t certs_address; int ret, resp_len; - lockdep_assert_held(&snp_cmd_mutex); - if (sockptr_is_null(io->req_data) || sockptr_is_null(io->resp_data)) return -EINVAL; @@ -605,12 +594,12 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long if (!input.msg_version) return -EINVAL; - mutex_lock(&snp_cmd_mutex); + snp_guest_cmd_lock(); /* Check if the VMPCK is not empty */ if (snp_is_vmpck_empty(snp_dev)) { dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n"); - mutex_unlock(&snp_cmd_mutex); + snp_guest_cmd_unlock(); return -ENOTTY; } @@ -635,7 +624,7 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long break; } - mutex_unlock(&snp_cmd_mutex); + snp_guest_cmd_unlock(); if (input.exitinfo2 && copy_to_user(argp, &input, sizeof(input))) return -EFAULT; @@ -725,14 +714,14 @@ static int sev_report_new(struct tsm_report *report, void *data) if (!buf) return -ENOMEM; - guard(mutex)(&snp_cmd_mutex); - /* Check if the VMPCK is not empty */ if (snp_is_vmpck_empty(snp_dev)) { dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n"); return -ENOTTY; } + snp_guest_cmd_lock(); + cert_table = buf + report_size; struct snp_ext_report_req ext_req = { .data = { .vmpl = desc->privlevel }, @@ -753,6 +742,8 @@ static int sev_report_new(struct tsm_report *report, void *data) }; ret = get_ext_report(snp_dev, &input, &io); + snp_guest_cmd_unlock(); + if (ret) return ret;