On 4/16/2024 2:36 PM, Borislav Petkov wrote: > On Tue, Apr 16, 2024 at 11:27:24AM +0530, Nikunj A. Dadhania wrote: >>> Why does that snp_get_os_area_msg_seqno() returns a pointer when you >>> deref it here again? >>> >>> A function which returns a sequence number should return that number >>> - not a pointer to it. >>> >>> Which then makes that u32 *os_area_msg_seqno redundant and you can use >>> the function directly. >>> >>> IOW: >>> >>> static inline u32 snp_get_os_area_msg_seqno(struct snp_guest_dev *snp_dev) >>> { >>> return snp_dev->layout->os_area.msg_seqno_0 + snp_dev->vmpck_id; >> >> This patch removes setting of layour page in snp_dev structure. > > So? * Instead of using snp_dev->layout, we will need to access it using platform_data->layout structure. * Below will give incorrect value of sequence number, it will get VMPCK_0's sequence number and will add vmpck_id to that. Will work by fluke for VMPCK=0, but will fail for all other keys. return snp_dev->layout->os_area.msg_seqno_0 + snp_dev->vmpck_id; struct secrets_os_area { ... u32 msg_seqno_0; u32 msg_seqno_1; u32 msg_seqno_2; u32 msg_seqno_3; ... } * I am using vmpck_id to index to correct msg_seqno_* Changing this to struct secrets_os_area { ... u32 msg_seqno[VMPCK_MAX_NUM]; ... } > >> static inline u32 snp_get_os_area_msg_seqno(struct snp_guest_dev *snp_dev) >> { >> if (!platform_data) >> return NULL; >> >> return *(&platform_data->layout->os_area.msg_seqno_0 + vmpck_id); >> } > > What?! I can change the secrets_os_area like below to simplify things: struct secrets_os_area { ... u32 msg_seqno[VMPCK_MAX_NUM]; ... } static inline u32 snp_get_os_area_msg_seqno(struct snp_guest_dev *snp_dev) { if (!platform_data) return NULL; return platform_data->layout->os_area.msg_seqno[snp_dev->vmpck_id]; } > > This snp_get_os_area_msg_seqno() is a new function added by this patch. > >> I had a getter for getting the os_area_msg_seqno pointer, probably not >> a good function name. > > Probably you need to go back to the drawing board and think about how > this thing should look like. > >>> Do you see the imbalance in the APIs? >> >> The msg_seqno should only be incremented by 2 (always), that was the reason to avoid a setter. > > And what's wrong with the setter doing the incrementation so that > callers can't even get it wrong? Are you suggesting that setter should always increment by 2? static inline u32 snp_set_os_area_msg_seqno(struct snp_guest_dev *snp_dev) { ... os_area.msg_seqno[snp_dev->vmpck_id] += 2; ... } > > It sounds to me like you should redesign this sequence number handling > in a *separate* patch. Sure, let me rethink and will post it as separate patch. Regards Nikunj