| Message ID | bf497b79-7661-8fa8-6979-90f9951c8735@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IOMMU/x86: fix build with old gcc after IO-APIC RTE changes | expand |
On 16/08/2023 10:51 am, Jan Beulich wrote: > Old gcc won't cope with initializers involving unnamed struct/union > fields. > > Fixes: 3e033172b025 ("x86/iommu: pass full IO-APIC RTE for remapping table update") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, although > --- a/xen/drivers/passthrough/vtd/intremap.c > +++ b/xen/drivers/passthrough/vtd/intremap.c > @@ -432,8 +432,7 @@ unsigned int cf_check io_apic_read_remap > void cf_check io_apic_write_remap_rte( > unsigned int apic, unsigned int pin, uint64_t rte) > { > - struct IO_xAPIC_route_entry new_rte = { .raw = rte }; > - struct IO_xAPIC_route_entry old_rte = { }; > + struct IO_xAPIC_route_entry old_rte = { }, new_rte; Any chance we can make this = {} while at it? ~Andrew
Hi Jan, On 16/08/2023 10:51, Jan Beulich wrote: > Old gcc won't cope with initializers involving unnamed struct/union Can you specify the newest version of GCC that breaks? This would help to reproduce your problem in case someone complain about this change. > fields. Cheers,
On 16/08/2023 10:51 am, Jan Beulich wrote: > Old gcc won't cope with initializers involving unnamed struct/union > fields. > > Fixes: 3e033172b025 ("x86/iommu: pass full IO-APIC RTE for remapping table update") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, although > --- a/xen/drivers/passthrough/vtd/intremap.c > +++ b/xen/drivers/passthrough/vtd/intremap.c > @@ -432,8 +432,7 @@ unsigned int cf_check io_apic_read_remap > void cf_check io_apic_write_remap_rte( > unsigned int apic, unsigned int pin, uint64_t rte) > { > - struct IO_xAPIC_route_entry new_rte = { .raw = rte }; > - struct IO_xAPIC_route_entry old_rte = { }; > + struct IO_xAPIC_route_entry old_rte = { }, new_rte; Any chance we can make this = {} while at it? ~Andrew
On 16.08.2023 18:40, Andrew Cooper wrote: > On 16/08/2023 10:51 am, Jan Beulich wrote: >> Old gcc won't cope with initializers involving unnamed struct/union >> fields. >> >> Fixes: 3e033172b025 ("x86/iommu: pass full IO-APIC RTE for remapping table update") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, although Thanks. >> --- a/xen/drivers/passthrough/vtd/intremap.c >> +++ b/xen/drivers/passthrough/vtd/intremap.c >> @@ -432,8 +432,7 @@ unsigned int cf_check io_apic_read_remap >> void cf_check io_apic_write_remap_rte( >> unsigned int apic, unsigned int pin, uint64_t rte) >> { >> - struct IO_xAPIC_route_entry new_rte = { .raw = rte }; >> - struct IO_xAPIC_route_entry old_rte = { }; >> + struct IO_xAPIC_route_entry old_rte = { }, new_rte; > > Any chance we can make this = {} while at it? Sure, no problem at all. To be honest it's not really clear to me whether we prefer either form, and if so which one. Jan
On 16.08.2023 18:57, Julien Grall wrote: > On 16/08/2023 10:51, Jan Beulich wrote: >> Old gcc won't cope with initializers involving unnamed struct/union > > Can you specify the newest version of GCC that breaks? This would help > to reproduce your problem in case someone complain about this change. I can't, without actually putting in effort to find out. I'm observing these problems with 4.3.x iirc. And of course this isn't the first such change, and I don't think we ever bothered writing down precise version boundaries in any of the commits. Sorry, Jan
Hi Jan, On 17/08/2023 07:39, Jan Beulich wrote: > On 16.08.2023 18:57, Julien Grall wrote: >> On 16/08/2023 10:51, Jan Beulich wrote: >>> Old gcc won't cope with initializers involving unnamed struct/union >> >> Can you specify the newest version of GCC that breaks? This would help >> to reproduce your problem in case someone complain about this change. > > I can't, without actually putting in effort to find out. I'm observing > these problems with 4.3.x iirc. You are proving my point. :) If you can't already remember which version of GCC was breaking. How can you expect someone in a few months time to figure out why this was added and whether and it can reworked differently? > And of course this isn't the first such > change, and I don't think we ever bothered writing down precise version > boundaries in any of the commits. I am not looking at a precise boundary. What I meant by the 'newest' is the newest one you try. With 'old', it is not clear what this really mean. For instance, technically the previous GCC version is already old. So a bit more information about the GCC version you tried on would be useful. Cheers,
On 17.08.2023 09:06, Julien Grall wrote: > On 17/08/2023 07:39, Jan Beulich wrote: >> On 16.08.2023 18:57, Julien Grall wrote: >>> On 16/08/2023 10:51, Jan Beulich wrote: >>>> Old gcc won't cope with initializers involving unnamed struct/union >>> >>> Can you specify the newest version of GCC that breaks? This would help >>> to reproduce your problem in case someone complain about this change. >> >> I can't, without actually putting in effort to find out. I'm observing >> these problems with 4.3.x iirc. > > You are proving my point. :) If you can't already remember which version > of GCC was breaking. How can you expect someone in a few months time to > figure out why this was added and whether and it can reworked differently? Well, I know for sure that this doesn't work with the version recorded in ./README. Imo that's sufficient to justify submitting patches like this, and without going into version details. Once that baseline version is bumped, much more than just this code can and wants to be re-evaluated, by simply trying with the then-lowest supported version (which imo really ought to be part of what is tested in CI, to not always leave it to me to find and fix such issues). >> And of course this isn't the first such >> change, and I don't think we ever bothered writing down precise version >> boundaries in any of the commits. > > I am not looking at a precise boundary. What I meant by the 'newest' is > the newest one you try. Okay, that's slightly different and hence possible to record. I can do so here just to please you, but as per above I don't think that ought to be a requirement (and as said earlier it also hasn't been in the past). > With 'old', it is not clear what this really mean. For instance, > technically the previous GCC version is already old. So a bit more > information about the GCC version you tried on would be useful. Hmm, no, that's not really my interpretation of "old". Jan
Hi, On 17/08/2023 08:25, Jan Beulich wrote: > On 17.08.2023 09:06, Julien Grall wrote: >> On 17/08/2023 07:39, Jan Beulich wrote: >>> On 16.08.2023 18:57, Julien Grall wrote: >>>> On 16/08/2023 10:51, Jan Beulich wrote: >>>>> Old gcc won't cope with initializers involving unnamed struct/union >>>> >>>> Can you specify the newest version of GCC that breaks? This would help >>>> to reproduce your problem in case someone complain about this change. >>> >>> I can't, without actually putting in effort to find out. I'm observing >>> these problems with 4.3.x iirc. >> >> You are proving my point. :) If you can't already remember which version >> of GCC was breaking. How can you expect someone in a few months time to >> figure out why this was added and whether and it can reworked differently? > > Well, I know for sure that this doesn't work with the version recorded in > ./README. Imo that's sufficient to justify submitting patches like this, > and without going into version details. Once that baseline version is > bumped, much more than just this code can and wants to be re-evaluated, > by simply trying with the then-lowest supported version (which imo really > ought to be part of what is tested in CI, to not always leave it to me to > find and fix such issues). Right, but to do it efficiently, you will want to know when the issue was introduced. So you have a rough idea whether it is worth investigating to remove. For instance, this may be a bug in GCC 4.8 but we only bump the requirement to GCC 4.5. If you know it was in GCC 4.8 then you don't need to re-assess. > >>> And of course this isn't the first such >>> change, and I don't think we ever bothered writing down precise version >>> boundaries in any of the commits. >> >> I am not looking at a precise boundary. What I meant by the 'newest' is >> the newest one you try. > > Okay, that's slightly different and hence possible to record. I can do > so here just to please you, but as per above I don't think that ought to > be a requirement It is not about pleasing me here. It is for us to save time and argument in the future. I still have in mind an optimization we tried to revert recently because it wasn't one. But one of the maintainer were not willing to revert as it may have helped in some situation which were not documented and nobody could reproduced it. This is not an optimization here. But I am concerned, this would end up to a similar situation. This we could easily be prevented by been a bit more verbose up front... > (and as said earlier it also hasn't been in the past). Which is fine. Process evolved based on experience. > >> With 'old', it is not clear what this really mean. For instance, >> technically the previous GCC version is already old. So a bit more >> information about the GCC version you tried on would be useful. > > Hmm, no, that's not really my interpretation of "old". 'old' doesn't really have any exact time other than been in the past. It doesn't hurt to qualify it properly... At the end of the day, this is x86 code. So feel free to ignore it. Hopefully, I will not be the person reporting an issue related to this change :). Cheers,
> From: Jan Beulich <jbeulich@suse.com> > Sent: Wednesday, August 16, 2023 5:52 PM > > Old gcc won't cope with initializers involving unnamed struct/union > fields. > > Fixes: 3e033172b025 ("x86/iommu: pass full IO-APIC RTE for remapping table > update") > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
--- a/xen/drivers/passthrough/amd/iommu_intr.c +++ b/xen/drivers/passthrough/amd/iommu_intr.c @@ -321,8 +321,7 @@ static int update_intremap_entry_from_io void cf_check amd_iommu_ioapic_update_ire( unsigned int apic, unsigned int pin, uint64_t rte) { - struct IO_APIC_route_entry old_rte; - struct IO_APIC_route_entry new_rte = { .raw = rte }; + struct IO_APIC_route_entry old_rte, new_rte; int seg, bdf, rc; struct amd_iommu *iommu; unsigned int idx; @@ -331,6 +330,9 @@ void cf_check amd_iommu_ioapic_update_ir if ( idx == MAX_IO_APICS ) return; + /* Not the initializer, for old gcc to cope. */ + new_rte.raw = rte; + /* get device id of ioapic devices */ bdf = ioapic_sbdf[idx].bdf; seg = ioapic_sbdf[idx].seg; --- a/xen/drivers/passthrough/vtd/intremap.c +++ b/xen/drivers/passthrough/vtd/intremap.c @@ -432,8 +432,7 @@ unsigned int cf_check io_apic_read_remap void cf_check io_apic_write_remap_rte( unsigned int apic, unsigned int pin, uint64_t rte) { - struct IO_xAPIC_route_entry new_rte = { .raw = rte }; - struct IO_xAPIC_route_entry old_rte = { }; + struct IO_xAPIC_route_entry old_rte = { }, new_rte; struct vtd_iommu *iommu = ioapic_to_iommu(IO_APIC_ID(apic)); bool masked = true; int rc; @@ -453,6 +452,9 @@ void cf_check io_apic_write_remap_rte( } } + /* Not the initializer, for old gcc to cope. */ + new_rte.raw = rte; + rc = ioapic_rte_to_remap_entry(iommu, apic, pin, &old_rte, new_rte); if ( rc ) {
Old gcc won't cope with initializers involving unnamed struct/union fields. Fixes: 3e033172b025 ("x86/iommu: pass full IO-APIC RTE for remapping table update") Signed-off-by: Jan Beulich <jbeulich@suse.com>