Re: [PATCH gnumach] Add vm_pages_phys

[Sergey Bugaev]

Hello,

On Mon, Jan 29, 2024 at 11:59 PM Samuel Thibault
<samuel.thibault@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, &copy);
        assert(kr == KERN_SUCCESS);
        *pagespp = (rpc_phys_addr_array_t) copy;
}
*countp = count;

> +       *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