The subject line is too long. Also, no period at the end please.
You can write it as:
drivers: hv: dxgkrnl: Add VMBus message support and initialize channels
On Tue, Mar 01, 2022 at 11:45:50AM -0800, Iouri Tarassov wrote:
[...]
> +
> +struct dxgvmbusmsgres {
> +/* Points to the allocated buffer */
> + struct dxgvmb_ext_header *hdr;
> +/* Points to dxgkvmb_command_vm_to_host or dxgkvmb_command_vgpu_to_host */
> + void *msg;
> +/* The vm bus channel, used to pass the message to the host */
> + struct dxgvmbuschannel *channel;
> +/* Message size in bytes including the header, the payload and the result */
> + u32 size;
> +/* Result buffer size in bytes */
> + u32 res_size;
> +/* Points to the result within the allocated buffer */
> + void *res;
Please align the comments with their fields.
> +};
> +
[...]
> +static void process_inband_packet(struct dxgvmbuschannel *channel,
> + struct vmpacket_descriptor *desc)
> +{
> + u32 packet_length = hv_pkt_datalen(desc);
> + struct dxgkvmb_command_host_to_vm *packet;
> +
> + if (packet_length < sizeof(struct dxgkvmb_command_host_to_vm)) {
> + pr_err("Invalid global packet");
> + } else {
> + packet = hv_pkt_data(desc);
> + pr_debug("global packet %d",
> + packet->command_type);
Unnecessary line wrap.
> + switch (packet->command_type) {
> + case DXGK_VMBCOMMAND_SIGNALGUESTEVENT:
> + case DXGK_VMBCOMMAND_SIGNALGUESTEVENTPASSIVE:
> + break;
> + case DXGK_VMBCOMMAND_SENDWNFNOTIFICATION:
> + break;
> + default:
> + pr_err("unexpected host message %d",
> + packet->command_type);
> + }
> + }
> +}
> +
[...]
> +
> +/* Receive callback for messages from the host */
> +void dxgvmbuschannel_receive(void *ctx)
> +{
> + struct dxgvmbuschannel *channel = ctx;
> + struct vmpacket_descriptor *desc;
> + u32 packet_length = 0;
> +
> + foreach_vmbus_pkt(desc, channel->channel) {
> + packet_length = hv_pkt_datalen(desc);
> + pr_debug("next packet (id, size, type): %llu %d %d",
> + desc->trans_id, packet_length, desc->type);
> + if (desc->type == VM_PKT_COMP) {
> + process_completion_packet(channel, desc);
> + } else {
> + if (desc->type != VM_PKT_DATA_INBAND)
> + pr_err("unexpected packet type");
This can potentially flood the guest if the backend is misbehaving.
We've seen flooding before so would definitely not want more of it.
Please consider using the ratelimit version pr_err.
The same comment goes for all other pr calls in repeatedly called paths.
I can see the value of having precise output from the pr_debug a few
lines above though.
> + else
> + process_inband_packet(channel, desc);
> + }
> + }
> +}
> +
[...]
> +int dxgvmb_send_set_iospace_region(u64 start, u64 len,
> + struct vmbus_gpadl *shared_mem_gpadl)
> +{
> + int ret;
> + struct dxgkvmb_command_setiospaceregion *command;
> + struct dxgvmbusmsg msg;
> +
> + ret = init_message(&msg, NULL, sizeof(*command));
> + if (ret)
> + return ret;
> + command = (void *)msg.msg;
> +
> + ret = dxgglobal_acquire_channel_lock();
> + if (ret < 0)
> + goto cleanup;
> +
> + command_vm_to_host_init1(&command->hdr,
> + DXGK_VMBCOMMAND_SETIOSPACEREGION);
> + command->start = start;
> + command->length = len;
> + if (command->shared_page_gpadl)
> + command->shared_page_gpadl = shared_mem_gpadl->gpadl_handle;
shared_mem_gpadl should be checked to be non-null. There is at least one
call site passes 0 to it.
> + ret = dxgvmb_send_sync_msg_ntstatus(&dxgglobal->channel, msg.hdr,
> + msg.size);
> + if (ret < 0)
> + pr_err("send_set_iospace_region failed %x", ret);
> +
> + dxgglobal_release_channel_lock();
> +cleanup:
> + free_message(&msg, NULL);
> + if (ret)
> + pr_debug("err: %s %d", __func__, ret);
> + return ret;
> +}
> +
[...]
> +
> +
> +#define NT_SUCCESS(status) (status.v >= 0)
> +
> +#ifndef DEBUG
> +
> +#define DXGKRNL_ASSERT(exp)
> +
> +#else
> +
> +#define DXGKRNL_ASSERT(exp) \
> +do { \
> + if (!(exp)) { \
> + dump_stack(); \
> + BUG_ON(true); \
> + } \
> +} while (0)
You can just use BUG_ON(exp), right? BUG_ON calls panic, which already
dumps the stack when CONFIG_DEBUG_VERBOSE is set.
Thanks,
Wei.
