Received-SPF: pass (google.com: domain of linux-kernel+bounces-146539-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 16 Apr 2024 11:06:58 +0200
From: Borislav Petkov <bp@alien8.de>
To: "Nikunj A. Dadhania" <nikunj@amd.com>
Cc: linux-kernel@vger.kernel.org, thomas.lendacky@amd.com, x86@kernel.org,
	kvm@vger.kernel.org, mingo@redhat.com, tglx@linutronix.de,
	dave.hansen@linux.intel.com, pgonda@google.com, seanjc@google.com,
	pbonzini@redhat.com
Subject: Re: [PATCH v8 04/16] virt: sev-guest: Add vmpck_id to snp_guest_dev
 struct
Message-ID: <20240416090658.GBZh4_sj7ursRtzijI@fat_crate.local>
References: <20240215113128.275608-1-nikunj@amd.com>
 <20240215113128.275608-5-nikunj@amd.com>
 <20240409102348.GBZhUXND0CDk7tGv8a@fat_crate.local>
 <74f17321-42ed-417c-ad24-8bc4e7207117@amd.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <74f17321-42ed-417c-ad24-8bc4e7207117@amd.com>

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?

> 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?!

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?

It sounds to me like you should redesign this sequence number handling
in a *separate* patch.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette