diff mbox series

[RFC,08/19] HostMem: Add private property to indicate to use kvm gmem

Message ID 20230731162201.271114-9-xiaoyao.li@intel.com (mailing list archive)
State New, archived
Headers show
Series QEMU gmem implemention | expand

Commit Message

Xiaoyao Li July 31, 2023, 4:21 p.m. UTC
From: Isaku Yamahata <isaku.yamahata@intel.com>

Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 backends/hostmem.c       | 18 ++++++++++++++++++
 include/sysemu/hostmem.h |  2 +-
 qapi/qom.json            |  4 ++++
 3 files changed, 23 insertions(+), 1 deletion(-)

Comments

Xiaoyao Li Aug. 1, 2023, 2:54 p.m. UTC | #1
On 8/1/2023 1:22 AM, Markus Armbruster wrote:
> Xiaoyao Li <xiaoyao.li@intel.com> writes:
> 
>> From: Isaku Yamahata <isaku.yamahata@intel.com>
>>
>> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> 
> [...]
> 
>> diff --git a/qapi/qom.json b/qapi/qom.json
>> index 7f92ea43e8e1..e0b2044e3d20 100644
>> --- a/qapi/qom.json
>> +++ b/qapi/qom.json
>> @@ -605,6 +605,9 @@
>>   # @reserve: if true, reserve swap space (or huge pages) if applicable
>>   #     (default: true) (since 6.1)
>>   #
>> +# @private: if true, use KVM gmem private memory
>> +#           (default: false) (since 8.1)
>> +#
> 
> Please format like
> 
>     # @private: if true, use KVM gmem private memory (default: false)
>     #     (since 8.1)
> 
> to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
> to conform to current conventions).

will do it in next version.

Thanks!
-Xiaoyao

>>   # @size: size of the memory region in bytes
>>   #
>>   # @x-use-canonical-path-for-ramblock-id: if true, the canonical path
>> @@ -631,6 +634,7 @@
>>               '*prealloc-context': 'str',
>>               '*share': 'bool',
>>               '*reserve': 'bool',
>> +            '*private': 'bool',
>>               'size': 'size',
>>               '*x-use-canonical-path-for-ramblock-id': 'bool' } }
>
Daniel P. Berrangé Aug. 1, 2023, 2:57 p.m. UTC | #2
On Mon, Jul 31, 2023 at 07:22:05PM +0200, Markus Armbruster wrote:
> Xiaoyao Li <xiaoyao.li@intel.com> writes:
> 
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> >
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> 
> [...]
> 
> > diff --git a/qapi/qom.json b/qapi/qom.json
> > index 7f92ea43e8e1..e0b2044e3d20 100644
> > --- a/qapi/qom.json
> > +++ b/qapi/qom.json
> > @@ -605,6 +605,9 @@
> >  # @reserve: if true, reserve swap space (or huge pages) if applicable
> >  #     (default: true) (since 6.1)
> >  #
> > +# @private: if true, use KVM gmem private memory
> > +#           (default: false) (since 8.1)
> > +#
> 
> Please format like
> 
>    # @private: if true, use KVM gmem private memory (default: false)
>    #     (since 8.1)

Also QEMU 8.1.0 is in freeze right now, so there's no chance
of these patches making 8.1.0. IOW, use "since 8.2" as the
next release you might achieve merge for.

