On Thu, Dec 23, 2021 at 05:35:37PM +0000, Sean Christopherson wrote:
> On Thu, Dec 23, 2021, Chao Peng wrote:
>
> > + struct file *file;
>
> Please use more descriptive names, shaving characters is not at all priority.
>
> > + u64 ofs;
>
> I believe this should be loff_t.
>
> struct file *private_file;
> struct loff_t private_offset;
>
> > };
> >
> > +static inline bool kvm_slot_is_private(const struct kvm_memory_slot *slot)
> > +{
> > + if (slot && (slot->flags & KVM_MEM_PRIVATE))
> > + return true;
> > + return false;
>
> return slot && (slot->flags & KVM_MEM_PRIVATE);
>
> > +}
> > +
> > static inline bool kvm_slot_dirty_track_enabled(const struct kvm_memory_slot *slot)
> > {
> > return slot->flags & KVM_MEM_LOG_DIRTY_PAGES;
> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > index 1daa45268de2..41434322fa23 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -103,6 +103,17 @@ struct kvm_userspace_memory_region {
> > __u64 userspace_addr; /* start of the userspace allocated memory */
> > };
> >
> > +struct kvm_userspace_memory_region_ext {
> > + __u32 slot;
> > + __u32 flags;
> > + __u64 guest_phys_addr;
> > + __u64 memory_size; /* bytes */
> > + __u64 userspace_addr; /* hva */
>
> Would it make sense to embed "struct kvm_userspace_memory_region"?
>
> > + __u64 ofs; /* offset into fd */
> > + __u32 fd;
>
> Again, use descriptive names, then comments like "offset into fd" are unnecessary.
>
> __u64 private_offset;
> __u32 private_fd;
My original thought is the same fields might be used for shared memslot
as well in future (e.g. there may be another KVM_MEM_* bit can reuse the
same fields for shared slot) so non private-specific name may sound
better. But definitely I have no objection and can use private_* names
for next version unless there is other objection.
Thanks,
Chao
>
> > + __u32 padding[5];
> > +};
> > +
> > /*
> > * The bit 0 ~ bit 15 of kvm_memory_region::flags are visible for userspace,
> > * other bits are reserved for kvm internal use which are defined in
> > @@ -110,6 +121,7 @@ struct kvm_userspace_memory_region {
> > */
> > #define KVM_MEM_LOG_DIRTY_PAGES (1UL << 0)
> > #define KVM_MEM_READONLY (1UL << 1)
> > +#define KVM_MEM_PRIVATE (1UL << 2)
> >
> > /* for KVM_IRQ_LINE */
> > struct kvm_irq_level {
> > --
> > 2.17.1
> >
