Re: [PATCH] hw/i386/amd_iommu: Allow migration

[Joao Martins <joao.m.martins@oracle.com>]

On 05/02/2025 02:45, Suthikulpanit, Suravee wrote:
> 
> 
> On 11/29/2024 12:14 AM, Joao Martins wrote:
>> On 21/11/2024 11:42, Joao Martins wrote:> On 20/11/2024 07:31, Suravee
>> Suthikulpanit wrote:
>>>> Add migration support for AMD IOMMU model by saving necessary AMDVIState
>>>> parameters for MMIO registers, device table, command buffer, and event
>>>> buffers.
>>>>
>>>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>>> ---
>>>>   hw/i386/amd_iommu.c | 36 +++++++++++++++++++++++++++++++++++-
>>>>   1 file changed, 35 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>>>> index 13af7211e1..3d2bb9d81e 100644
>>>> --- a/hw/i386/amd_iommu.c
>>>> +++ b/hw/i386/amd_iommu.c
>>>> @@ -1673,7 +1673,41 @@ static Property amdvi_properties[] = {
>>>>
>>>>   static const VMStateDescription vmstate_amdvi_sysbus = {
>>>>       .name = "amd-iommu",
>>>> -    .unmigratable = 1
>>>> +    .version_id = 1,
>>>> +    .minimum_version_id = 1,
>>>> +    .priority = MIG_PRI_IOMMU,
>>>> +    .fields = (VMStateField[]) {
>>>> +      /* Updated in  amdvi_handle_control_write() */
>>>> +      VMSTATE_BOOL(enabled, AMDVIState),
>>>
>>> no xtsup ?  I guess you are relying on the dest command line having xtsup=on
>>> like intel-iommu
>>>
>>
>> Having said, I think I found a flaw here that sort of "ignores" the default
>> command line param of 'device-iotlb' (broad x86-iommu param). By default it
>> seems we enable device-iotlb in amd-iommu regardless, even though it's disabled
>> by default in qemu command line params.
>>
>> Should we enable migration I think stuff like that starts to be important to
>> honor given the compability issues we would have to deal apriori. See below on
>> how to fix, happy to formally send if what I explained makes sense to all
> 
> As you mentioned, AMD vIOMMU currently ignore this parameter and QEMU default
> dt_supported to false, and set the IOTLBSup in the IVRS table to 1. If we start
> interpret this option, we need to also emulate the following cases when
> dt_supported=0:
> 
> * When DTE[I]=1. From AMD IOMMU Spec:
> 
> I: IOTLB enable. Controls IOMMU response to address translation requests from
> peripherals. 0=IOMMU returns target abort status when it receives an ATS
> requests from the peripheral. 1=IOMMU responds to ATS requests from the
> peripheral. This bit does not affect interrupts from the peripheral. If I=1 when
> Capability Offset 00h[IotlbSup]=0, the results are undefined.
> 
> * Handle the INVALIDATE_IOTLB_PAGES command, which also needs dt_supported=1.
> 
> Apart from trying to be compatible w/ the x86-iommu dt_supported param (which is
> mostly for Intel vIOMMU), I am not sure if handling the dt_supported is
> necessary for AMD vIOMMU.

I agree; I certainly not suggesting going out of our way to support
device-iotlb=1 with my command. We could throw an error and fail the amd-iommu
device realize if we wanna explicit that amd vIOMMU doesn't support device_iotlb=1.

> Do we have a scenario where we need to disable ATS
> support for vIOMMU?
> It's closer to reality that ATS isn't quite fully exerciable in Qemu (until this
series lands IIUC:
https://lore.kernel.org/qemu-devel/20250120174033.308518-20-clement.mathieu--drif@eviden.com/).


My whole point was to honor the default, which is disabled and not mis-advertise
it as being supported. Specially considering the cases abov you point out look
to be be obvious gaps that aren't implemented, all the more reason to not be
misadvertising it.