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.
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.
