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

["Roger Pau Monné" <roger.pau@citrix.com>]

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@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().

Thanks, Roger.