diff mbox series

[v4,09/15] Drivers: hv: Introduce hv_output_arg_exists in hv_common.c

Message ID 1696010501-24584-10-git-send-email-nunodasneves@linux.microsoft.com (mailing list archive)
State New, archived
Headers show
Series Introduce /dev/mshv drivers | expand

Commit Message

Nuno Das Neves Sept. 29, 2023, 6:01 p.m. UTC
This is a more flexible approach for determining whether to allocate the
output page.

Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
Acked-by: Wei Liu <wei.liu@kernel.org>
---
 drivers/hv/hv_common.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

Comments

Alex Ionescu Oct. 2, 2023, 7:29 p.m. UTC | #1
Hi Nuno,

Is it possible to simply change to always allocating the output page?
For example, the output page could be needed in scenarios where Linux
is not running as the root partition, since certain hypercalls that a
guest can make will still require one (I realize that's not the case
_today_, but I don't believe this optimization buys much).

Best regards,
Alex Ionescu

Best regards,
Alex Ionescu


On Fri, Sep 29, 2023 at 2:02 PM Nuno Das Neves
<nunodasneves@linux.microsoft.com> wrote:
>
> This is a more flexible approach for determining whether to allocate the
> output page.
>
> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
> Acked-by: Wei Liu <wei.liu@kernel.org>
> ---
>  drivers/hv/hv_common.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index 39077841d518..3f6f23e4c579 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -58,6 +58,14 @@ EXPORT_SYMBOL_GPL(hyperv_pcpu_input_arg);
>  void * __percpu *hyperv_pcpu_output_arg;
>  EXPORT_SYMBOL_GPL(hyperv_pcpu_output_arg);
>
> +/*
> + * Determine whether output arg is needed
> + */
> +static inline bool hv_output_arg_exists(void)
> +{
> +       return hv_root_partition ? true : false;
> +}
> +
>  static void hv_kmsg_dump_unregister(void);
>
>  static struct ctl_table_header *hv_ctl_table_hdr;
> @@ -342,10 +350,12 @@ int __init hv_common_init(void)
>         hyperv_pcpu_input_arg = alloc_percpu(void  *);
>         BUG_ON(!hyperv_pcpu_input_arg);
>
> -       /* Allocate the per-CPU state for output arg for root */
> -       if (hv_root_partition) {
> +       if (hv_output_arg_exists()) {
>                 hyperv_pcpu_output_arg = alloc_percpu(void *);
>                 BUG_ON(!hyperv_pcpu_output_arg);
> +       }
> +
> +       if (hv_root_partition) {
>                 hv_synic_eventring_tail = alloc_percpu(u8 *);
>                 BUG_ON(hv_synic_eventring_tail == NULL);
>         }
> @@ -375,7 +385,7 @@ int hv_common_cpu_init(unsigned int cpu)
>         u8 **synic_eventring_tail;
>         u64 msr_vp_index;
>         gfp_t flags;
> -       int pgcount = hv_root_partition ? 2 : 1;
> +       int pgcount = hv_output_arg_exists() ? 2 : 1;
>         void *mem;
>         int ret;
>
> @@ -393,9 +403,12 @@ int hv_common_cpu_init(unsigned int cpu)
>                 if (!mem)
>                         return -ENOMEM;
>
> -               if (hv_root_partition) {
> +               if (hv_output_arg_exists()) {
>                         outputarg = (void **)this_cpu_ptr(hyperv_pcpu_output_arg);
>                         *outputarg = (char *)mem + HV_HYP_PAGE_SIZE;
> +               }
> +
> +               if (hv_root_partition) {
>                         synic_eventring_tail = (u8 **)this_cpu_ptr(hv_synic_eventring_tail);
>                         *synic_eventring_tail = kcalloc(HV_SYNIC_SINT_COUNT, sizeof(u8),
>                                                         flags);
> --
> 2.25.1
>
>
Nuno Das Neves Oct. 4, 2023, 6:27 p.m. UTC | #2
On 10/2/2023 12:29 PM, Alex Ionescu wrote:
> Hi Nuno,
> 
> Is it possible to simply change to always allocating the output page?
> For example, the output page could be needed in scenarios where Linux
> is not running as the root partition, since certain hypercalls that a
> guest can make will still require one (I realize that's not the case
> _today_, but I don't believe this optimization buys much).

I agree - it would indeed simplify the code, and guests will probably
make use of it sooner or later.

Happy to make that change if Hyper-V guest maintainers agree.
Long, Dexuan, Michael, what do you think?

Thanks,
Nuno

> Best regards,
> Alex Ionescu
> 
> 
> On Fri, Sep 29, 2023 at 2:02 PM Nuno Das Neves
> <nunodasneves@linux.microsoft.com> wrote:
>>
>> This is a more flexible approach for determining whether to allocate the
>> output page.
>>
>> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
>> Acked-by: Wei Liu <wei.liu@kernel.org>
>> ---
>>   drivers/hv/hv_common.c | 21 +++++++++++++++++----
>>   1 file changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
>> index 39077841d518..3f6f23e4c579 100644
>> --- a/drivers/hv/hv_common.c
>> +++ b/drivers/hv/hv_common.c
>> @@ -58,6 +58,14 @@ EXPORT_SYMBOL_GPL(hyperv_pcpu_input_arg);
>>   void * __percpu *hyperv_pcpu_output_arg;
>>   EXPORT_SYMBOL_GPL(hyperv_pcpu_output_arg);
>>
>> +/*
>> + * Determine whether output arg is needed
>> + */
>> +static inline bool hv_output_arg_exists(void)
>> +{
>> +       return hv_root_partition ? true : false;
>> +}
>> +
>>   static void hv_kmsg_dump_unregister(void);
>>
>>   static struct ctl_table_header *hv_ctl_table_hdr;
>> @@ -342,10 +350,12 @@ int __init hv_common_init(void)
>>          hyperv_pcpu_input_arg = alloc_percpu(void  *);
>>          BUG_ON(!hyperv_pcpu_input_arg);
>>
>> -       /* Allocate the per-CPU state for output arg for root */
>> -       if (hv_root_partition) {
>> +       if (hv_output_arg_exists()) {
>>                  hyperv_pcpu_output_arg = alloc_percpu(void *);
>>                  BUG_ON(!hyperv_pcpu_output_arg);
>> +       }
>> +
>> +       if (hv_root_partition) {
>>                  hv_synic_eventring_tail = alloc_percpu(u8 *);
>>                  BUG_ON(hv_synic_eventring_tail == NULL);
>>          }
>> @@ -375,7 +385,7 @@ int hv_common_cpu_init(unsigned int cpu)
>>          u8 **synic_eventring_tail;
>>          u64 msr_vp_index;
>>          gfp_t flags;
>> -       int pgcount = hv_root_partition ? 2 : 1;
>> +       int pgcount = hv_output_arg_exists() ? 2 : 1;
>>          void *mem;
>>          int ret;
>>
>> @@ -393,9 +403,12 @@ int hv_common_cpu_init(unsigned int cpu)
>>                  if (!mem)
>>                          return -ENOMEM;
>>
>> -               if (hv_root_partition) {
>> +               if (hv_output_arg_exists()) {
>>                          outputarg = (void **)this_cpu_ptr(hyperv_pcpu_output_arg);
>>                          *outputarg = (char *)mem + HV_HYP_PAGE_SIZE;
>> +               }
>> +
>> +               if (hv_root_partition) {
>>                          synic_eventring_tail = (u8 **)this_cpu_ptr(hv_synic_eventring_tail);
>>                          *synic_eventring_tail = kcalloc(HV_SYNIC_SINT_COUNT, sizeof(u8),
>>                                                          flags);
>> --
>> 2.25.1
>>
>>
Long Li Oct. 6, 2023, 6:13 p.m. UTC | #3
> Subject: Re: [PATCH v4 09/15] Drivers: hv: Introduce hv_output_arg_exists in
> hv_common.c
> 
> On 10/2/2023 12:29 PM, Alex Ionescu wrote:
> > Hi Nuno,
> >
> > Is it possible to simply change to always allocating the output page?
> > For example, the output page could be needed in scenarios where Linux
> > is not running as the root partition, since certain hypercalls that a
> > guest can make will still require one (I realize that's not the case
> > _today_, but I don't believe this optimization buys much).
> 
> I agree - it would indeed simplify the code, and guests will probably make use of it
> sooner or later.
> 
> Happy to make that change if Hyper-V guest maintainers agree.
> Long, Dexuan, Michael, what do you think?

There is no use case as of today for guest VMs. And it allocates extra memory that are never used when running as guest VMs.

I suggest keep this code unchanged as is.

Thanks,
Long
diff mbox series

Patch

diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
index 39077841d518..3f6f23e4c579 100644
--- a/drivers/hv/hv_common.c
+++ b/drivers/hv/hv_common.c
@@ -58,6 +58,14 @@  EXPORT_SYMBOL_GPL(hyperv_pcpu_input_arg);
 void * __percpu *hyperv_pcpu_output_arg;
 EXPORT_SYMBOL_GPL(hyperv_pcpu_output_arg);
 
+/*
+ * Determine whether output arg is needed
+ */
+static inline bool hv_output_arg_exists(void)
+{
+	return hv_root_partition ? true : false;
+}
+
 static void hv_kmsg_dump_unregister(void);
 
 static struct ctl_table_header *hv_ctl_table_hdr;
@@ -342,10 +350,12 @@  int __init hv_common_init(void)
 	hyperv_pcpu_input_arg = alloc_percpu(void  *);
 	BUG_ON(!hyperv_pcpu_input_arg);
 
-	/* Allocate the per-CPU state for output arg for root */
-	if (hv_root_partition) {
+	if (hv_output_arg_exists()) {
 		hyperv_pcpu_output_arg = alloc_percpu(void *);
 		BUG_ON(!hyperv_pcpu_output_arg);
+	}
+
+	if (hv_root_partition) {
 		hv_synic_eventring_tail = alloc_percpu(u8 *);
 		BUG_ON(hv_synic_eventring_tail == NULL);
 	}
@@ -375,7 +385,7 @@  int hv_common_cpu_init(unsigned int cpu)
 	u8 **synic_eventring_tail;
 	u64 msr_vp_index;
 	gfp_t flags;
-	int pgcount = hv_root_partition ? 2 : 1;
+	int pgcount = hv_output_arg_exists() ? 2 : 1;
 	void *mem;
 	int ret;
 
@@ -393,9 +403,12 @@  int hv_common_cpu_init(unsigned int cpu)
 		if (!mem)
 			return -ENOMEM;
 
-		if (hv_root_partition) {
+		if (hv_output_arg_exists()) {
 			outputarg = (void **)this_cpu_ptr(hyperv_pcpu_output_arg);
 			*outputarg = (char *)mem + HV_HYP_PAGE_SIZE;
+		}
+
+		if (hv_root_partition) {
 			synic_eventring_tail = (u8 **)this_cpu_ptr(hv_synic_eventring_tail);
 			*synic_eventring_tail = kcalloc(HV_SYNIC_SINT_COUNT, sizeof(u8),
 							flags);