Re: [PATCH v5 5/5] iommufd/vdevice: add TSM request ioctl

[Aneesh Kumar K . V]

"Dan Williams (nvidia)" <djbw@xxxxxxxxxx> writes:

> Aneesh Kumar K.V wrote:
> [..]
>> > I think we can wait to move it to its own IOMMU operation unless/until
>> > there is a need to set RUN outside of an explicit guest request, right?
>> 
>> Something like the below? (the diff against this series)
>> 
>> I have not yet integrated this into the full CCA patchset for testing,
>> but I wanted to make sure we are aligned on the UAPI.
> [..]
>> -static bool iommufd_vdevice_tsm_req_scope_valid(u32 scope)
>> +static bool iommufd_vdevice_tsm_req_arch_valid(u32 tvm_arch)
>>  {
>> -	if (scope > IOMMU_VDEVICE_TSM_REQ_SCOPE_PCI_LAST)
>> +	switch (tvm_arch) {
>> +	case IOMMU_VDEVICE_TSM_TVM_ARCH_CCA:
>> +	case IOMMU_VDEVICE_TSM_TVM_ARCH_SEV:
>> +	case IOMMU_VDEVICE_TSM_TVM_ARCH_TDX:
>
> Makes sense for any command that needs tunneling. However, see below, what is
> that set, and do we need a IOMMU_VDEVICE_TSM_COMMON when architecture
> differentiation is not required?
>
>> +		return true;
>> +	default:
>>  		return false;
>> +	}
>> +}
>>  
>> -	switch (scope) {
>> -	case IOMMU_VDEVICE_TSM_REQ_PCI_INFO:
>> -	case IOMMU_VDEVICE_TSM_REQ_PCI_STATE_CHANGE:
>> -	case IOMMU_VDEVICE_TSM_REQ_PCI_DEBUG_READ:
>> -	case IOMMU_VDEVICE_TSM_REQ_PCI_DEBUG_WRITE:
>> +static bool iommufd_vdevice_tsm_req_op_valid(u32 op, u32 tvm_arch)
>> +{
>> +	switch (op) {
>> +	case TSM_REQ_READ_OBJECT:
>> +	case TSM_REQ_REGEN_OBJECT:
>> +	case TSM_REQ_OBJECT_INFO:
>
> The design goal of the netlink device-evidence interface is to be able
> to respond to all shapes of requests for evidence. So netlink caches
> objects that the hypercall handler can fill responses from.
>
> It eliminates a class of commands that need tunneling.
>

Sure, I can drop this from the iommufd ioctl and use netlink to read and
regenerate the objects from the VMM.

Can I use netlink to find the cached object size? CCA supports
RHI_DA_OBJECT_SIZE, which can be used to query the object size.
If not should we have TSM_REQ_OBJECT_INFO? 

>
>> +	case TSM_REQ_VALIDATE_MMIO:
>> +	case TSM_REQ_SET_TDI_STATE:
>
> Are these potentially candidates for a IOMMU_VDEVICE_TSM_COMMON? The
> handler knows how to do the arch-specific response from the common
> iommufd result, or is there TSM-specific payload beyond @tsm_code for
> these.
>
> Make it the case that guest_req only needs non-common arch for
> operations that are implementation unique, or where the response payload
> exceeds what can be conveyed via @tsm_code.
>

I am not sure I follow the IOMMU_VDEVICE_TSM_COMMON feedback above.

Earlier discussions around this concluded that we may want iommufd
to validate all input commands, rather than making the guest request
ioctl a passthrough interface.

If we make the ops IOMMU_VDEVICE_TSM_COMMON, we would still need to add
TSM_REQ_VALIDATE_MMIO and TSM_REQ_SET_TDI_STATE for the arch-specific
handler. Why not expose those to the generic iommufd layer, so that we
can add operation validation there and completely drop IOMMU_VDEVICE_TSM_COMMON?

>>  		return true;
>> +	case TSM_REQ_SEV_ENABLE_DMA:
>> +	case TSM_REQ_SEV_DISABLE_DMA:
>> +		return tvm_arch == IOMMU_VDEVICE_TSM_TVM_ARCH_SEV;
>
> Right, this appears to be the only case where the command is
> implementation unique. The handler can only ask iommufd to take
> arch-specific action.

-aneesh