Re: [Xen-devel] [PATCHv2 2/2] xen/privcmd: add ioctls for locking/unlocking hypercall buffers

[David Vrabel <david.vrabel@xxxxxxxxxx>]

On 04/08/16 17:02, Jan Beulich wrote:
>>>> On 04.08.16 at 17:16, <david.vrabel@xxxxxxxxxx> wrote:
>> Using mlock() for hypercall buffers is not sufficient since mlocked
>> pages are still subject to compaction and page migration.  Page
>> migration can be prevented by taking additional references to the
>> pages.
>>
>> Introduce two new ioctls: IOCTL_PRIVCMD_HCALL_BUF_LOCK and
>> IOCTL_PRIVCMD_HCALL_BUF_UNLOCK which get and put the necessary page
>> references.  The buffers do not need to be page aligned and they may
>> overlap with other buffers.  However, it is not possible to partially
>> unlock a buffer (i.e., the LOCK/UNLOCK must always be paired).  Any
>> locked buffers are automatically unlocked when the file descriptor is
>> closed.
>>
>> An alternative approach would be to extend the driver with an ioctl to
>> populate a VMA with "special", non-migratable pages.  But the
>> LOCK/UNLOCK ioctls are more flexible as they allow any page to be used
>> for hypercalls (e.g., stack, mmap'd files, etc.).  This could be used
>> to minimize bouncing for performance critical hypercalls.
>>
>> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> with two remarks: Do you not care about compat mode callers?

No.  Compat mode callers can't make hypercalls since the hypervisor ABI
structures aren't converted anywhere.

> case you indeed mean to not support them, does the default
> handling ensure they will see an error instead of you mis-interpreting
> (and overrunning) the presented structure?

I will look at this.

>> +static struct privcmd_hbuf *privcmd_hbuf_alloc(struct privcmd_hcall_buf 
>> *buf)
>> +{
>> +    struct privcmd_hbuf *hbuf;
>> +    unsigned long start, end, nr_pages;
>> +    int ret;
>> +
>> +    start = (unsigned long)buf->start & PAGE_MASK;
>> +    end = ALIGN((unsigned long)buf->start + buf->len, PAGE_SIZE);
>> +    nr_pages = (end - start) / PAGE_SIZE;
> 
> So entry re-use is based on the actual length the caller passed,
> while page tracking of course is page granular. Wouldn't it make
> sense to re-use entries based on the pages they encompass,
> which in particular would mean that two distinct buffers on the
> same page would get just a single entry?

If you make the entries page aligned, you can't give always give an
error when the user unlocks something of the wrong size.

David

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel