From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by smtp.subspace.kernel.org (Postfix) with ESMTP id CFEF5125D2 for ; Fri, 25 Aug 2023 18:26:03 +0000 (UTC) Received: from [192.168.0.5] (71-212-112-68.tukw.qwest.net [71.212.112.68]) by linux.microsoft.com (Postfix) with ESMTPSA id E99C62127C95; Fri, 25 Aug 2023 11:26:02 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com E99C62127C95 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1692987963; bh=i/LcYatUMQg88pTskF3rrDjHam8btTKsOaforNox3cY=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=p97fGzQv0o9SeAtgAZ2SZ5rRMB7jI5VGEkz9QIN1l31MP5IqV9EJqibSEgwG5bFPj UG5QUFFeGy0VD4/NFO2D1opM+z30iFiUImxaWfX2o96tTXxtE60/QSxhI8cmd8k9cA miX8qYJhfygj2ws4RHDr3sgzHwPw5HXrSFmDjDpc= Message-ID: <68344e40-45aa-41d1-9df2-26f12db9e109@linux.microsoft.com> Date: Fri, 25 Aug 2023 11:26:02 -0700 Precedence: bulk X-Mailing-List: patches@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 15/15] Drivers: hv: Add modules to expose /dev/mshv to VMMs running on Hyper-V Content-Language: en-US To: Saurabh Singh Sengar , "linux-hyperv@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "x86@kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-arch@vger.kernel.org" Cc: "patches@lists.linux.dev" , "Michael Kelley (LINUX)" , KY Srinivasan , "wei.liu@kernel.org" , Haiyang Zhang , Dexuan Cui , "apais@linux.microsoft.com" , Tianyu Lan , "ssengar@linux.microsoft.com" , MUKESH RATHOR , "stanislav.kinsburskiy@gmail.com" , "jinankjain@linux.microsoft.com" , vkuznets , "tglx@linutronix.de" , "mingo@redhat.com" , "bp@alien8.de" , "dave.hansen@linux.intel.com" , "hpa@zytor.com" , "will@kernel.org" , "catalin.marinas@arm.com" References: <1692309711-5573-1-git-send-email-nunodasneves@linux.microsoft.com> <1692309711-5573-16-git-send-email-nunodasneves@linux.microsoft.com> <664aec4c-7ea9-447f-afab-9e31e9e106c1@linux.microsoft.com> <8978223c-c5e8-4235-a0ed-2031583c2751@linux.microsoft.com> From: Nuno Das Neves In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 8/23/2023 12:40 AM, Saurabh Singh Sengar wrote: > > >> -----Original Message----- >> From: Nuno Das Neves >> Sent: Wednesday, August 23, 2023 1:49 AM >> To: Saurabh Singh Sengar ; linux- >> hyperv@vger.kernel.org; linux-kernel@vger.kernel.org; x86@kernel.org; linux- >> arm-kernel@lists.infradead.org; linux-arch@vger.kernel.org >> Cc: patches@lists.linux.dev; Michael Kelley (LINUX) >> ; KY Srinivasan ; >> wei.liu@kernel.org; Haiyang Zhang ; Dexuan Cui >> ; apais@linux.microsoft.com; Tianyu Lan >> ; ssengar@linux.microsoft.com; MUKESH >> RATHOR ; stanislav.kinsburskiy@gmail.com; >> jinankjain@linux.microsoft.com; vkuznets ; >> tglx@linutronix.de; mingo@redhat.com; bp@alien8.de; >> dave.hansen@linux.intel.com; hpa@zytor.com; will@kernel.org; >> catalin.marinas@arm.com >> Subject: Re: [PATCH v2 15/15] Drivers: hv: Add modules to expose /dev/mshv >> to VMMs running on Hyper-V >> >> On 8/19/2023 10:19 PM, Saurabh Singh Sengar wrote: >>> >>> >>>> -----Original Message----- >>>> From: Nuno Das Neves >>>> Sent: Saturday, August 19, 2023 12:30 AM >>>> To: Saurabh Singh Sengar ; linux- >>>> hyperv@vger.kernel.org; linux-kernel@vger.kernel.org; x86@kernel.org; >>>> linux- arm-kernel@lists.infradead.org; linux-arch@vger.kernel.org >>>> Cc: patches@lists.linux.dev; Michael Kelley (LINUX) >>>> ; KY Srinivasan ; >>>> wei.liu@kernel.org; Haiyang Zhang ; Dexuan >>>> Cui ; apais@linux.microsoft.com; Tianyu Lan >>>> ; ssengar@linux.microsoft.com; MUKESH >>>> RATHOR ; >> stanislav.kinsburskiy@gmail.com; >>>> jinankjain@linux.microsoft.com; vkuznets ; >>>> tglx@linutronix.de; mingo@redhat.com; bp@alien8.de; >>>> dave.hansen@linux.intel.com; hpa@zytor.com; will@kernel.org; >>>> catalin.marinas@arm.com >>>> Subject: Re: [PATCH v2 15/15] Drivers: hv: Add modules to expose >>>> /dev/mshv to VMMs running on Hyper-V >>>> >>>> On 8/18/2023 6:08 AM, Saurabh Singh Sengar wrote: >>>>>> + >>>>>> +config MSHV_VTL >>>>>> + tristate "Microsoft Hyper-V VTL driver" >>>>>> + depends on MSHV >>>>>> + select HYPERV_VTL_MODE >>>>>> + select TRANSPARENT_HUGEPAGE >>>>> >>>>> TRANSPARENT_HUGEPAGE can be avoided for now. >>>>> >>>> >>>> I will remove it in the next version. Thanks. >>>>>> + >>>>>> +#define HV_GET_REGISTER_BATCH_SIZE \ >>>>>> + (HV_HYP_PAGE_SIZE / sizeof(union hv_register_value)) >>>>>> +#define HV_SET_REGISTER_BATCH_SIZE \ >>>>>> + ((HV_HYP_PAGE_SIZE - sizeof(struct hv_input_set_vp_registers)) \ >>>>>> + / sizeof(struct hv_register_assoc)) >>>>>> + >>>>>> +int hv_call_get_vp_registers( >>>>>> + u32 vp_index, >>>>>> + u64 partition_id, >>>>>> + u16 count, >>>>>> + union hv_input_vtl input_vtl, >>>>>> + struct hv_register_assoc *registers) { >>>>>> + struct hv_input_get_vp_registers *input_page; >>>>>> + union hv_register_value *output_page; >>>>>> + u16 completed = 0; >>>>>> + unsigned long remaining = count; >>>>>> + int rep_count, i; >>>>>> + u64 status; >>>>>> + unsigned long flags; >>>>>> + >>>>>> + local_irq_save(flags); >>>>>> + >>>>>> + input_page = *this_cpu_ptr(hyperv_pcpu_input_arg); >>>>>> + output_page = *this_cpu_ptr(hyperv_pcpu_output_arg); >>>>>> + >>>>>> + input_page->partition_id = partition_id; >>>>>> + input_page->vp_index = vp_index; >>>>>> + input_page->input_vtl.as_uint8 = input_vtl.as_uint8; >>>>>> + input_page->rsvd_z8 = 0; >>>>>> + input_page->rsvd_z16 = 0; >>>>>> + >>>>>> + while (remaining) { >>>>>> + rep_count = min(remaining, HV_GET_REGISTER_BATCH_SIZE); >>>>>> + for (i = 0; i < rep_count; ++i) >>>>>> + input_page->names[i] = registers[i].name; >>>>>> + >>>>>> + status = hv_do_rep_hypercall(HVCALL_GET_VP_REGISTERS, >>>>>> rep_count, >>>>>> + 0, input_page, output_page); >>>>> >>>>> Is there any possibility that count value is passed 0 by mistake ? >>>>> In that case status will remain uninitialized. >>>>> >>>> >>>> These lines ensure rep_count is never 0 here: >>>> >>>> while (remaining) { >>>> rep_count = min(remaining, HV_GET_REGISTER_BATCH_SIZE); >>>> >>>> Remaining can't be 0 or the loop would exit, and >>>> HV_GET_REGISTER_BATCH_SIZE is not 0, or we would never get any >> registers. >>> >>> There is a parameter in this function "count". I was checking if there >>> is any possibility that is passed as 0 by mistake ? this will lead to >>> "remaining" value as 0 and loop will never execute. Which results using >> "status" uninitialized later in the function. >>> >>> >> >> Ah ok, thanks! I will change it to return immediately in case count is 0. > > Or you can initialize status with appropriate error value, either way is fine. > Please consider fixing the same issue in hv_call_set_vp_registers as well. > Thanks again - noted.