| Message ID | 640545a6-689a-6ba9-4978-a83183df2e1b@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AMD/IOMMU: correct handling when XT's prereq features are unavailable | expand |
On Thu, Feb 27, 2020 at 03:34:48PM +0100, Jan Beulich wrote: > We should neither cause IOMMU initialization as a whole to fail in this > case (we should still be able to bring up the system in non-x2APIC or > x2APIC physical mode), nor should the remainder of the function be > skipped (as the main part of it won't get entered a 2nd time) in such an > event. It is merely necessary for the function to indicate to the caller > (iov_supports_xt()) that setup failed as far as x2APIC is concerned. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/drivers/passthrough/amd/iommu_init.c > +++ b/xen/drivers/passthrough/amd/iommu_init.c > @@ -1364,6 +1364,7 @@ static int __init amd_iommu_prepare_one( > int __init amd_iommu_prepare(bool xt) > { > struct amd_iommu *iommu; > + bool no_xt = false; > int rc = -ENODEV; > > BUG_ON( !iommu_found() ); > @@ -1400,9 +1401,8 @@ int __init amd_iommu_prepare(bool xt) > if ( rc ) > goto error_out; > > - rc = -ENODEV; > - if ( xt && (!iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup) ) > - goto error_out; > + if ( !iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup ) > + no_xt = true; Don't you need to also adjust the usage of xt in the for_each_amd_iommu loop below, so that the control registers fields get initialized properly? Thanks, Roger.
On 27/02/2020 14:34, Jan Beulich wrote: > --- a/xen/drivers/passthrough/amd/iommu_init.c > +++ b/xen/drivers/passthrough/amd/iommu_init.c > @@ -1364,6 +1364,7 @@ static int __init amd_iommu_prepare_one( > int __init amd_iommu_prepare(bool xt) > { > struct amd_iommu *iommu; > + bool no_xt = false; I think the logic would be easier to follow if this was has_xt, with inverted meaning. However... > int rc = -ENODEV; > > BUG_ON( !iommu_found() ); > @@ -1400,9 +1401,8 @@ int __init amd_iommu_prepare(bool xt) > if ( rc ) > goto error_out; > > - rc = -ENODEV; > - if ( xt && (!iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup) ) > - goto error_out; > + if ( !iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup ) > + no_xt = true; > } > > for_each_amd_iommu ( iommu ) ... the contents of this loop depends on the early exit path you've just deleted. In the case of x2apic not being available, we'll still set {ga,xt}_en to the caller requested value. ~Andrew
On 27.02.2020 16:25, Andrew Cooper wrote: > On 27/02/2020 14:34, Jan Beulich wrote: >> --- a/xen/drivers/passthrough/amd/iommu_init.c >> +++ b/xen/drivers/passthrough/amd/iommu_init.c >> @@ -1364,6 +1364,7 @@ static int __init amd_iommu_prepare_one( >> int __init amd_iommu_prepare(bool xt) >> { >> struct amd_iommu *iommu; >> + bool no_xt = false; > > I think the logic would be easier to follow if this was has_xt, with > inverted meaning. I'm not fully convinced it'll make it easier, but I've switched things around. > However... > >> int rc = -ENODEV; >> >> BUG_ON( !iommu_found() ); >> @@ -1400,9 +1401,8 @@ int __init amd_iommu_prepare(bool xt) >> if ( rc ) >> goto error_out; >> >> - rc = -ENODEV; >> - if ( xt && (!iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup) ) >> - goto error_out; >> + if ( !iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup ) >> + no_xt = true; >> } >> >> for_each_amd_iommu ( iommu ) > > ... the contents of this loop depends on the early exit path you've just > deleted. > > In the case of x2apic not being available, we'll still set {ga,xt}_en to > the caller requested value. Oh, indeed (and Roger, thank you too for noticing this). In fact it explains a hang later during boot that I did observe, and that I meant to look into later. That said, interrupts for Dom0 still don't seem to work quite right in this (partly broken) mode even with this fixed (there are several "No irq handler for vector" messages, and at the very least the disk doesn't work); I'll see to find time to look into that as well, but I'm pretty convinced it's an independent issue. (Seeing that interrupt remapping gets enabled this way, and x2APIC is pre-enabled, I'm suspecting this is another case where we need to force physical mode.) Let me blame this on the cold I'm fighting, which isn't quite bad enough to stay home, but which also isn't helpful. v2 coming ... Jan
On 28.02.2020 09:14, Jan Beulich wrote: > On 27.02.2020 16:25, Andrew Cooper wrote: >> On 27/02/2020 14:34, Jan Beulich wrote: >>> @@ -1400,9 +1401,8 @@ int __init amd_iommu_prepare(bool xt) >>> if ( rc ) >>> goto error_out; >>> >>> - rc = -ENODEV; >>> - if ( xt && (!iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup) ) >>> - goto error_out; >>> + if ( !iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup ) >>> + no_xt = true; >>> } >>> >>> for_each_amd_iommu ( iommu ) >> >> ... the contents of this loop depends on the early exit path you've just >> deleted. >> >> In the case of x2apic not being available, we'll still set {ga,xt}_en to >> the caller requested value. > > Oh, indeed (and Roger, thank you too for noticing this). In fact > it explains a hang later during boot that I did observe, and that > I meant to look into later. That said, interrupts for Dom0 still > don't seem to work quite right in this (partly broken) mode even > with this fixed (there are several "No irq handler for vector" > messages, and at the very least the disk doesn't work); I'll see > to find time to look into that as well, but I'm pretty convinced > it's an independent issue. (Seeing that interrupt remapping gets > enabled this way, and x2APIC is pre-enabled, I'm suspecting this > is another case where we need to force physical mode.) And indeed x2apic_phys on the command line makes things work. Now I'll have to figure a reasonable way when and how to communicate the need to switch the mode. (Another potential issue I'm seeing is that it may not be okay to bring up a CPU with APIC ID 0xff in this case, and it pretty certainly would require further precautions if we were to allow bringing up CPUs with even larger APIC IDs.) Jan
--- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -1364,6 +1364,7 @@ static int __init amd_iommu_prepare_one( int __init amd_iommu_prepare(bool xt) { struct amd_iommu *iommu; + bool no_xt = false; int rc = -ENODEV; BUG_ON( !iommu_found() ); @@ -1400,9 +1401,8 @@ int __init amd_iommu_prepare(bool xt) if ( rc ) goto error_out; - rc = -ENODEV; - if ( xt && (!iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup) ) - goto error_out; + if ( !iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup ) + no_xt = true; } for_each_amd_iommu ( iommu ) @@ -1422,7 +1422,7 @@ int __init amd_iommu_prepare(bool xt) ivhd_type = 0; } - return rc; + return rc ?: xt && no_xt ? -ENODEV : 0; } int __init amd_iommu_init(bool xt)
We should neither cause IOMMU initialization as a whole to fail in this case (we should still be able to bring up the system in non-x2APIC or x2APIC physical mode), nor should the remainder of the function be skipped (as the main part of it won't get entered a 2nd time) in such an event. It is merely necessary for the function to indicate to the caller (iov_supports_xt()) that setup failed as far as x2APIC is concerned. Signed-off-by: Jan Beulich <jbeulich@suse.com>