Re: [PATCH v3 03/30] drivers: hv: dxgkrnl: Add VM bus message support, initialize VM bus channels.

[Wei Liu]

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.