With regards,
Daniel
David Hildenbrand Aug. 1, 2023, 5:21 p.m. UTC | #3
On 31.07.23 18:21, Xiaoyao Li wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>   backends/hostmem.c       | 18 ++++++++++++++++++
>   include/sysemu/hostmem.h |  2 +-
>   qapi/qom.json            |  4 ++++
>   3 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index 747e7838c031..dbdbb0aafd45 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -461,6 +461,20 @@ static void host_memory_backend_set_reserve(Object *o, bool value, Error **errp)
>       }
>       backend->reserve = value;
>   }
> +
> +static bool host_memory_backend_get_private(Object *o, Error **errp)
> +{
> +    HostMemoryBackend *backend = MEMORY_BACKEND(o);
> +
> +    return backend->private;
> +}
> +
> +static void host_memory_backend_set_private(Object *o, bool value, Error **errp)
> +{
> +    HostMemoryBackend *backend = MEMORY_BACKEND(o);
> +
> +    backend->private = value;
> +}
>   #endif /* CONFIG_LINUX */
>   
>   static bool
> @@ -541,6 +555,10 @@ host_memory_backend_class_init(ObjectClass *oc, void *data)
>           host_memory_backend_get_reserve, host_memory_backend_set_reserve);
>       object_class_property_set_description(oc, "reserve",
>           "Reserve swap space (or huge pages) if applicable");
> +    object_class_property_add_bool(oc, "private",
> +        host_memory_backend_get_private, host_memory_backend_set_private);
> +    object_class_property_set_description(oc, "private",
> +        "Use KVM gmem private memory");
>   #endif /* CONFIG_LINUX */
>       /*
>        * Do not delete/rename option. This option must be considered stable
> diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
> index 39326f1d4f9c..d88970395618 100644
> --- a/include/sysemu/hostmem.h
> +++ b/include/sysemu/hostmem.h
> @@ -65,7 +65,7 @@ struct HostMemoryBackend {
>       /* protected */
>       uint64_t size;
>       bool merge, dump, use_canonical_path;
> -    bool prealloc, is_mapped, share, reserve;
> +    bool prealloc, is_mapped, share, reserve, private;
>       uint32_t prealloc_threads;
>       ThreadContext *prealloc_context;
>       DECLARE_BITMAP(host_nodes, MAX_NODES + 1);
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 7f92ea43e8e1..e0b2044e3d20 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -605,6 +605,9 @@
>   # @reserve: if true, reserve swap space (or huge pages) if applicable
>   #     (default: true) (since 6.1)
>   #
> +# @private: if true, use KVM gmem private memory
> +#           (default: false) (since 8.1)
> +#

But that's not what any of this does.

This patch only adds a property and doesn't even explain what it intends 
to achieve with that.

How will it be used from a user? What will it affect internally? What 
will it modify in regards of the memory backend?

That all should go into the surprisingly empty patch description.
Xiaoyao Li Aug. 2, 2023, 8:03 a.m. UTC | #4
On 8/2/2023 1:21 AM, David Hildenbrand wrote:
> On 31.07.23 18:21, Xiaoyao Li wrote:
>> From: Isaku Yamahata <isaku.yamahata@intel.com>
>>
>> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   backends/hostmem.c       | 18 ++++++++++++++++++
>>   include/sysemu/hostmem.h |  2 +-
>>   qapi/qom.json            |  4 ++++
>>   3 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>> index 747e7838c031..dbdbb0aafd45 100644
>> --- a/backends/hostmem.c
>> +++ b/backends/hostmem.c
>> @@ -461,6 +461,20 @@ static void 
>> host_memory_backend_set_reserve(Object *o, bool value, Error **errp)
>>       }
>>       backend->reserve = value;
>>   }
>> +
>> +static bool host_memory_backend_get_private(Object *o, Error **errp)
>> +{
>> +    HostMemoryBackend *backend = MEMORY_BACKEND(o);
>> +
>> +    return backend->private;
>> +}
>> +
>> +static void host_memory_backend_set_private(Object *o, bool value, 
>> Error **errp)
>> +{
>> +    HostMemoryBackend *backend = MEMORY_BACKEND(o);
>> +
>> +    backend->private = value;
>> +}
>>   #endif /* CONFIG_LINUX */
>>   static bool
>> @@ -541,6 +555,10 @@ host_memory_backend_class_init(ObjectClass *oc, 
>> void *data)
>>           host_memory_backend_get_reserve, 
>> host_memory_backend_set_reserve);
>>       object_class_property_set_description(oc, "reserve",
>>           "Reserve swap space (or huge pages) if applicable");
>> +    object_class_property_add_bool(oc, "private",
>> +        host_memory_backend_get_private, 
>> host_memory_backend_set_private);
>> +    object_class_property_set_description(oc, "private",
>> +        "Use KVM gmem private memory");
>>   #endif /* CONFIG_LINUX */
>>       /*
>>        * Do not delete/rename option. This option must be considered 
>> stable
>> diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
>> index 39326f1d4f9c..d88970395618 100644
>> --- a/include/sysemu/hostmem.h
>> +++ b/include/sysemu/hostmem.h
>> @@ -65,7 +65,7 @@ struct HostMemoryBackend {
>>       /* protected */
>>       uint64_t size;
>>       bool merge, dump, use_canonical_path;
>> -    bool prealloc, is_mapped, share, reserve;
>> +    bool prealloc, is_mapped, share, reserve, private;
>>       uint32_t prealloc_threads;
>>       ThreadContext *prealloc_context;
>>       DECLARE_BITMAP(host_nodes, MAX_NODES + 1);
>> diff --git a/qapi/qom.json b/qapi/qom.json
>> index 7f92ea43e8e1..e0b2044e3d20 100644
>> --- a/qapi/qom.json
>> +++ b/qapi/qom.json
>> @@ -605,6 +605,9 @@
>>   # @reserve: if true, reserve swap space (or huge pages) if applicable
>>   #     (default: true) (since 6.1)
>>   #
>> +# @private: if true, use KVM gmem private memory
>> +#           (default: false) (since 8.1)
>> +#
> 
> But that's not what any of this does.
> 
> This patch only adds a property and doesn't even explain what it intends 
> to achieve with that.
> 
> How will it be used from a user? What will it affect internally? What 
> will it modify in regards of the memory backend?

