Re: [PATCH] AMD/IOMMU: drop remaining guest-IOMMU bits too

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.03.2024 11:46, Roger Pau Monné wrote:
> On Tue, Mar 19, 2024 at 02:28:12PM +0100, Jan Beulich wrote:
>> With a02174c6c885 ("amd/iommu: clean up unused guest iommu related
>> functions") having removed the sole place where d->g_iommu would be set
>> to non-NULL, guest_iommu_add_ppr_log() will unconditionally bail the
>> latest from its 2nd if(). With it dropped, all other stuff in the file
>> is unused, too. Delete iommu_guest.c altogether.
>>
>> Further delete struct guest{_buffer,_dev_table,_iommu{,_msi}} as well as
>> struct mmio_reg for being unused with the unused g_iommu also dropped
>> from struct arch_iommu.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> I wasn't sure how far to further go with removing the body of
>> parse_ppr_log_entry(), or perhaps even the entire function, and then
>> further up to all PPR logging code. Hence why for now I've merely
>> commented out the function call into the file being deleted (which of
>> course Misra isn't going to like). Thoughts / suggestions?
>>
>> I further wonder whether set_iommu_guest_translation_control() should
>> have been invoked independent of guest-IOMMU actually being enabled. IOW
>> that may want purging, too. Along these lines iommuv2_enabled may also
>> want dropping, for not having any consumer left. Much like has_viommu()
>> and then also {XEN_,}X86_EMU_IOMMU, i.e. going as far as affecting the
>> public interface.
>>
>> --- a/xen/arch/x86/include/asm/iommu.h
>> +++ b/xen/arch/x86/include/asm/iommu.h
>> @@ -52,7 +52,6 @@ struct arch_iommu
>>          struct {
>>              unsigned int paging_mode;
>>              struct page_info *root_table;
>> -            struct guest_iommu *g_iommu;
>>          } amd;
>>      };
>>  };
>> --- a/xen/drivers/passthrough/amd/Makefile
>> +++ b/xen/drivers/passthrough/amd/Makefile
>> @@ -5,4 +5,3 @@ obj-y += pci_amd_iommu.o
>>  obj-bin-y += iommu_acpi.init.o
>>  obj-y += iommu_intr.o
>>  obj-y += iommu_cmd.o
>> -obj-$(CONFIG_HVM) += iommu_guest.o
>> --- a/xen/drivers/passthrough/amd/iommu.h
>> +++ b/xen/drivers/passthrough/amd/iommu.h
>> @@ -145,57 +145,6 @@ int iterate_ivrs_entries(int (*handler)(
>>                                          struct ivrs_mappings *map,
>>                                          uint16_t bdf));
>>  
>> -/* iommu tables in guest space */
>> -struct mmio_reg {
>> -    uint32_t    lo;
>> -    uint32_t    hi;
>> -};
>> -
>> -struct guest_dev_table {
>> -    struct mmio_reg         reg_base;
>> -    uint32_t                size;
>> -};
>> -
>> -struct guest_buffer {
>> -    struct mmio_reg         reg_base;
>> -    struct mmio_reg         reg_tail;
>> -    struct mmio_reg         reg_head;
>> -    uint32_t                size;
>> -};
>> -
>> -struct guest_iommu_msi {
>> -    uint8_t                 vector;
>> -    uint8_t                 dest;
>> -    uint8_t                 dest_mode;
>> -    uint8_t                 delivery_mode;
>> -    uint8_t                 trig_mode;
>> -};
>> -
>> -/* virtual IOMMU structure */
>> -struct guest_iommu {
>> -
>> -    struct domain          *domain;
>> -    spinlock_t              lock;
>> -    bool                    enabled;
>> -
>> -    struct guest_dev_table  dev_table;
>> -    struct guest_buffer     cmd_buffer;
>> -    struct guest_buffer     event_log;
>> -    struct guest_buffer     ppr_log;
>> -
>> -    struct tasklet          cmd_buffer_tasklet;
>> -
>> -    uint64_t                mmio_base;             /* MMIO base address */
>> -
>> -    /* MMIO regs */
>> -    union amd_iommu_control reg_ctrl;              /* MMIO offset 0018h */
>> -    struct mmio_reg         reg_status;            /* MMIO offset 2020h */
>> -    union amd_iommu_ext_features reg_ext_feature;  /* MMIO offset 0030h */
>> -
>> -    /* guest interrupt settings */
>> -    struct guest_iommu_msi  msi;
>> -};
>> -
>>  extern bool iommuv2_enabled;
> 
> I think iommuv2_enabled can also go away, since it's only used by
> guest_iommu_init()?
> 
> It's set in amd_iommu_prepare() and amd_iommu_init_cleanup() but only
> consumed by guest_iommu_init().

See the post-commit-message remarks: The main question is where to stop.

Jan