lkml.org 
[lkml]   [2021]   [Aug]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH V3 04/13] HV: Mark vmbus ring buffer visible to host in Isolation VM
Date


On 8/13/2021 6:20 AM, Michael Kelley wrote:
>> @@ -474,6 +482,13 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
>> if (ret)
>> return ret;
>>
>> + ret = set_memory_decrypted((unsigned long)kbuffer,
>> + HVPFN_UP(size));
>> + if (ret) {
>> + pr_warn("Failed to set host visibility.\n");
> Enhance this message a bit. "Failed to set host visibility for new GPADL\n"
> and also output the value of ret.

OK. This looks better. Thanks.

>
>> @@ -539,6 +554,10 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
>> /* At this point, we received the gpadl created msg */
>> *gpadl_handle = gpadlmsg->gpadl;
>>
>> + channel->gpadl_array[index].size = size;
>> + channel->gpadl_array[index].buffer = kbuffer;
>> + channel->gpadl_array[index].gpadlhandle = *gpadl_handle;
>> +
> I can see the merits of transparently stashing the memory address and size
> that will be needed by vmbus_teardown_gpadl(), so that the callers of
> __vmbus_establish_gpadl() don't have to worry about it. But doing the
> stashing transparently is somewhat messy.
>
> Given that the callers are already have memory allocated to save the
> GPADL handle, a little refactoring would make for a much cleaner solution.
> Instead of having memory allocated for the 32-bit GPADL handle, callers
> should allocate the slightly larger struct vmbus_gpadl that you've
> defined below. The calling interfaces can be updated to take a pointer
> to this structure instead of a pointer to the 32-bit GPADL handle, and
> you can save the memory address and size right along with the GPADL
> handle. This approach touches a few more files, but I think there are
> only two callers outside of the channel management code -- netvsc
> and hv_uio -- so it's not a big change.

Yes, this is a good suggestion and Will update in the next version.

>
>> @@ -859,6 +886,19 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle)
>> spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
>>
>> kfree(info);
>> +
>> + /* Find gpadl buffer virtual address and size. */
>> + for (i = 0; i < VMBUS_GPADL_RANGE_COUNT; i++)
>> + if (channel->gpadl_array[i].gpadlhandle == gpadl_handle)
>> + break;
>> +
>> + if (set_memory_encrypted((unsigned long)channel->gpadl_array[i].buffer,
>> + HVPFN_UP(channel->gpadl_array[i].size)))
>> + pr_warn("Fail to set mem host visibility.\n");
> Enhance this message a bit: "Failed to set host visibility in GPADL teardown\n".
> Also output the returned error code to help in debugging any occurrences of
> a failure
Yes, agree. Will update.

Thanks.

\
 
 \ /
  Last update: 2021-08-15 17:22    [W:0.120 / U:0.640 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site