In the course of reading the response to v1 (patch 1 only) I realized
that not only that patch needs further adjustment, but that also
further changes are needed (and there's likely yet more amiss).

1: x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing
2: x86/APIC: avoid iommu_supports_x2apic() on error path
3: AMD/IOMMU: iommu_enable vs iommu_intremap

Jan

On 21.10.2021 11:57, Jan Beulich wrote:
> In the course of reading the response to v1 (patch 1 only) I realized
> that not only that patch needs further adjustment, but that also
> further changes are needed (and there's likely yet more amiss).
> 
> 1: x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing
> 2: x86/APIC: avoid iommu_supports_x2apic() on error path
> 3: AMD/IOMMU: iommu_enable vs iommu_intremap

Ian, while we further discuss / refine patch 3, the first two have the
needed R-b, but will now need you release-ack aiui.

Andrew, did you perhaps have a chance to actually try v2 of patch 1? It
works for me when suitably configuring the BIOS on my Skylake, so I
wouldn't feel uncertain in committing without a Tested-by, but it would
feel even better if I had one.

Thanks, Jan

On 02.11.2021 11:17, Jan Beulich wrote:
> On 21.10.2021 11:57, Jan Beulich wrote:
>> In the course of reading the response to v1 (patch 1 only) I realized
>> that not only that patch needs further adjustment, but that also
>> further changes are needed (and there's likely yet more amiss).
>>
>> 1: x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing
>> 2: x86/APIC: avoid iommu_supports_x2apic() on error path
>> 3: AMD/IOMMU: iommu_enable vs iommu_intremap
> 
> Ian, while we further discuss / refine patch 3, the first two have the
> needed R-b, but will now need you release-ack aiui.

Seeing your reply on IRC, here an attempt at a release justification
(the patches were ready by Oct 29, but no-one cared to commit them
in my absence, so I thought I'd get away without such a write-up):

Patch 1 addresses a regression identified by Andrew. The main risk I
see here (which has turned up only very recently) is disagreement on
patch 3 which imo has an effect also on what patch 1 does, as to the
(non-)effects of "iommu=off" on the hypervisor command line. This,
however, is not an effect of the patch, but pre-existing behavior.
The behavioral change (in this regard) is in patch 3, which is still
under discussion.

Patch 2 corrects an (unlikely but not impossible to be taken) error
path, supposedly making systems functional again in case they would
in fact cause that error path to be taken. The risk looks low to me,
given that two function calls with previously assumed to be
identical results now get folded into one with the result latched.

Jan

> Andrew, did you perhaps have a chance to actually try v2 of patch 1? It
> works for me when suitably configuring the BIOS on my Skylake, so I
> wouldn't feel uncertain in committing without a Tested-by, but it would
> feel even better if I had one.
> 
> Thanks, Jan
> 

Jan Beulich writes ("Re: [PATCH v2 0/3][4.16] x86/IOMMU: enabled / intremap handling"):
> On 02.11.2021 11:17, Jan Beulich wrote:
> > On 21.10.2021 11:57, Jan Beulich wrote:
> >> In the course of reading the response to v1 (patch 1 only) I realized
> >> that not only that patch needs further adjustment, but that also
> >> further changes are needed (and there's likely yet more amiss).
> >>
> >> 1: x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing
> >> 2: x86/APIC: avoid iommu_supports_x2apic() on error path
> >> 3: AMD/IOMMU: iommu_enable vs iommu_intremap
> > 
> > Ian, while we further discuss / refine patch 3, the first two have the
> > needed R-b, but will now need you release-ack aiui.
> 
> Seeing your reply on IRC, here an attempt at a release justification
> (the patches were ready by Oct 29, but no-one cared to commit them
> in my absence, so I thought I'd get away without such a write-up):
> 
> Patch 1 addresses a regression identified by Andrew. The main risk I
> see here (which has turned up only very recently) is disagreement on
> patch 3 which imo has an effect also on what patch 1 does, as to the
> (non-)effects of "iommu=off" on the hypervisor command line. This,
> however, is not an effect of the patch, but pre-existing behavior.
> The behavioral change (in this regard) is in patch 3, which is still
> under discussion.

Thank you.  I also went to the list and read the thread there.

Patch 1:

Reviewed-by: Ian Jackson <iwj@xenproject.org>

> Patch 2 corrects an (unlikely but not impossible to be taken) error
> path, supposedly making systems functional again in case they would
> in fact cause that error path to be taken. The risk looks low to me,
> given that two function calls with previously assumed to be
> identical results now get folded into one with the result latched.

This one also:

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

I think, from reading the thread, that patch 3 is not targeting 4.16.

Thanks,
Ian.

On 03.11.2021 17:19, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH v2 0/3][4.16] x86/IOMMU: enabled / intremap handling"):
>> On 02.11.2021 11:17, Jan Beulich wrote:
>>> On 21.10.2021 11:57, Jan Beulich wrote:
>>>> In the course of reading the response to v1 (patch 1 only) I realized
>>>> that not only that patch needs further adjustment, but that also
>>>> further changes are needed (and there's likely yet more amiss).
>>>>
>>>> 1: x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing
>>>> 2: x86/APIC: avoid iommu_supports_x2apic() on error path
>>>> 3: AMD/IOMMU: iommu_enable vs iommu_intremap
>>>
>>> Ian, while we further discuss / refine patch 3, the first two have the
>>> needed R-b, but will now need you release-ack aiui.
>>
>> Seeing your reply on IRC, here an attempt at a release justification
>> (the patches were ready by Oct 29, but no-one cared to commit them
>> in my absence, so I thought I'd get away without such a write-up):
>>
>> Patch 1 addresses a regression identified by Andrew. The main risk I
>> see here (which has turned up only very recently) is disagreement on
>> patch 3 which imo has an effect also on what patch 1 does, as to the
>> (non-)effects of "iommu=off" on the hypervisor command line. This,
>> however, is not an effect of the patch, but pre-existing behavior.
>> The behavioral change (in this regard) is in patch 3, which is still
>> under discussion.
> 
> Thank you.  I also went to the list and read the thread there.
> 
> Patch 1:
> 
> Reviewed-by: Ian Jackson <iwj@xenproject.org>
> 
>> Patch 2 corrects an (unlikely but not impossible to be taken) error
>> path, supposedly making systems functional again in case they would
>> in fact cause that error path to be taken. The risk looks low to me,
>> given that two function calls with previously assumed to be
>> identical results now get folded into one with the result latched.
> 
> This one also:
> 
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Thanks, but your reply leaves me a little confused: Your use of "also"
may mean R-b for both patches but R-a-b only for patch 2. But I could
also find a variety of other interpretations, including that the
first R-b really was meant to be R-a-b (which otherwise I'd need on
top of the R-b anyway). Please clarify.

> I think, from reading the thread, that patch 3 is not targeting 4.16.

Correct. The other related one now targeting 4.16 is the separately
submitted "x86/x2APIC: defer probe until after IOMMU ACPI table
parsing". But I can see reasons for you to prefer to have that
deferred.

Jan

Jan Beulich writes ("Re: [PATCH v2 0/3][4.16] x86/IOMMU: enabled / intremap handling"):
> > Reviewed-by: Ian Jackson <iwj@xenproject.org>
> > 
> >> Patch 2 corrects an (unlikely but not impossible to be taken) error
> >> path, supposedly making systems functional again in case they would
> >> in fact cause that error path to be taken. The risk looks low to me,
> >> given that two function calls with previously assumed to be
> >> identical results now get folded into one with the result latched.
> > 
> > This one also:
> > 
> > Release-Acked-by: Ian Jackson <iwj@xenproject.org>
> 
> Thanks, but your reply leaves me a little confused: Your use of "also"
> may mean R-b for both patches but R-a-b only for patch 2. But I could
> also find a variety of other interpretations, including that the
> first R-b really was meant to be R-a-b (which otherwise I'd need on
> top of the R-b anyway). Please clarify.

Um.  Well spotted.  I meant release-acked-by for both.  I botched the
keystroke in my MUA.  Sorry for the confusion.

So FTAAD I have *not* "Reviewed" either patch (although I did read it,
I don't consider myself qualified to give an R-b).

> > I think, from reading the thread, that patch 3 is not targeting 4.16.
> 
> Correct. The other related one now targeting 4.16 is the separately
> submitted "x86/x2APIC: defer probe until after IOMMU ACPI table
> parsing". But I can see reasons for you to prefer to have that
> deferred.

Thanks,
Ian.