On Tue, Jan 30, 2024 at 1:38 AM Sergey Bugaev <buga...@gmail.com> wrote:
> Hello,
>
> On Mon, Jan 29, 2024 at 11:59 PM Samuel Thibault
> <samuel.thiba...@gnu.org> wrote:
> > Please notably review the RPC part, I really don't know that much about
> > mig.
>
> Some nitpicks inline. Flávio, does what I'm saying below make sense to you?
>
> > +/*
> > + * vm_pages_phys returns information about a region of memory
> > + */
> > +kern_return_t vm_pages_phys(
> > + host_t host,
> > + vm_map_t map,
> > + vm_address_t address,
> > + vm_size_t size,
>
> This is supposed to be a number of pages rather than VM region size,
> right? Then it would be better off with a different name, and perhaps
> type. Alternatively perhaps it _should_ be a VM region size?
>
> > + rpc_phys_addr_array_t *pagespp,
> > + mach_msg_type_number_t *countp)
> > +{
> > + if (host == HOST_NULL)
> > + return KERN_INVALID_HOST;
> > + if (map == VM_MAP_NULL)
> > + return KERN_INVALID_TASK;
> > +
> > + if (!page_aligned(address))
> > + return KERN_INVALID_ARGUMENT;
> > + if (!page_aligned(size))
> > + return KERN_INVALID_ARGUMENT;
> > +
> > + mach_msg_type_number_t count = atop(size), cur;
> > +
> > + if (*countp < count)
> > + return KERN_INVALID_ARGUMENT;
>
> It's not an error if the passed-in buffer is smaller than what you're
> planning to return. This is not even something that the user side of
> the RPC controls, the buffer is fixed-size and allocated by MIG in the
> reply message -- unless you use CountInOut, that is.
>
> You're supposed to allocate your own buffer in this case, something like
> this:
>
> rpc_phys_addr_array_t pages = *pagespp;
> if (*countp < count) {
> vm_offset_t allocated;
> kr = kmem_alloc(ipc_kernel_map, &allocated, some_size);
> if (kr != KERN_SUCCESS)
> return kr;
> pages = (rpc_phys_addr_array_t) allocated;
> }
>
> and at the end:
>
> if (pages != *pagespp) {
> vm_map_copy_t copy;
> kr = vm_map_copyin(ipc_kernel_map, (vm_offset_t) pages,
> some_size, TRUE, ©);
> assert(kr == KERN_SUCCESS);
> *pagespp = (rpc_phys_addr_array_t) copy;
> }
> *countp = count;
>
Yes, this is my understanding as well. The server stub *countp is always
256 here so if count is larger than that, it is better to allocate a larger
array.
The client stubs will handle the larger array correctly.
>
> > + *countp = count;
> > +
> > + return KERN_SUCCESS;
> > +}
>
> OK. My current understanding is that you indeed do not need to
> deallocate either the 'vm_map_t map' argument or the task port right
> behind it on the success path here. But perhaps it would be clearer
> with a comment stating that this is intentional (and not simply
> forgotten).
>
> Sergey
>
