Messages in this thread | | | From | Tianyu Lan <> | Subject | Re: [PATCH V3 04/13] HV: Mark vmbus ring buffer visible to host in Isolation VM | Date | Sun, 15 Aug 2021 23:21:14 +0800 |
| |
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.
| |