While commit 46c4061cd2bf ("x86/IOMMU: mark IOMMU / intremap not in use
when ACPI tables are missing") deals with apic_x2apic_probe() as called
from x2apic_bsp_setup(), the check_x2apic_preenabled() path is similarly
affected: The call needs to occur after acpi_iommu_init(), such that
iommu_intremap getting disabled there can be properly taken into account
by apic_x2apic_probe().

Note that, for the time being (further cleanup patches following),
reversing the order of the calls to generic_apic_probe() and
acpi_boot_init() is not an option:
- acpi_process_madt() calls clustered_apic_check() and hence relies on
  genapic to have got filled before,
- generic_bigsmp_probe() (called from acpi_process_madt()) needs to
  occur after generic_apic_probe(),
- acpi_parse_madt() (called from acpi_process_madt()) calls
  acpi_madt_oem_check(), which wants to be after generic_apic_probe().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Based on code inspection only - I have no affected system and hence no
way to actually test the case.
---
v2: Don't move generic_apic_probe() invocation, instead pull out
    acpi_iommu_init() from acpi_boot_init().
---
4.16: While investigating the issue addressed by the referenced commit,
      a variant of that problem was identified when firmware pre-enables
      x2APIC mode. Whether that's something sane firmware would do when
      at the same time IOMMU(s) is/are disabled is unclear, so this may
      be a purely academical consideration. Working around the problem
      also ought to be as simple as passing "iommu=no-intremap" on the
      command line. Considering the fragility of the code (as further
      demonstrated by v1 having been completely wrong), it may therefore
      be advisable to defer this change until after branching.
      Nevertheless it will then be a backporting candidate, so
      considering to take it right away can't simply be put off.

--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -757,8 +757,6 @@ int __init acpi_boot_init(void)
 
 	acpi_mmcfg_init();
 
-	acpi_iommu_init();
-
 	erst_init();
 
 	acpi_hest_init();
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1699,6 +1699,13 @@ void __init noreturn __start_xen(unsigne
 
     dmi_scan_machine();
 
+    /*
+     * IOMMU-related ACPI table parsing has to happen before APIC probing, for
+     * check_x2apic_preenabled() to be able to observe respective findings, in
+     * particular iommu_intremap having got turned off.
+     */
+    acpi_iommu_init();
+
     generic_apic_probe();
 
     mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges",

On Fri, Nov 05, 2021 at 01:32:18PM +0100, Jan Beulich wrote:
> While commit 46c4061cd2bf ("x86/IOMMU: mark IOMMU / intremap not in use
> when ACPI tables are missing") deals with apic_x2apic_probe() as called
> from x2apic_bsp_setup(), the check_x2apic_preenabled() path is similarly
> affected: The call needs to occur after acpi_iommu_init(), such that
> iommu_intremap getting disabled there can be properly taken into account
> by apic_x2apic_probe().
> 
> Note that, for the time being (further cleanup patches following),
> reversing the order of the calls to generic_apic_probe() and
> acpi_boot_init() is not an option:
> - acpi_process_madt() calls clustered_apic_check() and hence relies on
>   genapic to have got filled before,
> - generic_bigsmp_probe() (called from acpi_process_madt()) needs to
>   occur after generic_apic_probe(),
> - acpi_parse_madt() (called from acpi_process_madt()) calls
>   acpi_madt_oem_check(), which wants to be after generic_apic_probe().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Based on code inspection only - I have no affected system and hence no
> way to actually test the case.
> ---
> v2: Don't move generic_apic_probe() invocation, instead pull out
>     acpi_iommu_init() from acpi_boot_init().
> ---
> 4.16: While investigating the issue addressed by the referenced commit,
>       a variant of that problem was identified when firmware pre-enables
>       x2APIC mode. Whether that's something sane firmware would do when
>       at the same time IOMMU(s) is/are disabled is unclear, so this may
>       be a purely academical consideration. Working around the problem
>       also ought to be as simple as passing "iommu=no-intremap" on the
>       command line. Considering the fragility of the code (as further
>       demonstrated by v1 having been completely wrong), it may therefore
>       be advisable to defer this change until after branching.
>       Nevertheless it will then be a backporting candidate, so
>       considering to take it right away can't simply be put off.

The main issue here would be missing a dependency between
acpi_iommu_init and the rest of acpi_boot_init, apart from the
existing dependencies between acpi_iommu_init and generic_apic_probe.

> 
> --- a/xen/arch/x86/acpi/boot.c
> +++ b/xen/arch/x86/acpi/boot.c
> @@ -757,8 +757,6 @@ int __init acpi_boot_init(void)
>  
>  	acpi_mmcfg_init();
>  
> -	acpi_iommu_init();
> -
>  	erst_init();
>  
>  	acpi_hest_init();
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1699,6 +1699,13 @@ void __init noreturn __start_xen(unsigne
>  
>      dmi_scan_machine();
>  
> +    /*
> +     * IOMMU-related ACPI table parsing has to happen before APIC probing, for
> +     * check_x2apic_preenabled() to be able to observe respective findings, in
> +     * particular iommu_intremap having got turned off.
> +     */
> +    acpi_iommu_init();

If we pull this out I think we should add a check for acpi_disabled
and if set turn off iommu_intremap and iommu_enable?

Thanks, Roger.

Roger Pau Monné writes ("Re: [PATCH v2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing"):
> > 4.16: While investigating the issue addressed by the referenced commit,
> >       a variant of that problem was identified when firmware pre-enables
> >       x2APIC mode. Whether that's something sane firmware would do when
> >       at the same time IOMMU(s) is/are disabled is unclear, so this may
> >       be a purely academical consideration. Working around the problem
> >       also ought to be as simple as passing "iommu=no-intremap" on the
> >       command line. Considering the fragility of the code (as further
> >       demonstrated by v1 having been completely wrong), it may therefore
> >       be advisable to defer this change until after branching.

Thanks for the frank analysis.

> The main issue here would be missing a dependency between
> acpi_iommu_init and the rest of acpi_boot_init, apart from the
> existing dependencies between acpi_iommu_init and generic_apic_probe.

I have been thinking about this.  I'm still not sure.

> >       Nevertheless it will then be a backporting candidate, so
> >       considering to take it right away can't simply be put off.

This part confused me.  Under what circumstances would we backport
this ?  AIUI it would be backporting a potentially-fragile and
not-readily-testable bugfix, for a theoretical scenario with a
straightforward workaround.

Ian.

On 05.11.2021 16:47, Ian Jackson wrote:
> Roger Pau Monné writes ("Re: [PATCH v2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing"):
>>> 4.16: While investigating the issue addressed by the referenced commit,
>>>       a variant of that problem was identified when firmware pre-enables
>>>       x2APIC mode. Whether that's something sane firmware would do when
>>>       at the same time IOMMU(s) is/are disabled is unclear, so this may
>>>       be a purely academical consideration. Working around the problem
>>>       also ought to be as simple as passing "iommu=no-intremap" on the
>>>       command line. Considering the fragility of the code (as further
>>>       demonstrated by v1 having been completely wrong), it may therefore
>>>       be advisable to defer this change until after branching.
> 
> Thanks for the frank analysis.
> 
>> The main issue here would be missing a dependency between
>> acpi_iommu_init and the rest of acpi_boot_init, apart from the
>> existing dependencies between acpi_iommu_init and generic_apic_probe.
> 
> I have been thinking about this.  I'm still not sure.
> 
>>>       Nevertheless it will then be a backporting candidate, so
>>>       considering to take it right away can't simply be put off.
> 
> This part confused me.  Under what circumstances would we backport
> this ?  AIUI it would be backporting a potentially-fragile and
> not-readily-testable bugfix, for a theoretical scenario with a
> straightforward workaround.

Well, I've said "candidate" for this very reason: To me, every bug
fix is a candidate. Whether risks outweigh the potential benefits is
then influencing whether to _actually_ take the change. A reason to
take it despite the available workaround might be that
"straightforward" doesn't also mean "obvious" here. IOW once you
know what to do, doing so is easy. But one first needs to arrive
there.

Jan

Jan Beulich writes ("Re: [PATCH v2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing"):
> On 05.11.2021 16:47, Ian Jackson wrote:
> > This part confused me.  Under what circumstances would we backport
> > this ?  AIUI it would be backporting a potentially-fragile and
> > not-readily-testable bugfix, for a theoretical scenario with a
> > straightforward workaround.
> 
> Well, I've said "candidate" for this very reason: To me, every bug
> fix is a candidate. Whether risks outweigh the potential benefits is
> then influencing whether to _actually_ take the change. A reason to
> take it despite the available workaround might be that
> "straightforward" doesn't also mean "obvious" here. IOW once you
> know what to do, doing so is easy. But one first needs to arrive
> there.

Could we not do a smaller fix that would print something in the boot
output, mabye ?  That would be a lower risk change.

So far, I think the tradeoff here isn't looking very good: a risk of
unclear magnitude for many users, vs a hard crash at boot for a set of
users we believe to be empty.

As ever, feel free to contradict me if I have the wrong end of one of
the many sticks here...

Ian.

On 08.11.2021 16:04, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH v2 1/6][4.16?] x86/x2APIC: defer probe until after IOMMU ACPI table parsing"):
>> On 05.11.2021 16:47, Ian Jackson wrote:
>>> This part confused me.  Under what circumstances would we backport
>>> this ?  AIUI it would be backporting a potentially-fragile and
>>> not-readily-testable bugfix, for a theoretical scenario with a
>>> straightforward workaround.
>>
>> Well, I've said "candidate" for this very reason: To me, every bug
>> fix is a candidate. Whether risks outweigh the potential benefits is
>> then influencing whether to _actually_ take the change. A reason to
>> take it despite the available workaround might be that
>> "straightforward" doesn't also mean "obvious" here. IOW once you
>> know what to do, doing so is easy. But one first needs to arrive
>> there.
> 
> Could we not do a smaller fix that would print something in the boot
> output, mabye ?  That would be a lower risk change.

Hmm, maybe something could be done, but at the risk of getting the
conditions there wrong (and hence having false positives and/or
false negatives, confusing users at best) and with likely a clumsy
log message ("abc ran before xyz"), suggesting that we actually
know how to do better. IOW - I'd rather not go this route, and it
would feel better to me to simply defer this change to post-4.16
if we deem it too risky to put it in now.

> So far, I think the tradeoff here isn't looking very good: a risk of
> unclear magnitude for many users, vs a hard crash at boot for a set of
> users we believe to be empty.
> 
> As ever, feel free to contradict me if I have the wrong end of one of
> the many sticks here...

I think you've got it quite right. I did put the question mark in the
tag specifically to make clear that while I'd like this to be
considered, I'm myself not convinced the risks outweigh the benefits.

Jan

I wrote:
> Could we not do a smaller fix that would print something in the boot
> output, mabye ?  That would be a lower risk change.
> 
> So far, I think the tradeoff here isn't looking very good: a risk of
> unclear magnitude for many users, vs a hard crash at boot for a set of
> users we believe to be empty.
> 
> As ever, feel free to contradict me if I have the wrong end of one of
> the many sticks here...

On the question of backporting:

Jan Beulich:
> >Ian Jackson:
> >>>       Nevertheless it will then be a backporting candidate, so
> >>>       considering to take it right away can't simply be put off.
> > 
> > This part confused me.  Under what circumstances would we backport
> > this ?  AIUI it would be backporting a potentially-fragile and
> > not-readily-testable bugfix, for a theoretical scenario with a
> > straightforward workaround.
> 
> Well, I've said "candidate" for this very reason: To me, every bug
> fix is a candidate. Whether risks outweigh the potential benefits is
> then influencing whether to _actually_ take the change. A reason to
> take it despite the available workaround might be that
> "straightforward" doesn't also mean "obvious" here. IOW once you
> know what to do, doing so is easy. But one first needs to arrive
> there.

I didn't find this particularly convincing, TBH.

Despite the above exchanges, this patch is still marked 4.16? and the
release discussion inthe patch does not seem to have bene updated to
take into account this discussion.

In any case, I think at this stage of the freeze this patch is not a
good bet, particularly seeing as it has had to have another round.

Thanks,
Ian.

On 05.11.2021 16:38, Roger Pau Monné wrote:
> On Fri, Nov 05, 2021 at 01:32:18PM +0100, Jan Beulich wrote:
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1699,6 +1699,13 @@ void __init noreturn __start_xen(unsigne
>>  
>>      dmi_scan_machine();
>>  
>> +    /*
>> +     * IOMMU-related ACPI table parsing has to happen before APIC probing, for
>> +     * check_x2apic_preenabled() to be able to observe respective findings, in
>> +     * particular iommu_intremap having got turned off.
>> +     */
>> +    acpi_iommu_init();
> 
> If we pull this out I think we should add a check for acpi_disabled
> and if set turn off iommu_intremap and iommu_enable?

Hmm, I should have added a note regarding this. If we want to exactly
retain prior behavior, acpi_ht would also need checking. Yet that has
gone wrong long ago: We parse way too many tables when acpi_disabled
&& acpi_ht, and hence while correct wrt to prior behavior I'd consider
it wrong to (re)add a "!acpi_ht" check.

As a result I'm of the opinion that checking acpi_disabled here also
isn't necessarily appropriate, and instead IOMMU disabling would
better be solely under the control of "iommu=".

Additionally iirc Andrew has been suggesting to drop all this "ACPI
disabled / HT-only" machinery (I'm somewhat hesitant with that, but
as a result I'm also not very eager to actually correct to accumulated
bad behavior). The change here simply would be a tiny first step in
that direction.

Jan

On Mon, Nov 08, 2021 at 08:40:59AM +0100, Jan Beulich wrote:
> On 05.11.2021 16:38, Roger Pau Monné wrote:
> > On Fri, Nov 05, 2021 at 01:32:18PM +0100, Jan Beulich wrote:
> >> --- a/xen/arch/x86/setup.c
> >> +++ b/xen/arch/x86/setup.c
> >> @@ -1699,6 +1699,13 @@ void __init noreturn __start_xen(unsigne
> >>  
> >>      dmi_scan_machine();
> >>  
> >> +    /*
> >> +     * IOMMU-related ACPI table parsing has to happen before APIC probing, for
> >> +     * check_x2apic_preenabled() to be able to observe respective findings, in
> >> +     * particular iommu_intremap having got turned off.
> >> +     */
> >> +    acpi_iommu_init();
> > 
> > If we pull this out I think we should add a check for acpi_disabled
> > and if set turn off iommu_intremap and iommu_enable?
> 
> Hmm, I should have added a note regarding this. If we want to exactly
> retain prior behavior, acpi_ht would also need checking. Yet that has
> gone wrong long ago: We parse way too many tables when acpi_disabled
> && acpi_ht, and hence while correct wrt to prior behavior I'd consider
> it wrong to (re)add a "!acpi_ht" check.
> 
> As a result I'm of the opinion that checking acpi_disabled here also
> isn't necessarily appropriate, and instead IOMMU disabling would
> better be solely under the control of "iommu=".

I haven't looked very deeply, but will the acpi helpers work correctly
in that case? As acpi_boot_table_init will be short-circuited if
 `acpi_disabled && !acpi_ht`.

Thanks, Roger.

On 08.11.2021 10:36, Roger Pau Monné wrote:
> On Mon, Nov 08, 2021 at 08:40:59AM +0100, Jan Beulich wrote:
>> On 05.11.2021 16:38, Roger Pau Monné wrote:
>>> On Fri, Nov 05, 2021 at 01:32:18PM +0100, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/setup.c
>>>> +++ b/xen/arch/x86/setup.c
>>>> @@ -1699,6 +1699,13 @@ void __init noreturn __start_xen(unsigne
>>>>  
>>>>      dmi_scan_machine();
>>>>  
>>>> +    /*
>>>> +     * IOMMU-related ACPI table parsing has to happen before APIC probing, for
>>>> +     * check_x2apic_preenabled() to be able to observe respective findings, in
>>>> +     * particular iommu_intremap having got turned off.
>>>> +     */
>>>> +    acpi_iommu_init();
>>>
>>> If we pull this out I think we should add a check for acpi_disabled
>>> and if set turn off iommu_intremap and iommu_enable?
>>
>> Hmm, I should have added a note regarding this. If we want to exactly
>> retain prior behavior, acpi_ht would also need checking. Yet that has
>> gone wrong long ago: We parse way too many tables when acpi_disabled
>> && acpi_ht, and hence while correct wrt to prior behavior I'd consider
>> it wrong to (re)add a "!acpi_ht" check.
>>
>> As a result I'm of the opinion that checking acpi_disabled here also
>> isn't necessarily appropriate, and instead IOMMU disabling would
>> better be solely under the control of "iommu=".
> 
> I haven't looked very deeply, but will the acpi helpers work correctly
> in that case? As acpi_boot_table_init will be short-circuited if
>  `acpi_disabled && !acpi_ht`.

Oh, that's a good point you make.

Jan