[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <91756da8-f2c3-482a-95ee-6208e1205502@linux.microsoft.com>
Date: Tue, 22 Oct 2024 15:25:00 +0530
From: Naman Jain <namjain@...ux.microsoft.com>
To: Saurabh Singh Sengar <ssengar@...ux.microsoft.com>
Cc: "K . Y . Srinivasan" <kys@...rosoft.com>,
Haiyang Zhang <haiyangz@...rosoft.com>, Wei Liu <wei.liu@...nel.org>,
Dexuan Cui <decui@...rosoft.com>, linux-hyperv@...r.kernel.org,
linux-kernel@...r.kernel.org, John Starks <jostarks@...rosoft.com>,
jacob.pan@...ux.microsoft.com,
Easwar Hariharan <eahariha@...ux.microsoft.com>
Subject: Re: [PATCH 1/2] Drivers: hv: vmbus: Wait for offers during boot
On 10/21/2024 10:27 AM, Saurabh Singh Sengar wrote:
> On Fri, Oct 18, 2024 at 04:58:10AM -0700, Naman Jain wrote:
>> Channels offers are requested during vmbus initialization and resume
>
> Nit: s/vmbus/VMBus
Thanks for reviewing Saurabh.
Noted this for next patch. Thanks
>
>> from hibernation. Add support to wait for all channel offers to be
>> delivered and processed before returning from vmbus_request_offers.
>> This is to support user mode (VTL0) in OpenHCL (A Linux based
>> paravisor for Confidential VMs) to ensure that all channel offers
>> are present when init begins in VTL0, and it knows which channels
>> the host has offered and which it has not.
>
> Usermode isn't necessarily of VTL0, and this issue was actually identified
> at a higher VTL in OpenHCL. However, this change isn't specific to OpenHCL,
> but is intended for general use. I would prefer if the commit message were
> either more generic or precisely aligned with the specific issue it's
> addressing.
>
I'll make it generic.
>>
>> This is in analogy to a PCI bus not returning from probe until it has
>> scanned all devices on the bus.
>>
>> Without this, user mode can race with vmbus initialization and miss
>> channel offers. User mode has no way to work around this other than
>> sleeping for a while, since there is no way to know when vmbus has
>> finished processing offers.
>>
>> With this added functionality, remove earlier logic which keeps track
>> of count of offered channels post resume from hibernation. Once all
>> offers delivered message is received, no further offers are going to
>> be received. Consequently, logic to prevent suspend from happening
>> after previous resume had missing offers, is also removed.
>>
>> Co-developed-by: John Starks <jostarks@...rosoft.com>
>> Signed-off-by: John Starks <jostarks@...rosoft.com>
>> Signed-off-by: Naman Jain <namjain@...ux.microsoft.com>
>> ---
>> drivers/hv/channel_mgmt.c | 38 +++++++++++++++++++++++---------------
>> drivers/hv/connection.c | 4 ++--
>> drivers/hv/hyperv_vmbus.h | 14 +++-----------
<..>
>> }
>>
>> + /* Wait for the host to send all offers. */
>> + while (wait_for_completion_timeout(
>> + &vmbus_connection.all_offers_delivered_event, msecs_to_jiffies(10 * 1000)) == 0) {
>
> Nit: Can simply put 10000 instead of 10*1000
Noted.
>
>> + pr_warn("timed out waiting for all offers to be delivered...\n");
>
> I know we are moving from async to sync, so earlier we never checked this.
> But what if some channel timed out do we want to handle this case ? Or put
> a comment why this is OK.
>
> We could set error from here as well, but I see vmbus_request_offers return value
> is never checked.
It seems to be a best effort way to get all the offers. If its not
received in time, I think we can print a warning and continue. I can add
that to a comment.
>
>> + }
>> +
>> + /*
>> + * Flush handling of offer messages (which may initiate work on
>> + * other work queues).
>> + */
Regards,
Naman
Powered by blists - more mailing lists