How it will be used is in the next patch, patch 09.

for kvm_x86_sw_protected_vm type VM, it will allocate private gmem with 
KVM ioctl if the memory backend has property "private" on.

> That all should go into the surprisingly empty patch description.

I'm sorry. I admit the empty commit message is really bad.
Xiaoyao Li Aug. 2, 2023, 8:04 a.m. UTC | #5
On 8/1/2023 10:57 PM, Daniel P. Berrangé wrote:
> On Mon, Jul 31, 2023 at 07:22:05PM +0200, Markus Armbruster wrote:
>> Xiaoyao Li <xiaoyao.li@intel.com> writes:
>>
>>> From: Isaku Yamahata <isaku.yamahata@intel.com>
>>>
>>> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>
>> [...]
>>
>>> diff --git a/qapi/qom.json b/qapi/qom.json
>>> index 7f92ea43e8e1..e0b2044e3d20 100644
>>> --- a/qapi/qom.json
>>> +++ b/qapi/qom.json
>>> @@ -605,6 +605,9 @@
>>>   # @reserve: if true, reserve swap space (or huge pages) if applicable
>>>   #     (default: true) (since 6.1)
>>>   #
>>> +# @private: if true, use KVM gmem private memory
>>> +#           (default: false) (since 8.1)
>>> +#
>>
>> Please format like
>>
>>     # @private: if true, use KVM gmem private memory (default: false)
>>     #     (since 8.1)
> 
> Also QEMU 8.1.0 is in freeze right now, so there's no chance
> of these patches making 8.1.0. IOW, use "since 8.2" as the
> next release you might achieve merge for.

Thanks for pointing it out. Will do it in next version.

