On Tue, Aug 30, 2022 at 10:49:11PM +0000, Sean Christopherson wrote:
> On Tue, Aug 30, 2022, Ricardo Koller wrote:
> > On Mon, Aug 29, 2022 at 07:25:08PM +0200, Andrew Jones wrote:
> > > On Tue, Aug 23, 2022 at 11:47:21PM +0000, Ricardo Koller wrote:
> > > I feel we'll be revisiting this frequently when more and more region types
> > > are desired. For example, Sean wants a read-only memory region for ucall
> > > exits. How about putting a mem slot array in struct kvm_vm, defining an
> > > enum to index it (which will expand), and then single helper function,
> > > something like
> > >
> > > inline struct userspace_mem_region *
> > > vm_get_mem_region(struct kvm_vm *vm, enum memslot_type mst)
> > > {
> > > return memslot2region(vm, vm->memslots[mst]);
> > > }
>
>
> > >
> > > > +
> > > > /* Minimum allocated guest virtual and physical addresses */
> > > > #define KVM_UTIL_MIN_VADDR 0x2000
> > > > #define KVM_GUEST_PAGE_TABLE_MIN_PADDR 0x180000
> > > > @@ -637,19 +662,51 @@ vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
> > > > vm_paddr_t paddr_min, uint32_t memslot);
> > > > vm_paddr_t vm_alloc_page_table(struct kvm_vm *vm);
> > > >
> > > > +#define MEM_PARAMS_MAX_MEMSLOTS 3
> > >
> > > And this becomes MEMSLOT_MAX of the enum proposed above
> > >
> > > enum memslot_type {
> > > MEMSLOT_CODE,
> > > MEMSLOT_PT,
> > > MEMSLOT_DATA,
>
> "memslot" is going to be confusing, e.g. MEMSLOT_MAX is some arbitrary selftests
> constant that has no relationship to maximum number of memslots.
>
> > > MEMSLOT_MAX,
>
> I dislike "max" because it's ambiguous, e.g. is it the maximum number of regions,
> or is the max valid region?
>
> Maybe something like this?
>
> enum kvm_mem_region_type {
> MEM_REGION_CODE
> ...
> NR_MEM_REGIONS,
> }
>
> > > > +#else
> > > > + .mode = VM_MODE_DEFAULT,
> > > > +#endif
> > > > + .region[0] = {
> > > > + .src_type = VM_MEM_SRC_ANONYMOUS,
> > > > + .guest_paddr = 0,
> > > > + .slot = 0,
> > > > + /*
> > > > + * 4mb when page size is 4kb. Note that vm_nr_pages_required(),
> > > > + * the function used by most tests to calculate guest memory
> > > > + * requirements uses around ~520 pages for more tests.
> > >
> > > ...requirements, currently returns ~520 pages for the majority of tests.
> > >
> > > > + */
> > > > + .npages = 1024,
> > >
> > > And here we double it, but it's still fragile. I see we override this
> > > in __vm_create() below though, so now I wonder why we set it at all.
> > >
> >
> > I would prefer having a default that can be used by a test as-is. WDYT?
> > or should we make it explicit that the default needs some updates?
>
> In that case, the default should be '0'. There are two users of ____vm_create().
> __vm_create() takes the number of "extra" pages and calculates the "base" number
> of pages. vm_create_barebones() passes '0', i.e. can use default.
>
> If '0' as a default is too weird, make it an illegal value and force the caller
> to define the number of pages.
Sounds good, will try '0', and will make sure that not defining the
number of pages results in a clear error.
>
> It's not a coincidence that those are the only two callers, ____vm_create() isn't
> intended to be used directly. If a test wants to create a VM just to create a VM,
> then it shoulid use vm_create_barebones(). If a test wants to doing anything
> remotely useful with the VM, it should use __vm_create() or something higher up
> the food chain.
Thank you both for the reviews. I think this is ready for a v6, so
sending one soon.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
