| Message ID | 5eb98372-2992-4a93-846c-4e325c41af98@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | AMD/IOMMU: drop remaining guest-IOMMU bits too | expand |
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.
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@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
On Wed, Mar 20, 2024 at 11:58:50AM +0100, Jan Beulich wrote: > 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@suse.com> Acked-by: Roger Pau Monné <roger.pau@citrix.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. I would drop it all. The public interface part is not stable anyway, as it's a domctl, but I would be fine if you want to keep the X86_EMU_IOMMU. Thanks, Roger.
On 20.03.2024 15:06, Roger Pau Monné wrote: > On Wed, Mar 20, 2024 at 11:58:50AM +0100, Jan Beulich wrote: >> 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@suse.com> > > Acked-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. >>>> --- >>>> 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. > > I would drop it all. The public interface part is not stable anyway, > as it's a domctl, but I would be fine if you want to keep the X86_EMU_IOMMU. By "all" you also mean the PPR logging code? That's where I felt I might be going too far ... Jan
On Wed, Mar 20, 2024 at 03:51:55PM +0100, Jan Beulich wrote: > On 20.03.2024 15:06, Roger Pau Monné wrote: > > On Wed, Mar 20, 2024 at 11:58:50AM +0100, Jan Beulich wrote: > >> 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@suse.com> > > > > Acked-by: Roger Pau Monné <roger.pau@citrix.com> > > Thanks. > > >>>> --- > >>>> 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. > > > > I would drop it all. The public interface part is not stable anyway, > > as it's a domctl, but I would be fine if you want to keep the X86_EMU_IOMMU. > > By "all" you also mean the PPR logging code? That's where I felt I might > be going too far ... It's up to you. I've taken a look, and the PPR logging code seems to be tied to guest IOMMU also? Since PPR could be used for other purposes it's likely fine to leave it in, albeit I won't oppose if you want to remove it (maybe there's some purpose I'm missing). Thanks, Roger.
--- 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; struct acpi_ivrs_hardware; @@ -344,13 +293,6 @@ void cf_check amd_iommu_resume(void); int __must_check cf_check amd_iommu_suspend(void); void cf_check amd_iommu_crash_shutdown(void); -/* guest iommu support */ -#ifdef CONFIG_HVM -void guest_iommu_add_ppr_log(struct domain *d, uint32_t entry[]); -#else -static inline void guest_iommu_add_ppr_log(struct domain *d, uint32_t entry[]) {} -#endif - static inline u32 get_field_from_reg_u32(u32 reg_value, u32 mask, u32 shift) { u32 field; --- a/xen/drivers/passthrough/amd/iommu_guest.c +++ /dev/null @@ -1,120 +0,0 @@ -/* - * Copyright (C) 2011 Advanced Micro Devices, Inc. - * Author: Wei Wang <wei.wang2@amd.com> - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; either version 2 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program; If not, see <http://www.gnu.org/licenses/>. - */ - -#include <asm/p2m.h> - -#include "iommu.h" - -#define reg_to_u64(reg) (((uint64_t)reg.hi << 32) | reg.lo ) - -static uint16_t get_guest_bdf(struct domain *d, uint16_t machine_bdf) -{ - return machine_bdf; -} - -static inline struct guest_iommu *domain_iommu(struct domain *d) -{ - return dom_iommu(d)->arch.amd.g_iommu; -} - -static unsigned long get_gfn_from_base_reg(uint64_t base_raw) -{ - base_raw &= PADDR_MASK; - ASSERT ( base_raw != 0 ); - return base_raw >> PAGE_SHIFT; -} - -static void guest_iommu_deliver_msi(struct domain *d) -{ - uint8_t vector, dest, dest_mode, delivery_mode, trig_mode; - struct guest_iommu *iommu = domain_iommu(d); - - vector = iommu->msi.vector; - dest = iommu->msi.dest; - dest_mode = iommu->msi.dest_mode; - delivery_mode = iommu->msi.delivery_mode; - trig_mode = iommu->msi.trig_mode; - - vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode); -} - -static unsigned long guest_iommu_get_table_mfn(struct domain *d, - uint64_t base_raw, - unsigned int pos) -{ - unsigned long idx, gfn, mfn; - p2m_type_t p2mt; - - gfn = get_gfn_from_base_reg(base_raw); - idx = pos >> PAGE_SHIFT; - - mfn = mfn_x(get_gfn(d, gfn + idx, &p2mt)); - put_gfn(d, gfn); - - return mfn; -} - -void guest_iommu_add_ppr_log(struct domain *d, u32 entry[]) -{ - uint16_t gdev_id; - unsigned long mfn, tail, head; - ppr_entry_t *log; - struct guest_iommu *iommu; - - if ( !is_hvm_domain(d) ) - return; - - iommu = domain_iommu(d); - if ( !iommu ) - return; - - tail = iommu->ppr_log.reg_tail.lo; - head = iommu->ppr_log.reg_head.lo; - - if ( tail >= iommu->ppr_log.size || head >= iommu->ppr_log.size ) - { - AMD_IOMMU_DEBUG("Error: guest iommu ppr log overflows\n"); - iommu->enabled = 0; - return; - } - - mfn = guest_iommu_get_table_mfn(d, reg_to_u64(iommu->ppr_log.reg_base), - tail); - ASSERT(mfn_valid(_mfn(mfn))); - - log = map_domain_page(_mfn(mfn)) + (tail & ~PAGE_MASK); - - /* Convert physical device id back into virtual device id */ - gdev_id = get_guest_bdf(d, iommu_get_devid_from_cmd(entry[0])); - iommu_set_devid_to_cmd(&entry[0], gdev_id); - - memcpy(log, entry, sizeof(ppr_entry_t)); - - /* Now shift ppr log tail pointer */ - tail += sizeof(ppr_entry_t); - if ( tail >= iommu->ppr_log.size ) - { - tail = 0; - iommu->reg_status.lo |= IOMMU_STATUS_PPR_LOG_OVERFLOW; - } - - iommu->ppr_log.reg_tail.lo = tail; - unmap_domain_page(log); - - guest_iommu_deliver_msi(d); -} --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -646,7 +646,7 @@ static void cf_check parse_ppr_log_entry pcidevs_unlock(); if ( pdev ) - guest_iommu_add_ppr_log(pdev->domain, entry); + /* guest_iommu_add_ppr_log(pdev->domain, entry) */; } static void iommu_check_ppr_log(struct amd_iommu *iommu)
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.