> With regards,
> Daniel
David Hildenbrand Aug. 2, 2023, 2:14 p.m. UTC | #6
On 02.08.23 10:03, Xiaoyao Li wrote:
> On 8/2/2023 1:21 AM, David Hildenbrand wrote:
>> On 31.07.23 18:21, Xiaoyao Li wrote:
>>> From: Isaku Yamahata <isaku.yamahata@intel.com>
>>>
>>> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>> ---
>>>    backends/hostmem.c       | 18 ++++++++++++++++++
>>>    include/sysemu/hostmem.h |  2 +-
>>>    qapi/qom.json            |  4 ++++
>>>    3 files changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>>> index 747e7838c031..dbdbb0aafd45 100644
>>> --- a/backends/hostmem.c
>>> +++ b/backends/hostmem.c
>>> @@ -461,6 +461,20 @@ static void
>>> host_memory_backend_set_reserve(Object *o, bool value, Error **errp)
>>>        }
>>>        backend->reserve = value;
>>>    }
>>> +
>>> +static bool host_memory_backend_get_private(Object *o, Error **errp)
>>> +{
>>> +    HostMemoryBackend *backend = MEMORY_BACKEND(o);
>>> +
>>> +    return backend->private;
>>> +}
>>> +
>>> +static void host_memory_backend_set_private(Object *o, bool value,
>>> Error **errp)
>>> +{
>>> +    HostMemoryBackend *backend = MEMORY_BACKEND(o);
>>> +
>>> +    backend->private = value;
>>> +}
>>>    #endif /* CONFIG_LINUX */
>>>    static bool
>>> @@ -541,6 +555,10 @@ host_memory_backend_class_init(ObjectClass *oc,
>>> void *data)
>>>            host_memory_backend_get_reserve,
>>> host_memory_backend_set_reserve);
>>>        object_class_property_set_description(oc, "reserve",
>>>            "Reserve swap space (or huge pages) if applicable");
>>> +    object_class_property_add_bool(oc, "private",
>>> +        host_memory_backend_get_private,
>>> host_memory_backend_set_private);
>>> +    object_class_property_set_description(oc, "private",
>>> +        "Use KVM gmem private memory");
>>>    #endif /* CONFIG_LINUX */
>>>        /*
>>>         * Do not delete/rename option. This option must be considered
>>> stable
>>> diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
>>> index 39326f1d4f9c..d88970395618 100644
>>> --- a/include/sysemu/hostmem.h
>>> +++ b/include/sysemu/hostmem.h
>>> @@ -65,7 +65,7 @@ struct HostMemoryBackend {
>>>        /* protected */
>>>        uint64_t size;
>>>        bool merge, dump, use_canonical_path;
>>> -    bool prealloc, is_mapped, share, reserve;
>>> +    bool prealloc, is_mapped, share, reserve, private;
>>>        uint32_t prealloc_threads;
>>>        ThreadContext *prealloc_context;
>>>        DECLARE_BITMAP(host_nodes, MAX_NODES + 1);
>>> diff --git a/qapi/qom.json b/qapi/qom.json
>>> index 7f92ea43e8e1..e0b2044e3d20 100644
>>> --- a/qapi/qom.json
>>> +++ b/qapi/qom.json
>>> @@ -605,6 +605,9 @@
>>>    # @reserve: if true, reserve swap space (or huge pages) if applicable
>>>    #     (default: true) (since 6.1)
>>>    #
>>> +# @private: if true, use KVM gmem private memory
>>> +#           (default: false) (since 8.1)
>>> +#
>>
>> But that's not what any of this does.
>>
>> This patch only adds a property and doesn't even explain what it intends
>> to achieve with that.
>>
>> How will it be used from a user? What will it affect internally? What
>> will it modify in regards of the memory backend?
> 
> How it will be used is in the next patch, patch 09.
> 
> for kvm_x86_sw_protected_vm type VM, it will allocate private gmem with
> KVM ioctl if the memory backend has property "private" on.

It feels wired up the wrong way.

When creating/initializing the memory backend, we should also take care 
of allocating the gmem_fd, for example, by doing some gmem allocation 
callback, ideally *internally* creating the RAM memory region / RAMBlock.

And we should fail if that is impossible (gmem does not apply to the VM) 
or creating the gmem_fd failed for other reason.

Like passing a RAM_GMEM flag to memory_region_init_ram_flags_nomigrate() 
in ram_backend_memory_alloc(), to then handle it internally, failing if 
there is an error.
Isaku Yamahata Aug. 2, 2023, 10:53 p.m. UTC | #7
On Wed, Aug 02, 2023 at 04:14:29PM +0200,
David Hildenbrand <david@redhat.com> wrote:

