Re: [PATCH v3 3/3] amd_iommu: Generate XT interrupts when xt support is enabled

[Sairaj Kodilkar <sarunkod@amd.com>]

>>>>>> When MMIO 0x18[IntCapXTEn]=1, interrupts originating from the IOMMU
>>>>>> itself are
>>>>>> sent based on the programming in XT IOMMU Interrupt Control Registers in
>>>>>> MMIO
>>>>>> 0x170-0x180 instead of the programming in the IOMMU's MSI capability
>>>>>> registers.
>>>>>> The guest programs these registers with appropriate vector and
>>>>>> destination
>>>>>> ID instead of writing to PCI MSI capability.
>>>>>>
>>>>>> Current AMD vIOMMU is capable of generating interrupts only through PCI
>>>>>> MSI capability and does not care about xt mode. Because of this AMD
>>>>>> vIOMMU cannot generate event log interrupts when the guest has enabled
>>>>>> xt mode.
>>>>>>
>>>>>> Introduce a new flag "intcapxten" which is set when guest writes control
>>>>>> register [IntCapXTEn] (bit 51) and use vector and destination field in
>>>>>> the XT MMIO register (0x170) to support XT mode.
>>>>>>
>>>>>> Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
>>>>>> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>
>>>>>> Reviewed-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
>>>>>> ---
>>>>>>     hw/i386/amd_iommu.c  | 47 ++++++++++++++++++++++++++++++++++++++------
>>>>>>     hw/i386/amd_iommu.h  | 17 ++++++++++++++++
>>>>>>     hw/i386/trace-events |  1 +
>>>>>>     3 files changed, 59 insertions(+), 6 deletions(-)
>>>>>>
>>>>> .../...
>>>>>
>>>>>>           /* update the flags depending on the control register */
>>>>>>         if (s->cmdbuf_enabled) {
>>>>>> @@ -1732,6 +1762,9 @@ static void amdvi_mmio_write(void *opaque, hwaddr
>>>>>> addr, uint64_t val,
>>>>>>         case AMDVI_MMIO_STATUS:
>>>>>>             amdvi_mmio_reg_write(s, size, val, addr);
>>>>>>             break;
>>>>>> +    case AMDVI_MMIO_XT_GEN_INTR:
>>>>>> +        amdvi_mmio_reg_write(s, size, val, addr);
>>>>>> +        break;
>>>>>>         }
>>>>>>     }
>>>>>>     @@ -2382,6 +2415,7 @@ static void amdvi_init(AMDVIState *s)
>>>>>>         s->enabled = false;
>>>>>>         s->cmdbuf_enabled = false;
>>>>>>         s->xten = false;
>>>>>> +    s->intcapxten = false;
>>>>>>           /* reset MMIO */
>>>>>>         memset(s->mmior, 0, AMDVI_MMIO_SIZE);
>>>>>> @@ -2452,6 +2486,7 @@ static const VMStateDescription vmstate_xt = {
>>>>>>            .minimum_version_id = 1,
>>>>>>            .fields = (VMStateField[]) {
>>>>>>                VMSTATE_BOOL(xten, AMDVIState),
>>>>>> +           VMSTATE_BOOL(intcapxten, AMDVIState),
>>>>> Do we need to increase the version no?
>>>> No, because we have introduced a separate subsection, the older and newer
>>>> qemu are compatible.
>>>>
>>> I thought that was the case because the changes will still be part of the
>>> same "release". I don't believe we guarantee migration compatibility
>>> between random/intermediate commits, but...
>>>
>>> If we are going to follow the guidelines strictly then I think Vasant's
>>> observation is correct. The patch changes the layout of the subsection so
>>> we are in the same scenario that lead us to include a subsection to begin
>>> with.
>>>
>>> Because the two new values are still part of the same xt support domain, I
>>> think it makes sense to keep them in the same subsection and the simplest
>>> way is to do:
>>>
>>>    static const VMStateDescription vmstate_xt = {
>>>        .name = "amd-iommu-xt",
>>> -    .version_id = 1,
>>> +    .version_id = 2,
>>>        .minimum_version_id = 1,
>>>        .fields = (VMStateField[]) {
>>>            VMSTATE_BOOL(xten, AMDVIState),
>>> -        VMSTATE_BOOL(intcapxten, AMDVIState),
>>> +        VMSTATE_BOOL_V(intcapxten, AMDVIState, 2),
>>>            VMSTATE_END_OF_LIST()
>>>        }
>>>    };
>>>
>>> That change is on top of your current patch. There seems to be precedent
>>> for this based on my search in the git log. If you are ok with this
>>> approach let me know and I'll apply it, no need to send a new revision.
>> I am not sure if it is really useful here. Because without this patch
>> xt-support will not work and migrating from vm which only has first two
>> patches to the vm which has all three patches does not make sense to me.
> I agree it doesn't make much sense from a practical standpoint, unless we
> are bisecting a migration issue, and we don't want to fail between these
> two commits.
> But again, adding a new field to the subsection does change the payload
> that is sent "on the wire", and increasing the version like in my example
> above is the minimum change that keeps it all fully correct for migrations
> going forward (just like we ensured when adding the subsection in the first
> place).
>
>> I think we should treat three patches as a single unit, let me know what
>> do you think about this ?
>>
> I considered just moving the creation of the entire subsection to the last
> patch, but I think it is unnecessary. The approach of incrementing the
> version ID above doesn't have any downsides other than little additional
> complexity, and it is the "technically correct" way to do it.
>
> I don't want to introduce new subsections, since that has basically the
> same effect as the method I proposed, and it makes more sense to keep all
> of the XT-related fields in the same subsection (appropriately named
> "amd-iommu-xt")
>
> So unless you have any correctness concerns, we should go with my initial
> suggestion (actually it was Vasant's). I get the usefulness is limited, but
> it is the proper way to do this based on my interpretation of the docs.
Hi Alejandro,

I don't have any correctness concerns. I was trying to understand pros and
cons of this method. I am ok with this change.

Thanks
Sairaj