Re: [PATCH 1/2] Drivers: hv: vmbus: Wait for offers during boot

[Naman Jain <namjain@linux.microsoft.com>]

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@microsoft.com>
>> Signed-off-by: John Starks <jostarks@microsoft.com>
>> Signed-off-by: Naman Jain <namjain@linux.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