> On 02.08.23 10:03, Xiaoyao Li wrote:
> > On 8/2/2023 1:21 AM, David Hildenbrand wrote:
> > > On 31.07.23 18:21, Xiaoyao Li wrote:
> > > > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > > > 
> > > > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > > > ---
> > > >    backends/hostmem.c       | 18 ++++++++++++++++++
> > > >    include/sysemu/hostmem.h |  2 +-
> > > >    qapi/qom.json            |  4 ++++
> > > >    3 files changed, 23 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/backends/hostmem.c b/backends/hostmem.c
> > > > index 747e7838c031..dbdbb0aafd45 100644
> > > > --- a/backends/hostmem.c
> > > > +++ b/backends/hostmem.c
> > > > @@ -461,6 +461,20 @@ static void
> > > > host_memory_backend_set_reserve(Object *o, bool value, Error **errp)
> > > >        }
> > > >        backend->reserve = value;
> > > >    }
> > > > +
> > > > +static bool host_memory_backend_get_private(Object *o, Error **errp)
> > > > +{
> > > > +    HostMemoryBackend *backend = MEMORY_BACKEND(o);
> > > > +
> > > > +    return backend->private;
> > > > +}
> > > > +
> > > > +static void host_memory_backend_set_private(Object *o, bool value,
> > > > Error **errp)
> > > > +{
> > > > +    HostMemoryBackend *backend = MEMORY_BACKEND(o);
> > > > +
> > > > +    backend->private = value;
> > > > +}
> > > >    #endif /* CONFIG_LINUX */
> > > >    static bool
> > > > @@ -541,6 +555,10 @@ host_memory_backend_class_init(ObjectClass *oc,
> > > > void *data)
> > > >            host_memory_backend_get_reserve,
> > > > host_memory_backend_set_reserve);
> > > >        object_class_property_set_description(oc, "reserve",
> > > >            "Reserve swap space (or huge pages) if applicable");
> > > > +    object_class_property_add_bool(oc, "private",
> > > > +        host_memory_backend_get_private,
> > > > host_memory_backend_set_private);
> > > > +    object_class_property_set_description(oc, "private",
> > > > +        "Use KVM gmem private memory");
> > > >    #endif /* CONFIG_LINUX */
> > > >        /*
> > > >         * Do not delete/rename option. This option must be considered
> > > > stable
> > > > diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
> > > > index 39326f1d4f9c..d88970395618 100644
> > > > --- a/include/sysemu/hostmem.h
> > > > +++ b/include/sysemu/hostmem.h
> > > > @@ -65,7 +65,7 @@ struct HostMemoryBackend {
> > > >        /* protected */
> > > >        uint64_t size;
> > > >        bool merge, dump, use_canonical_path;
> > > > -    bool prealloc, is_mapped, share, reserve;
> > > > +    bool prealloc, is_mapped, share, reserve, private;
> > > >        uint32_t prealloc_threads;
> > > >        ThreadContext *prealloc_context;
> > > >        DECLARE_BITMAP(host_nodes, MAX_NODES + 1);
> > > > diff --git a/qapi/qom.json b/qapi/qom.json
> > > > index 7f92ea43e8e1..e0b2044e3d20 100644
> > > > --- a/qapi/qom.json
> > > > +++ b/qapi/qom.json
> > > > @@ -605,6 +605,9 @@
> > > >    # @reserve: if true, reserve swap space (or huge pages) if applicable
> > > >    #     (default: true) (since 6.1)
> > > >    #
> > > > +# @private: if true, use KVM gmem private memory
> > > > +#           (default: false) (since 8.1)
> > > > +#
> > > 
> > > But that's not what any of this does.
> > > 
> > > This patch only adds a property and doesn't even explain what it intends
> > > to achieve with that.
> > > 
> > > How will it be used from a user? What will it affect internally? What
> > > will it modify in regards of the memory backend?
> > 
> > How it will be used is in the next patch, patch 09.
> > 
> > for kvm_x86_sw_protected_vm type VM, it will allocate private gmem with
> > KVM ioctl if the memory backend has property "private" on.
> 
> It feels wired up the wrong way.
> 
> When creating/initializing the memory backend, we should also take care of
> allocating the gmem_fd, for example, by doing some gmem allocation callback,
> ideally *internally* creating the RAM memory region / RAMBlock.
> 
> And we should fail if that is impossible (gmem does not apply to the VM) or
> creating the gmem_fd failed for other reason.
> 
> Like passing a RAM_GMEM flag to memory_region_init_ram_flags_nomigrate() in
> ram_backend_memory_alloc(), to then handle it internally, failing if there
> is an error.

KVM gmem is tied to VM. not before creating VM. We have to delay of the
allocation of kvm gmem until VM initialization.

Hmm, one options is to move gmem_fd from RAMBlock to KVMSlot.  Handle the
allocation of KVM gmem (issuing KVM gmem ioctl) there. i.e. in
kvm_set_phys_mem() or kvm_region_add() (or whatever functions of KVM memory
listener).  Maybe we can drop ram_block_convert_range() and can have KVM
specific logic instead.

