bhyve/tpm: add emulation for crb register

[corvink]

Summary

This commit only adds the emulation. It doesn't enable the trap to those
registers. Enabling the trap yet, isn't possible. A tpm passthru device
maps the mmio region of the physical tpm device into the guest memory.
This mapping conflicts with the trap. A future commit will add the trap.

[markj]

usr.sbin/bhyve/tpm_intf_crb.c

300

Is it not important to validate the size of the write?

355

The comment doesn't explain why.

483

mr.name doesn't get copied. It'll become a pointer to stack garbage once this function returns.

508

"rw mutex" sounds like this should be a reader/writer lock, but presumably the mutex is really meant to serialize MMIO operations? Maybe mmio_mutex would be a better name?

[corvink]

• address markj's feedback

[corvink]

usr.sbin/bhyve/tpm_intf_crb.c

508

I've rechecked the usage of the mutex. It was useless as mem.c:access_memory already locks on mmio accesses.

[markj]

usr.sbin/bhyve/tpm_intf_crb.c

274

This can be replaced with

if (!(size == 1 || size == 2 || size == 4 || size == 8))
    return (EINVAL);
memcpy(dst, src, size);

292

Why bother trying to handle unaligned accesses at all? The code below becomes more complicated as a result (you need this is_offsetof macro), and its behaviour is hard to reason about.

349

warn/warnx don't need trailing newlines.

370

Suppose that the thread is currently processing a command. When it's done, it'll re-acquire the lock, then set crb->regs.ctrl_start.start = false, then go to sleep, so the signal is not acknowledged. Presumably that's not what we want here?

382

416

This can be defined on the stack. register_mem() just makes a copy of it, so there's no need for this to be a global variable.

[corvink]

usr.sbin/bhyve/tpm_intf_crb.c

370

When the thread is processing a command, the write is ignored by

			if (!start.start || crb->regs.ctrl_start.start)
				break;

That's the required behaviour by the TPM specification https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p05p_r14_pub.pdf

1. On a write of 0001h to the Start field:
   a. If the TPM is in the Command Reception state, the TPM SHALL transition to Command Execution and begin processing the command in the buffer.
   b. If the TPM is in any other State, the TPM SHALL ignore the write.
2. The TPM SHALL ignore writes of 0 to this field.
3. When the TPM completes command processing, the TPM SHALL clear this field to 0000h and transition to Command Completion.

[corvink]

• address feedback from markj

[markj]

usr.sbin/bhyve/tpm_intf_crb.c

370

Ok. Shouldn't we acquire the mutex before calling tpm_crb_mmiocpy() and checking !start.start || crb->regs.ctrl_start.start though?

[corvink]

usr.sbin/bhyve/tpm_intf_crb.c

370

I don't have a strong preference on that. The crb thread is only accessing cmd_size, cmd_addr, rsp_size, rsp_addr and data_buffer. My patch already acquires the mutex for accesses to those register.

[markj]

usr.sbin/bhyve/tpm_intf_crb.c

370

I think it would be better to avoid the data race nonetheless.

[corvink]

• acquire crb->mutex before writing and reading ctrl_start

[corvink]

@markj @jhb Any additional comments?

[markj]

usr.sbin/bhyve/tpm_intf_crb.c

367

370

398

Should this return error?

[corvink]

• fix style issues

[corvink]

• apply correct diff