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 <jbeul...@suse.com>
>> ---
>> 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