We still need a way for user to specify which guest memory region is subject
to KVM gmem, though.
David Hildenbrand Aug. 3, 2023, 1:05 p.m. UTC | #8
On 03.08.23 00:53, Isaku Yamahata wrote:
> On Wed, Aug 02, 2023 at 04:14:29PM +0200,
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 02.08.23 10:03, Xiaoyao Li wrote:
>>> On 8/2/2023 1:21 AM, David Hildenbrand wrote:
>>>> On 31.07.23 18:21, Xiaoyao Li wrote:
>>>>> From: Isaku Yamahata <isaku.yamahata@intel.com>
>>>>>
>>>>> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
>>>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>>>> ---
>>>>>     backends/hostmem.c       | 18 ++++++++++++++++++
>>>>>     include/sysemu/hostmem.h |  2 +-
>>>>>     qapi/qom.json            |  4 ++++
>>>>>     3 files changed, 23 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>>>>> index 747e7838c031..dbdbb0aafd45 100644
>>>>> --- a/backends/hostmem.c
>>>>> +++ b/backends/hostmem.c
>>>>> @@ -461,6 +461,20 @@ static void
>>>>> host_memory_backend_set_reserve(Object *o, bool value, Error **errp)
>>>>>         }
>>>>>         backend->reserve = value;
>>>>>     }
>>>>> +
>>>>> +static bool host_memory_backend_get_private(Object *o, Error **errp)
>>>>> +{
>>>>> +    HostMemoryBackend *backend = MEMORY_BACKEND(o);
>>>>> +
>>>>> +    return backend->private;
>>>>> +}
>>>>> +
>>>>> +static void host_memory_backend_set_private(Object *o, bool value,
>>>>> Error **errp)
>>>>> +{
>>>>> +    HostMemoryBackend *backend = MEMORY_BACKEND(o);
>>>>> +
>>>>> +    backend->private = value;
>>>>> +}
>>>>>     #endif /* CONFIG_LINUX */
>>>>>     static bool
>>>>> @@ -541,6 +555,10 @@ host_memory_backend_class_init(ObjectClass *oc,
>>>>> void *data)
>>>>>             host_memory_backend_get_reserve,
>>>>> host_memory_backend_set_reserve);
>>>>>         object_class_property_set_description(oc, "reserve",
>>>>>             "Reserve swap space (or huge pages) if applicable");
>>>>> +    object_class_property_add_bool(oc, "private",
>>>>> +        host_memory_backend_get_private,
>>>>> host_memory_backend_set_private);
>>>>> +    object_class_property_set_description(oc, "private",
>>>>> +        "Use KVM gmem private memory");
>>>>>     #endif /* CONFIG_LINUX */
>>>>>         /*
>>>>>          * Do not delete/rename option. This option must be considered
>>>>> stable
>>>>> diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
>>>>> index 39326f1d4f9c..d88970395618 100644
>>>>> --- a/include/sysemu/hostmem.h
>>>>> +++ b/include/sysemu/hostmem.h
>>>>> @@ -65,7 +65,7 @@ struct HostMemoryBackend {
>>>>>         /* protected */
>>>>>         uint64_t size;
>>>>>         bool merge, dump, use_canonical_path;
>>>>> -    bool prealloc, is_mapped, share, reserve;
>>>>> +    bool prealloc, is_mapped, share, reserve, private;
>>>>>         uint32_t prealloc_threads;
>>>>>         ThreadContext *prealloc_context;
>>>>>         DECLARE_BITMAP(host_nodes, MAX_NODES + 1);
>>>>> diff --git a/qapi/qom.json b/qapi/qom.json
>>>>> index 7f92ea43e8e1..e0b2044e3d20 100644
>>>>> --- a/qapi/qom.json
>>>>> +++ b/qapi/qom.json
>>>>> @@ -605,6 +605,9 @@
>>>>>     # @reserve: if true, reserve swap space (or huge pages) if applicable
>>>>>     #     (default: true) (since 6.1)
>>>>>     #
>>>>> +# @private: if true, use KVM gmem private memory
>>>>> +#           (default: false) (since 8.1)
>>>>> +#
>>>>
>>>> But that's not what any of this does.
>>>>
>>>> This patch only adds a property and doesn't even explain what it intends
>>>> to achieve with that.
>>>>
>>>> How will it be used from a user? What will it affect internally? What
>>>> will it modify in regards of the memory backend?
>>>
>>> How it will be used is in the next patch, patch 09.
>>>
>>> for kvm_x86_sw_protected_vm type VM, it will allocate private gmem with
>>> KVM ioctl if the memory backend has property "private" on.
>>
>> It feels wired up the wrong way.
>>
>> When creating/initializing the memory backend, we should also take care of
>> allocating the gmem_fd, for example, by doing some gmem allocation callback,
>> ideally *internally* creating the RAM memory region / RAMBlock.
>>
>> And we should fail if that is impossible (gmem does not apply to the VM) or
>> creating the gmem_fd failed for other reason.
>>
>> Like passing a RAM_GMEM flag to memory_region_init_ram_flags_nomigrate() in
>> ram_backend_memory_alloc(), to then handle it internally, failing if there
>> is an error.
> 
> KVM gmem is tied to VM. not before creating VM. We have to delay of the
> allocation of kvm gmem until VM initialization.

