linux-kernel - Re: [RFC PATCH 10/16] xen/balloon: support ballooning in xenhost

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aa050a5f-1897-64c0-d19d-d0f31b8d6c62@oracle.com>
Date:   Tue, 18 Jun 2019 19:24:59 -0700
From:   Ankur Arora <ankur.a.arora@...cle.com>
To:     Juergen Gross <jgross@...e.com>, linux-kernel@...r.kernel.org,
        xen-devel@...ts.xenproject.org
Cc:     pbonzini@...hat.com, boris.ostrovsky@...cle.com,
        konrad.wilk@...cle.com, sstabellini@...nel.org,
        joao.m.martins@...cle.com
Subject: Re: [RFC PATCH 10/16] xen/balloon: support ballooning in xenhost_t



On 6/17/19 2:28 AM, Juergen Gross wrote:
> On 09.05.19 19:25, Ankur Arora wrote:
>> Xen ballooning uses hollow struct pages (with the underlying GFNs being
>> populated/unpopulated via hypercalls) which are used by the grant logic
>> to map grants from other domains.
>>
>> This patch allows the default xenhost to provide an alternate ballooning
>> allocation mechanism. This is expected to be useful for local xenhosts
>> (type xenhost_r0) because unlike Xen, where there is an external
>> hypervisor which can change the memory underneath a GFN, that is not
>> possible when the hypervisor is running in the same address space
>> as the entity doing the ballooning.
>>
>> Co-developed-by: Ankur Arora <ankur.a.arora@...cle.com>
>> Signed-off-by: Joao Martins <joao.m.martins@...cle.com>
>> Signed-off-by: Ankur Arora <ankur.a.arora@...cle.com>
>> ---
>>   arch/x86/xen/enlighten_hvm.c       |  7 +++++++
>>   arch/x86/xen/enlighten_pv.c        |  8 ++++++++
>>   drivers/xen/balloon.c              | 19 ++++++++++++++++---
>>   drivers/xen/grant-table.c          |  4 ++--
>>   drivers/xen/privcmd.c              |  4 ++--
>>   drivers/xen/xen-selfballoon.c      |  2 ++
>>   drivers/xen/xenbus/xenbus_client.c |  6 +++---
>>   drivers/xen/xlate_mmu.c            |  4 ++--
>>   include/xen/balloon.h              |  4 ++--
>>   include/xen/xenhost.h              | 19 +++++++++++++++++++
>>   10 files changed, 63 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
>> index 5ef4d6ad920d..08becf574743 100644
>> --- a/drivers/xen/balloon.c
>> +++ b/drivers/xen/balloon.c
>> @@ -63,6 +63,7 @@
>>   #include <asm/tlb.h>
>>   #include <xen/interface/xen.h>
>> +#include <xen/xenhost.h>
>>   #include <asm/xen/hypervisor.h>
>>   #include <asm/xen/hypercall.h>
>> @@ -583,12 +584,21 @@ static int add_ballooned_pages(int nr_pages)
>>    * @pages: pages returned
>>    * @return 0 on success, error otherwise
>>    */
>> -int alloc_xenballooned_pages(int nr_pages, struct page **pages)
>> +int alloc_xenballooned_pages(xenhost_t *xh, int nr_pages, struct page 
>> **pages)
>>   {
>>       int pgno = 0;
>>       struct page *page;
>>       int ret;
>> +    /*
>> +     * xenmem transactions for remote xenhost are disallowed.
>> +     */
>> +    if (xh->type == xenhost_r2)
>> +        return -EINVAL;
> 
> Why don't you set a dummy function returning -EINVAL into the xenhost_r2
> structure instead?
Will do. And, same for the two comments below.

Ankur

> 
>> +
>> +    if (xh->ops->alloc_ballooned_pages)
>> +        return xh->ops->alloc_ballooned_pages(xh, nr_pages, pages);
>> +
> 
> Please make alloc_xenballooned_pages() an inline wrapper and use the
> current implmentaion as the default. This avoids another if ().
> 
>>       mutex_lock(&balloon_mutex);
>>       balloon_stats.target_unpopulated += nr_pages;
>> @@ -620,7 +630,7 @@ int alloc_xenballooned_pages(int nr_pages, struct 
>> page **pages)
>>       return 0;
>>    out_undo:
>>       mutex_unlock(&balloon_mutex);
>> -    free_xenballooned_pages(pgno, pages);
>> +    free_xenballooned_pages(xh, pgno, pages);
>>       return ret;
>>   }
>>   EXPORT_SYMBOL(alloc_xenballooned_pages);
>> @@ -630,10 +640,13 @@ EXPORT_SYMBOL(alloc_xenballooned_pages);
>>    * @nr_pages: Number of pages
>>    * @pages: pages to return
>>    */
>> -void free_xenballooned_pages(int nr_pages, struct page **pages)
>> +void free_xenballooned_pages(xenhost_t *xh, int nr_pages, struct page 
>> **pages)
>>   {
>>       int i;
>> +    if (xh->ops->free_ballooned_pages)
>> +        return xh->ops->free_ballooned_pages(xh, nr_pages, pages);
>> +
> 
> Same again: please use an inline wrapper.
> 
> 
> Juergen