Re: [PATCH v3 1/3] KVM: guest_memfd: Treat memslot binding offset+size as unsigned values

[Ackerley Tng]

Sean Christopherson <seanjc@xxxxxxxxxx> writes:

>
> [...snip...]
>
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index bf9659a7b0f6..a1cb72e66288 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -640,15 +640,16 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
>  }
>
>  int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
> -		  unsigned int fd, loff_t offset)
> +		  unsigned int fd, uoff_t offset)
>  {
> -	loff_t size = slot->npages << PAGE_SHIFT;
> +	uoff_t size = slot->npages << PAGE_SHIFT;

I get why uoff_t is chosen for byte offset, but why is uoff_t also
picked for size, over size_t?

Not trying to nitpick, guest_memfd deals with lots of sizes, byte and
page offsets so I'd like to derive a mental model for using these types
in functions.

>  	unsigned long start, end;
>  	struct gmem_file *f;
>  	struct inode *inode;
>  	struct file *file;
>  	int r = -EINVAL;
>
> +	BUILD_BUG_ON(sizeof(gpa_t) != sizeof(offset));
>  	BUILD_BUG_ON(sizeof(gfn_t) != sizeof(slot->gmem.pgoff));
>

I never knew this was meant to show the concept/typing relationship
between gpa_t and byte offset, gfn_t and page offset. Can we add a
comment to explain the presence of BUILD_BUG_ON()?

Also, what's the rationale for picking BUILD_BUG_ON() over
static_assert()? static_assert() could be placed outside block scope
which makes this relationship declaration more general/global. Putting
it in the function makes it seem local (seems to suggest trying to
assert some guard for just this function).

>
> [...snip...]
>

Reviewed-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>