In vl.c, the flow is

1) Create machine: qemu_create_machine()
2) Configure KVM: configure_accelerators()
3) Create backends: qemu_create_late_backends()

Staring at object_create_early(), "memory-backend-" area always late.

So maybe, at the time memory backends are created+initialized, 
everything you need  is already in place.

> 
> Hmm, one options is to move gmem_fd from RAMBlock to KVMSlot.  Handle the
> allocation of KVM gmem (issuing KVM gmem ioctl) there. i.e. in
> kvm_set_phys_mem() or kvm_region_add() (or whatever functions of KVM memory
> listener).  Maybe we can drop ram_block_convert_range() and can have KVM
> specific logic instead.

Might be doable as well.

> 
> We still need a way for user to specify which guest memory region is subject
> to KVM gmem, though.

Let's minimize the gmem hacks and come up with something clean.
diff mbox series

Patch

diff --git a/backends/hostmem.c b/backends/hostmem.c
index 747e7838c031..dbdbb0aafd45 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -461,6 +461,20 @@  static void host_memory_backend_set_reserve(Object *o, bool value, Error **errp)
     }
     backend->reserve = value;
 }
+
+static bool host_memory_backend_get_private(Object *o, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(o);
+
+    return backend->private;
+}
+
+static void host_memory_backend_set_private(Object *o, bool value, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(o);
+
+    backend->private = value;
+}
 #endif /* CONFIG_LINUX */
 
 static bool
@@ -541,6 +555,10 @@  host_memory_backend_class_init(ObjectClass *oc, void *data)
         host_memory_backend_get_reserve, host_memory_backend_set_reserve);
     object_class_property_set_description(oc, "reserve",
         "Reserve swap space (or huge pages) if applicable");
+    object_class_property_add_bool(oc, "private",
+        host_memory_backend_get_private, host_memory_backend_set_private);
+    object_class_property_set_description(oc, "private",
+        "Use KVM gmem private memory");
 #endif /* CONFIG_LINUX */
     /*
      * Do not delete/rename option. This option must be considered stable
diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
index 39326f1d4f9c..d88970395618 100644
--- a/include/sysemu/hostmem.h
+++ b/include/sysemu/hostmem.h
@@ -65,7 +65,7 @@  struct HostMemoryBackend {
     /* protected */
     uint64_t size;
     bool merge, dump, use_canonical_path;
-    bool prealloc, is_mapped, share, reserve;
+    bool prealloc, is_mapped, share, reserve, private;
     uint32_t prealloc_threads;
     ThreadContext *prealloc_context;
     DECLARE_BITMAP(host_nodes, MAX_NODES + 1);
diff --git a/qapi/qom.json b/qapi/qom.json
index 7f92ea43e8e1..e0b2044e3d20 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -605,6 +605,9 @@ 
 # @reserve: if true, reserve swap space (or huge pages) if applicable
 #     (default: true) (since 6.1)
 #
+# @private: if true, use KVM gmem private memory
+#           (default: false) (since 8.1)
+#
 # @size: size of the memory region in bytes
 #
 # @x-use-canonical-path-for-ramblock-id: if true, the canonical path
@@ -631,6 +634,7 @@ 
             '*prealloc-context': 'str',
             '*share': 'bool',
             '*reserve': 'bool',
+            '*private': 'bool',
             'size': 'size',
             '*x-use-canonical-path-for-ramblock-id': 'bool' } }