vmm: Initialize AMD IOMMU command buffers

[chuck]

Summary

The driver communicates with the AMD IOMMU by writing to the tail of a
fixed length command ring buffer. After issuing cmd_max commands, the
tail pointer wraps back to the beginning of the ring buffer. Now, each
command buffer entry will contain content from previous commands which
may set bits in fields marked as Reserved for the current command. In
some cases, the hardware will return an ILLEGAL_COMMAND_ERROR event when
this occurs.

Fix is to memset the command buffer prior to use.

PR: 270966
MFC after: 2 weeks

[markj]

Is there any reason not to do this memset() in amdvi_get_cmd_tail()?

[chuck]

Other functions call amdvi_get_cmd_tail() prior to calling the functions which would then format the command entry. Putting the memset() in amdvi_get_cmd_tail() would clear the command entry twice.

[markj]

In D53692#1226028, @chuck wrote:

Other functions call amdvi_get_cmd_tail() prior to calling the functions which would then format the command entry. Putting the memset() in amdvi_get_cmd_tail() would clear the command entry twice.

Are you referring to amdvi_inv_domain()? That's the only caller of amdvi_get_cmd_tail() that you didn't patch, but I can't see why that call invokes amdvi_get_cmd_tail() at all. It doesn't do anything with the returned pointer.

[chuck]

You are correct. I had (mistakenly) convinced myself that pattern occurred in more places. I'll move as suggested and delete the occurrence in amdvi_inv_domain

[chuck]

Address review feedback

[markj]

sys/amd64/vmm/amd/amdvi_hw.c

277

I think the NULL check is not needed. We already assert that softc->cmd != NULL, and tail is just softc->cmd + offset.

[chuck]

OK. I had that in because most (all?) calls to this function do a KASSERT(cmd != NULL) immediately afterwards. Should I delete the KASSERTs in the calling functions as well?

[markj]

In D53692#1226065, @chuck wrote:

OK. I had that in because most (all?) calls to this function do a KASSERT(cmd != NULL) immediately afterwards. Should I delete the KASSERTs in the calling functions as well?

Personally I dislike assertions of the form ptr != NULL since if that's false you'll get a crash anyway, and it's usually easy to tell that it was caused by a null pointer dereference. So, from my perspective they are just noise. if you want to remove them, please go ahead.

[chuck]

Remove kasserts and move memset to central location

[kib]

sys/amd64/vmm/amd/amdvi_hw.c

269

This code is really insists on asserting any pointer is not NULL.