| Message ID | 5C9CDFCC020000780022287E@prv1-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: eliminate Intel-isms from x2APIC setup | expand |
On 28/03/2019 14:53, Jan Beulich wrote: > @@ -35,6 +35,8 @@ void print_vtd_entries(struct iommu *iom > keyhandler_fn_t vtd_dump_iommu_info; > > bool intel_iommu_supports_eim(void); > +int intel_iommu_enable_x2apic_IR(void); > +void intel_iommu_disable_x2apic_IR(void); Is there any particular reason why these retain their _IR suffix? I'd suggest going with intel_iommu_{en,dis}able_eim() to match the supports name here, whereas... > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -2720,6 +2720,8 @@ const struct iommu_ops __initconstrel in > .free_page_table = iommu_free_page_table, > .reassign_device = reassign_device_ownership, > .get_device_group_id = intel_iommu_group_id, > + .enable_x2apic_IR = intel_iommu_enable_x2apic_IR, > + .disable_x2apic_IR = intel_iommu_disable_x2apic_IR, > .update_ire_from_apic = io_apic_write_remap_rte, > .update_ire_from_msi = msi_msg_write_remap_rte, > .read_apic_from_ire = io_apic_read_remap_rte, > @@ -2736,6 +2738,7 @@ const struct iommu_ops __initconstrel in > }; > > const struct iommu_init_ops __initconstrel intel_iommu_init_ops = { > + .ops = &intel_iommu_ops, > .setup = vtd_setup, > .supports_x2apic = intel_iommu_supports_eim, > }; > --- a/xen/drivers/passthrough/x86/iommu.c > +++ b/xen/drivers/passthrough/x86/iommu.c > @@ -26,6 +26,24 @@ > const struct iommu_init_ops *__initdata iommu_init_ops; > struct iommu_ops __read_mostly iommu_ops; > > +int iommu_enable_x2apic_IR(void) ... using iommu_{en,dis}able_x2apic() here to match the supports_x2apic() init hook. I don't think these shorter names are any more ambiguous, and loosing the _IR suffix does make them more consistent with the rest of Xen's function naming conventions. ~Andrew
>>> On 28.03.19 at 18:37, <andrew.cooper3@citrix.com> wrote: > On 28/03/2019 14:53, Jan Beulich wrote: >> @@ -35,6 +35,8 @@ void print_vtd_entries(struct iommu *iom >> keyhandler_fn_t vtd_dump_iommu_info; >> >> bool intel_iommu_supports_eim(void); >> +int intel_iommu_enable_x2apic_IR(void); >> +void intel_iommu_disable_x2apic_IR(void); > > Is there any particular reason why these retain their _IR suffix? Well, I've too been thinking about the naming here. I decided to keep the _IR suffixes because that's what the functions really do: They enable/disable interrupt remapping for x2APIC mode. They don't enable x2APIC itself in any way, and iirc x2APIC mode could be used (without wider APIC IDs and in physical mode) even without any IR enabled. > I'd suggest going with intel_iommu_{en,dis}able_eim() to match the > supports name here, whereas... > >> --- a/xen/drivers/passthrough/vtd/iommu.c >> +++ b/xen/drivers/passthrough/vtd/iommu.c >> @@ -2720,6 +2720,8 @@ const struct iommu_ops __initconstrel in >> .free_page_table = iommu_free_page_table, >> .reassign_device = reassign_device_ownership, >> .get_device_group_id = intel_iommu_group_id, >> + .enable_x2apic_IR = intel_iommu_enable_x2apic_IR, >> + .disable_x2apic_IR = intel_iommu_disable_x2apic_IR, >> .update_ire_from_apic = io_apic_write_remap_rte, >> .update_ire_from_msi = msi_msg_write_remap_rte, >> .read_apic_from_ire = io_apic_read_remap_rte, >> @@ -2736,6 +2738,7 @@ const struct iommu_ops __initconstrel in >> }; >> >> const struct iommu_init_ops __initconstrel intel_iommu_init_ops = { >> + .ops = &intel_iommu_ops, >> .setup = vtd_setup, >> .supports_x2apic = intel_iommu_supports_eim, >> }; >> --- a/xen/drivers/passthrough/x86/iommu.c >> +++ b/xen/drivers/passthrough/x86/iommu.c >> @@ -26,6 +26,24 @@ >> const struct iommu_init_ops *__initdata iommu_init_ops; >> struct iommu_ops __read_mostly iommu_ops; >> >> +int iommu_enable_x2apic_IR(void) > > ... using iommu_{en,dis}able_x2apic() here to match the > supports_x2apic() init hook. > > > I don't think these shorter names are any more ambiguous, and loosing > the _IR suffix does make them more consistent with the rest of Xen's > function naming conventions. The above said, in the end I'm not overly fussed, but before deciding which route to go I'll wait to see whether in particular Kevin has an opinion either way. Jan
> From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Friday, March 29, 2019 5:13 PM > > >>> On 28.03.19 at 18:37, <andrew.cooper3@citrix.com> wrote: > > On 28/03/2019 14:53, Jan Beulich wrote: > >> @@ -35,6 +35,8 @@ void print_vtd_entries(struct iommu *iom > >> keyhandler_fn_t vtd_dump_iommu_info; > >> > >> bool intel_iommu_supports_eim(void); > >> +int intel_iommu_enable_x2apic_IR(void); > >> +void intel_iommu_disable_x2apic_IR(void); > > > > Is there any particular reason why these retain their _IR suffix? > > Well, I've too been thinking about the naming here. I decided to > keep the _IR suffixes because that's what the functions really > do: They enable/disable interrupt remapping for x2APIC mode. > They don't enable x2APIC itself in any way, and iirc x2APIC > mode could be used (without wider APIC IDs and in physical > mode) even without any IR enabled. > > > I'd suggest going with intel_iommu_{en,dis}able_eim() to match the > > supports name here, whereas... > > > >> --- a/xen/drivers/passthrough/vtd/iommu.c > >> +++ b/xen/drivers/passthrough/vtd/iommu.c > >> @@ -2720,6 +2720,8 @@ const struct iommu_ops __initconstrel in > >> .free_page_table = iommu_free_page_table, > >> .reassign_device = reassign_device_ownership, > >> .get_device_group_id = intel_iommu_group_id, > >> + .enable_x2apic_IR = intel_iommu_enable_x2apic_IR, > >> + .disable_x2apic_IR = intel_iommu_disable_x2apic_IR, > >> .update_ire_from_apic = io_apic_write_remap_rte, > >> .update_ire_from_msi = msi_msg_write_remap_rte, > >> .read_apic_from_ire = io_apic_read_remap_rte, > >> @@ -2736,6 +2738,7 @@ const struct iommu_ops __initconstrel in > >> }; > >> > >> const struct iommu_init_ops __initconstrel intel_iommu_init_ops = { > >> + .ops = &intel_iommu_ops, > >> .setup = vtd_setup, > >> .supports_x2apic = intel_iommu_supports_eim, > >> }; > >> --- a/xen/drivers/passthrough/x86/iommu.c > >> +++ b/xen/drivers/passthrough/x86/iommu.c > >> @@ -26,6 +26,24 @@ > >> const struct iommu_init_ops *__initdata iommu_init_ops; > >> struct iommu_ops __read_mostly iommu_ops; > >> > >> +int iommu_enable_x2apic_IR(void) > > > > ... using iommu_{en,dis}able_x2apic() here to match the > > supports_x2apic() init hook. > > > > > > I don't think these shorter names are any more ambiguous, and loosing > > the _IR suffix does make them more consistent with the rest of Xen's > > function naming conventions. > > The above said, in the end I'm not overly fussed, but before deciding > which route to go I'll wait to see whether in particular Kevin has an > opinion either way. > let's remove _IR. we have intel_iommu prefix which is sufficient to indicate enable_x2apic in iommu context is about IR. since renaming is small thing, here is my: Reviewed-by: Kevin Tian <kevin.tian@intel.com>
On 02/04/2019 04:24, Tian, Kevin wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Friday, March 29, 2019 5:13 PM >> >>>>> On 28.03.19 at 18:37, <andrew.cooper3@citrix.com> wrote: >>> On 28/03/2019 14:53, Jan Beulich wrote: >>>> @@ -35,6 +35,8 @@ void print_vtd_entries(struct iommu *iom >>>> keyhandler_fn_t vtd_dump_iommu_info; >>>> >>>> bool intel_iommu_supports_eim(void); >>>> +int intel_iommu_enable_x2apic_IR(void); >>>> +void intel_iommu_disable_x2apic_IR(void); >>> Is there any particular reason why these retain their _IR suffix? >> Well, I've too been thinking about the naming here. I decided to >> keep the _IR suffixes because that's what the functions really >> do: They enable/disable interrupt remapping for x2APIC mode. >> They don't enable x2APIC itself in any way, and iirc x2APIC >> mode could be used (without wider APIC IDs and in physical >> mode) even without any IR enabled. >> >>> I'd suggest going with intel_iommu_{en,dis}able_eim() to match the >>> supports name here, whereas... >>> >>>> --- a/xen/drivers/passthrough/vtd/iommu.c >>>> +++ b/xen/drivers/passthrough/vtd/iommu.c >>>> @@ -2720,6 +2720,8 @@ const struct iommu_ops __initconstrel in >>>> .free_page_table = iommu_free_page_table, >>>> .reassign_device = reassign_device_ownership, >>>> .get_device_group_id = intel_iommu_group_id, >>>> + .enable_x2apic_IR = intel_iommu_enable_x2apic_IR, >>>> + .disable_x2apic_IR = intel_iommu_disable_x2apic_IR, >>>> .update_ire_from_apic = io_apic_write_remap_rte, >>>> .update_ire_from_msi = msi_msg_write_remap_rte, >>>> .read_apic_from_ire = io_apic_read_remap_rte, >>>> @@ -2736,6 +2738,7 @@ const struct iommu_ops __initconstrel in >>>> }; >>>> >>>> const struct iommu_init_ops __initconstrel intel_iommu_init_ops = { >>>> + .ops = &intel_iommu_ops, >>>> .setup = vtd_setup, >>>> .supports_x2apic = intel_iommu_supports_eim, >>>> }; >>>> --- a/xen/drivers/passthrough/x86/iommu.c >>>> +++ b/xen/drivers/passthrough/x86/iommu.c >>>> @@ -26,6 +26,24 @@ >>>> const struct iommu_init_ops *__initdata iommu_init_ops; >>>> struct iommu_ops __read_mostly iommu_ops; >>>> >>>> +int iommu_enable_x2apic_IR(void) >>> ... using iommu_{en,dis}able_x2apic() here to match the >>> supports_x2apic() init hook. >>> >>> >>> I don't think these shorter names are any more ambiguous, and loosing >>> the _IR suffix does make them more consistent with the rest of Xen's >>> function naming conventions. >> The above said, in the end I'm not overly fussed, but before deciding >> which route to go I'll wait to see whether in particular Kevin has an >> opinion either way. >> > let's remove _IR. we have intel_iommu prefix which is sufficient > to indicate enable_x2apic in iommu context is about IR. > > since renaming is small thing, here is my: > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> No point posting this series a second time just for the rename. With the suggested adjustments, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> On 02.04.19 at 05:24, <kevin.tian@intel.com> wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Friday, March 29, 2019 5:13 PM >> >> >>> On 28.03.19 at 18:37, <andrew.cooper3@citrix.com> wrote: >> > On 28/03/2019 14:53, Jan Beulich wrote: >> >> @@ -35,6 +35,8 @@ void print_vtd_entries(struct iommu *iom >> >> keyhandler_fn_t vtd_dump_iommu_info; >> >> >> >> bool intel_iommu_supports_eim(void); >> >> +int intel_iommu_enable_x2apic_IR(void); >> >> +void intel_iommu_disable_x2apic_IR(void); >> > >> > Is there any particular reason why these retain their _IR suffix? >> >> Well, I've too been thinking about the naming here. I decided to >> keep the _IR suffixes because that's what the functions really >> do: They enable/disable interrupt remapping for x2APIC mode. >> They don't enable x2APIC itself in any way, and iirc x2APIC >> mode could be used (without wider APIC IDs and in physical >> mode) even without any IR enabled. >> >> > I'd suggest going with intel_iommu_{en,dis}able_eim() to match the >> > supports name here, whereas... >> > >> >> --- a/xen/drivers/passthrough/vtd/iommu.c >> >> +++ b/xen/drivers/passthrough/vtd/iommu.c >> >> @@ -2720,6 +2720,8 @@ const struct iommu_ops __initconstrel in >> >> .free_page_table = iommu_free_page_table, >> >> .reassign_device = reassign_device_ownership, >> >> .get_device_group_id = intel_iommu_group_id, >> >> + .enable_x2apic_IR = intel_iommu_enable_x2apic_IR, >> >> + .disable_x2apic_IR = intel_iommu_disable_x2apic_IR, >> >> .update_ire_from_apic = io_apic_write_remap_rte, >> >> .update_ire_from_msi = msi_msg_write_remap_rte, >> >> .read_apic_from_ire = io_apic_read_remap_rte, >> >> @@ -2736,6 +2738,7 @@ const struct iommu_ops __initconstrel in >> >> }; >> >> >> >> const struct iommu_init_ops __initconstrel intel_iommu_init_ops = { >> >> + .ops = &intel_iommu_ops, >> >> .setup = vtd_setup, >> >> .supports_x2apic = intel_iommu_supports_eim, >> >> }; >> >> --- a/xen/drivers/passthrough/x86/iommu.c >> >> +++ b/xen/drivers/passthrough/x86/iommu.c >> >> @@ -26,6 +26,24 @@ >> >> const struct iommu_init_ops *__initdata iommu_init_ops; >> >> struct iommu_ops __read_mostly iommu_ops; >> >> >> >> +int iommu_enable_x2apic_IR(void) >> > >> > ... using iommu_{en,dis}able_x2apic() here to match the >> > supports_x2apic() init hook. >> > >> > >> > I don't think these shorter names are any more ambiguous, and loosing >> > the _IR suffix does make them more consistent with the rest of Xen's >> > function naming conventions. >> >> The above said, in the end I'm not overly fussed, but before deciding >> which route to go I'll wait to see whether in particular Kevin has an >> opinion either way. >> > > let's remove _IR. we have intel_iommu prefix which is sufficient > to indicate enable_x2apic in iommu context is about IR. > > since renaming is small thing, here is my: > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> Thanks, but well, I'll then follow Andrew's suggestion and also name the VT-d functions intel_iommu_{en,dis}able_eim(). I hope that's okay with you. Jan
> From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Tuesday, April 2, 2019 9:17 PM > > >>> On 02.04.19 at 05:24, <kevin.tian@intel.com> wrote: > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: Friday, March 29, 2019 5:13 PM > >> > >> >>> On 28.03.19 at 18:37, <andrew.cooper3@citrix.com> wrote: > >> > On 28/03/2019 14:53, Jan Beulich wrote: > >> >> @@ -35,6 +35,8 @@ void print_vtd_entries(struct iommu *iom > >> >> keyhandler_fn_t vtd_dump_iommu_info; > >> >> > >> >> bool intel_iommu_supports_eim(void); > >> >> +int intel_iommu_enable_x2apic_IR(void); > >> >> +void intel_iommu_disable_x2apic_IR(void); > >> > > >> > Is there any particular reason why these retain their _IR suffix? > >> > >> Well, I've too been thinking about the naming here. I decided to > >> keep the _IR suffixes because that's what the functions really > >> do: They enable/disable interrupt remapping for x2APIC mode. > >> They don't enable x2APIC itself in any way, and iirc x2APIC > >> mode could be used (without wider APIC IDs and in physical > >> mode) even without any IR enabled. > >> > >> > I'd suggest going with intel_iommu_{en,dis}able_eim() to match the > >> > supports name here, whereas... > >> > > >> >> --- a/xen/drivers/passthrough/vtd/iommu.c > >> >> +++ b/xen/drivers/passthrough/vtd/iommu.c > >> >> @@ -2720,6 +2720,8 @@ const struct iommu_ops __initconstrel in > >> >> .free_page_table = iommu_free_page_table, > >> >> .reassign_device = reassign_device_ownership, > >> >> .get_device_group_id = intel_iommu_group_id, > >> >> + .enable_x2apic_IR = intel_iommu_enable_x2apic_IR, > >> >> + .disable_x2apic_IR = intel_iommu_disable_x2apic_IR, > >> >> .update_ire_from_apic = io_apic_write_remap_rte, > >> >> .update_ire_from_msi = msi_msg_write_remap_rte, > >> >> .read_apic_from_ire = io_apic_read_remap_rte, > >> >> @@ -2736,6 +2738,7 @@ const struct iommu_ops __initconstrel in > >> >> }; > >> >> > >> >> const struct iommu_init_ops __initconstrel intel_iommu_init_ops = { > >> >> + .ops = &intel_iommu_ops, > >> >> .setup = vtd_setup, > >> >> .supports_x2apic = intel_iommu_supports_eim, > >> >> }; > >> >> --- a/xen/drivers/passthrough/x86/iommu.c > >> >> +++ b/xen/drivers/passthrough/x86/iommu.c > >> >> @@ -26,6 +26,24 @@ > >> >> const struct iommu_init_ops *__initdata iommu_init_ops; > >> >> struct iommu_ops __read_mostly iommu_ops; > >> >> > >> >> +int iommu_enable_x2apic_IR(void) > >> > > >> > ... using iommu_{en,dis}able_x2apic() here to match the > >> > supports_x2apic() init hook. > >> > > >> > > >> > I don't think these shorter names are any more ambiguous, and loosing > >> > the _IR suffix does make them more consistent with the rest of Xen's > >> > function naming conventions. > >> > >> The above said, in the end I'm not overly fussed, but before deciding > >> which route to go I'll wait to see whether in particular Kevin has an > >> opinion either way. > >> > > > > let's remove _IR. we have intel_iommu prefix which is sufficient > > to indicate enable_x2apic in iommu context is about IR. > > > > since renaming is small thing, here is my: > > > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > > Thanks, but well, I'll then follow Andrew's suggestion and also > name the VT-d functions intel_iommu_{en,dis}able_eim(). I > hope that's okay with you. > OK to me.
--- a/xen/drivers/passthrough/vtd/extern.h +++ b/xen/drivers/passthrough/vtd/extern.h @@ -35,6 +35,8 @@ void print_vtd_entries(struct iommu *iom keyhandler_fn_t vtd_dump_iommu_info; bool intel_iommu_supports_eim(void); +int intel_iommu_enable_x2apic_IR(void); +void intel_iommu_disable_x2apic_IR(void); int enable_qinval(struct iommu *iommu); void disable_qinval(struct iommu *iommu); --- a/xen/drivers/passthrough/vtd/intremap.c +++ b/xen/drivers/passthrough/vtd/intremap.c @@ -882,23 +882,13 @@ out: * This function is used to enable Interrupt remapping when * enable x2apic */ -int iommu_enable_x2apic_IR(void) +int intel_iommu_enable_x2apic_IR(void) { struct acpi_drhd_unit *drhd; struct iommu *iommu; - if ( system_state < SYS_STATE_active ) - { - if ( !intel_iommu_supports_eim() ) - return -EOPNOTSUPP; - - if ( !platform_supports_x2apic() ) - return -ENXIO; - - iommu_ops = intel_iommu_ops; - } - else if ( !x2apic_enabled ) - return -EOPNOTSUPP; + if ( system_state < SYS_STATE_active && !platform_supports_x2apic() ) + return -ENXIO; for_each_drhd_unit ( drhd ) { @@ -946,14 +936,10 @@ int iommu_enable_x2apic_IR(void) * This function is used to disable Interrutp remapping when * suspend local apic */ -void iommu_disable_x2apic_IR(void) +void intel_iommu_disable_x2apic_IR(void) { struct acpi_drhd_unit *drhd; - /* x2apic_enabled implies iommu_supports_eim(). */ - if ( !x2apic_enabled ) - return; - for_each_drhd_unit ( drhd ) disable_intremap(drhd->iommu); --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -2720,6 +2720,8 @@ const struct iommu_ops __initconstrel in .free_page_table = iommu_free_page_table, .reassign_device = reassign_device_ownership, .get_device_group_id = intel_iommu_group_id, + .enable_x2apic_IR = intel_iommu_enable_x2apic_IR, + .disable_x2apic_IR = intel_iommu_disable_x2apic_IR, .update_ire_from_apic = io_apic_write_remap_rte, .update_ire_from_msi = msi_msg_write_remap_rte, .read_apic_from_ire = io_apic_read_remap_rte, @@ -2736,6 +2738,7 @@ const struct iommu_ops __initconstrel in }; const struct iommu_init_ops __initconstrel intel_iommu_init_ops = { + .ops = &intel_iommu_ops, .setup = vtd_setup, .supports_x2apic = intel_iommu_supports_eim, }; --- a/xen/drivers/passthrough/x86/iommu.c +++ b/xen/drivers/passthrough/x86/iommu.c @@ -26,6 +26,24 @@ const struct iommu_init_ops *__initdata iommu_init_ops; struct iommu_ops __read_mostly iommu_ops; +int iommu_enable_x2apic_IR(void) +{ + if ( system_state < SYS_STATE_active ) + { + if ( !iommu_supports_x2apic() ) + return -EOPNOTSUPP; + + iommu_ops = *iommu_init_ops->ops; + } + else if ( !x2apic_enabled ) + return -EOPNOTSUPP; + + if ( !iommu_ops.enable_x2apic_IR ) + return -EOPNOTSUPP; + + return iommu_ops.enable_x2apic_IR(); +} + void iommu_update_ire_from_apic( unsigned int apic, unsigned int reg, unsigned int value) { --- a/xen/include/asm-x86/apic.h +++ b/xen/include/asm-x86/apic.h @@ -29,7 +29,6 @@ enum apic_mode { }; extern u8 apic_verbosity; -extern bool x2apic_enabled; extern bool directed_eoi_enabled; void check_x2apic_preenabled(void); --- a/xen/include/asm-x86/apicdef.h +++ b/xen/include/asm-x86/apicdef.h @@ -126,4 +126,6 @@ #define MAX_IO_APICS 128 +extern bool x2apic_enabled; + #endif --- a/xen/include/asm-x86/iommu.h +++ b/xen/include/asm-x86/iommu.h @@ -17,6 +17,7 @@ #include <xen/errno.h> #include <xen/list.h> #include <xen/spinlock.h> +#include <asm/apicdef.h> #include <asm/processor.h> #include <asm/hvm/vmx/vmcs.h> @@ -65,6 +66,7 @@ static inline const struct iommu_ops *io } struct iommu_init_ops { + const struct iommu_ops *ops; int (*setup)(void); bool (*supports_x2apic)(void); }; @@ -97,7 +99,12 @@ static inline bool iommu_supports_x2apic } int iommu_enable_x2apic_IR(void); -void iommu_disable_x2apic_IR(void); + +static inline void iommu_disable_x2apic_IR(void) +{ + if ( x2apic_enabled && iommu_ops.disable_x2apic_IR ) + iommu_ops.disable_x2apic_IR(); +} extern bool untrusted_msi; --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -216,11 +216,16 @@ struct iommu_ops { unsigned int *flags); void (*free_page_table)(struct page_info *); + #ifdef CONFIG_X86 + int (*enable_x2apic_IR)(void); + void (*disable_x2apic_IR)(void); + void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, unsigned int value); unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int reg); int (*setup_hpet_msi)(struct msi_desc *); #endif /* CONFIG_X86 */ + int __must_check (*suspend)(void); void (*resume)(void); void (*share_p2m)(struct domain *d);
Introduce respective elements in struct iommu_init_ops as well as a pointer to the main ops structure. Signed-off-by: Jan Beulich <jbeulich@suse.com>