| Message ID | 20190606090146.77381-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pci: expand usage of pci_sbdf_t | expand |
> -----Original Message----- > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf Of Roger Pau Monne > Sent: 06 June 2019 10:02 > To: xen-devel@lists.xenproject.org > Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com>; George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper > <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>; Julien > Grall <julien.grall@arm.com>; Jan Beulich <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com> > Subject: [Xen-devel] [PATCH v2 01/12] pci: introduce a devfn field to pci_sbdf_t > > This is equivalent to the current extfunc field in term of contents. > > Switch the two current users of extfunc to use devfn instead for > correctness. > > No functional change. > > Requested-by: Jan Beulich <jbeulich@suse.com> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: George Dunlap <George.Dunlap@eu.citrix.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Julien Grall <julien.grall@arm.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Tim Deegan <tim@xen.org> > Cc: Wei Liu <wl@xen.org> > --- > Changes since v1: > - New in this version. > --- > NB: Paul suggested to name the function field fn instead of func, so > that it would match the naming of the devfn field. Sadly the func > field cannot be aliased to another field using a union because it's a > bit field, so the only option is to rename func to fn. Is that true? Can you not do something like... union { struct { uint8_t func : 3, dev : 5; }; struct { uint8_t fn : 3, pad : 5; }; uint8_t extfunc; }; ? I'm certainly not seeing any complaints from gcc. > I don't have a > strong opinion, but if there's consensus it should be done after this > patch IMO and not later in the series, as more occurrences of > sbdf.func will appear. IMO we either have 'devfunc' and 'func', or 'devfn' and 'fn'. Paul > --- > xen/drivers/vpci/vpci.c | 4 ++-- > xen/include/xen/pci.h | 5 ++++- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c > index 82607bdb9a..4f1f95ab69 100644 > --- a/xen/drivers/vpci/vpci.c > +++ b/xen/drivers/vpci/vpci.c > @@ -327,7 +327,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size) > } > > /* Find the PCI dev matching the address. */ > - pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.extfunc); > + pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn); > if ( !pdev ) > return vpci_read_hw(sbdf, reg, size); > > @@ -432,7 +432,7 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size, > * Find the PCI dev matching the address. > * Passthrough everything that's not trapped. > */ > - pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.extfunc); > + pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn); > if ( !pdev ) > { > vpci_write_hw(sbdf, reg, size, data); > diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h > index 8b21e8dc84..ec98274675 100644 > --- a/xen/include/xen/pci.h > +++ b/xen/include/xen/pci.h > @@ -49,7 +49,10 @@ typedef union { > uint8_t func : 3, > dev : 5; > }; > - uint8_t extfunc; > + union { > + uint8_t extfunc, > + devfn; > + }; > }; > uint8_t bus; > }; > -- > 2.20.1 (Apple Git-117) > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel
>>> On 06.06.19 at 11:50, <Paul.Durrant@citrix.com> wrote: >> -----Original Message----- >> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf Of > Roger Pau Monne >> Sent: 06 June 2019 10:02 >> To: xen-devel@lists.xenproject.org >> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Konrad > Rzeszutek Wilk >> <konrad.wilk@oracle.com>; George Dunlap <George.Dunlap@citrix.com>; Andrew > Cooper >> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim (Xen.org) > <tim@xen.org>; Julien >> Grall <julien.grall@arm.com>; Jan Beulich <jbeulich@suse.com>; Roger Pau Monne > <roger.pau@citrix.com> >> Subject: [Xen-devel] [PATCH v2 01/12] pci: introduce a devfn field to > pci_sbdf_t >> >> This is equivalent to the current extfunc field in term of contents. >> >> Switch the two current users of extfunc to use devfn instead for >> correctness. >> >> No functional change. >> >> Requested-by: Jan Beulich <jbeulich@suse.com> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> --- >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >> Cc: George Dunlap <George.Dunlap@eu.citrix.com> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >> Cc: Jan Beulich <jbeulich@suse.com> >> Cc: Julien Grall <julien.grall@arm.com> >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> Cc: Tim Deegan <tim@xen.org> >> Cc: Wei Liu <wl@xen.org> >> --- >> Changes since v1: >> - New in this version. >> --- >> NB: Paul suggested to name the function field fn instead of func, so >> that it would match the naming of the devfn field. Sadly the func >> field cannot be aliased to another field using a union because it's a >> bit field, so the only option is to rename func to fn. > > Is that true? Can you not do something like... > > union { > struct { > uint8_t func : 3, > dev : 5; > }; > struct { > uint8_t fn : 3, > pad : 5; And the "pad" field here wouldn't really be necessary. Is there a reason "func" needs to be kept? If so, is there a plan to phase out its use? If so, perhaps fn and dev should be grouped together, and func should become the (temporary) alias? Jan
On Thu, Jun 06, 2019 at 11:50:06AM +0200, Paul Durrant wrote: > > -----Original Message----- > > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf Of Roger Pau Monne > > Sent: 06 June 2019 10:02 > > To: xen-devel@lists.xenproject.org > > Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Konrad Rzeszutek Wilk > > <konrad.wilk@oracle.com>; George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper > > <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>; Julien > > Grall <julien.grall@arm.com>; Jan Beulich <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com> > > Subject: [Xen-devel] [PATCH v2 01/12] pci: introduce a devfn field to pci_sbdf_t > > > > This is equivalent to the current extfunc field in term of contents. > > > > Switch the two current users of extfunc to use devfn instead for > > correctness. > > > > No functional change. > > > > Requested-by: Jan Beulich <jbeulich@suse.com> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > Cc: George Dunlap <George.Dunlap@eu.citrix.com> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > Cc: Jan Beulich <jbeulich@suse.com> > > Cc: Julien Grall <julien.grall@arm.com> > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Cc: Stefano Stabellini <sstabellini@kernel.org> > > Cc: Tim Deegan <tim@xen.org> > > Cc: Wei Liu <wl@xen.org> > > --- > > Changes since v1: > > - New in this version. > > --- > > NB: Paul suggested to name the function field fn instead of func, so > > that it would match the naming of the devfn field. Sadly the func > > field cannot be aliased to another field using a union because it's a > > bit field, so the only option is to rename func to fn. > > Is that true? Can you not do something like... > > union { > struct { > uint8_t func : 3, > dev : 5; > }; > struct { > uint8_t fn : 3, > pad : 5; > }; > uint8_t extfunc; > }; > > ? I'm certainly not seeing any complaints from gcc. D'oh, right. I was so focused on aliasing the func field only that it didn't occur to me. I could even do: union { struct { uint8_t func : 3, dev : 5; }; struct { uint8_t fn : 3, : 5; }; uint8_t extfunc, devfn; }; Let me make sure this works with all compilers and I will send an updated version of this patch. Thanks, Roger.
On Thu, Jun 06, 2019 at 04:09:31AM -0600, Jan Beulich wrote: > >>> On 06.06.19 at 11:50, <Paul.Durrant@citrix.com> wrote: > >> -----Original Message----- > >> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf Of > > Roger Pau Monne > >> Sent: 06 June 2019 10:02 > >> To: xen-devel@lists.xenproject.org > >> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Konrad > > Rzeszutek Wilk > >> <konrad.wilk@oracle.com>; George Dunlap <George.Dunlap@citrix.com>; Andrew > > Cooper > >> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim (Xen.org) > > <tim@xen.org>; Julien > >> Grall <julien.grall@arm.com>; Jan Beulich <jbeulich@suse.com>; Roger Pau Monne > > <roger.pau@citrix.com> > >> Subject: [Xen-devel] [PATCH v2 01/12] pci: introduce a devfn field to > > pci_sbdf_t > >> > >> This is equivalent to the current extfunc field in term of contents. > >> > >> Switch the two current users of extfunc to use devfn instead for > >> correctness. > >> > >> No functional change. > >> > >> Requested-by: Jan Beulich <jbeulich@suse.com> > >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > >> --- > >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> > >> Cc: George Dunlap <George.Dunlap@eu.citrix.com> > >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> > >> Cc: Jan Beulich <jbeulich@suse.com> > >> Cc: Julien Grall <julien.grall@arm.com> > >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > >> Cc: Stefano Stabellini <sstabellini@kernel.org> > >> Cc: Tim Deegan <tim@xen.org> > >> Cc: Wei Liu <wl@xen.org> > >> --- > >> Changes since v1: > >> - New in this version. > >> --- > >> NB: Paul suggested to name the function field fn instead of func, so > >> that it would match the naming of the devfn field. Sadly the func > >> field cannot be aliased to another field using a union because it's a > >> bit field, so the only option is to rename func to fn. > > > > Is that true? Can you not do something like... > > > > union { > > struct { > > uint8_t func : 3, > > dev : 5; > > }; > > struct { > > uint8_t fn : 3, > > pad : 5; > > And the "pad" field here wouldn't really be necessary. > > Is there a reason "func" needs to be kept? If so, is there a plan to > phase out its use? If so, perhaps fn and dev should be grouped > together, and func should become the (temporary) alias? I think I can prepare a pre-patch to rename func to fn, the users of pci_sbdf_t are very limited at this point. If you agree with this I will add such a patch at the beginning of the series. Thanks, Roger.
>>> On 06.06.19 at 12:13, <roger.pau@citrix.com> wrote: > On Thu, Jun 06, 2019 at 04:09:31AM -0600, Jan Beulich wrote: >> >>> On 06.06.19 at 11:50, <Paul.Durrant@citrix.com> wrote: >> >> -----Original Message----- >> >> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf Of >> > Roger Pau Monne >> >> Sent: 06 June 2019 10:02 >> >> To: xen-devel@lists.xenproject.org >> >> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Konrad >> > Rzeszutek Wilk >> >> <konrad.wilk@oracle.com>; George Dunlap <George.Dunlap@citrix.com>; Andrew >> > Cooper >> >> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim > (Xen.org) >> > <tim@xen.org>; Julien >> >> Grall <julien.grall@arm.com>; Jan Beulich <jbeulich@suse.com>; Roger Pau Monne >> > <roger.pau@citrix.com> >> >> Subject: [Xen-devel] [PATCH v2 01/12] pci: introduce a devfn field to >> > pci_sbdf_t >> >> >> >> This is equivalent to the current extfunc field in term of contents. >> >> >> >> Switch the two current users of extfunc to use devfn instead for >> >> correctness. >> >> >> >> No functional change. >> >> >> >> Requested-by: Jan Beulich <jbeulich@suse.com> >> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> >> --- >> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >> >> Cc: George Dunlap <George.Dunlap@eu.citrix.com> >> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >> >> Cc: Jan Beulich <jbeulich@suse.com> >> >> Cc: Julien Grall <julien.grall@arm.com> >> >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> >> Cc: Tim Deegan <tim@xen.org> >> >> Cc: Wei Liu <wl@xen.org> >> >> --- >> >> Changes since v1: >> >> - New in this version. >> >> --- >> >> NB: Paul suggested to name the function field fn instead of func, so >> >> that it would match the naming of the devfn field. Sadly the func >> >> field cannot be aliased to another field using a union because it's a >> >> bit field, so the only option is to rename func to fn. >> > >> > Is that true? Can you not do something like... >> > >> > union { >> > struct { >> > uint8_t func : 3, >> > dev : 5; >> > }; >> > struct { >> > uint8_t fn : 3, >> > pad : 5; >> >> And the "pad" field here wouldn't really be necessary. >> >> Is there a reason "func" needs to be kept? If so, is there a plan to >> phase out its use? If so, perhaps fn and dev should be grouped >> together, and func should become the (temporary) alias? > > I think I can prepare a pre-patch to rename func to fn, the users of > pci_sbdf_t are very limited at this point. If you agree with this I > will add such a patch at the beginning of the series. Well, I'm okay with either, as each has it's up and down sides: "fn" is more consistent with "devfn", but "func" fits better with PCI_FUNC() (which is already not really fitting with PCI_DEVFN(), just like PCI_SLOT() isn't). Therefore I wouldn't object to sticking to func, but since Paul would prefer it to become fn, I'm also okay with that. Of course just a single, consistently used name for the field as the final result of the series would be very desirable. Jan
On Thu, Jun 06, 2019 at 04:22:54AM -0600, Jan Beulich wrote: > >>> On 06.06.19 at 12:13, <roger.pau@citrix.com> wrote: > > On Thu, Jun 06, 2019 at 04:09:31AM -0600, Jan Beulich wrote: > >> >>> On 06.06.19 at 11:50, <Paul.Durrant@citrix.com> wrote: > >> >> -----Original Message----- > >> >> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf Of > >> > Roger Pau Monne > >> >> Sent: 06 June 2019 10:02 > >> >> To: xen-devel@lists.xenproject.org > >> >> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Konrad > >> > Rzeszutek Wilk > >> >> <konrad.wilk@oracle.com>; George Dunlap <George.Dunlap@citrix.com>; Andrew > >> > Cooper > >> >> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim > > (Xen.org) > >> > <tim@xen.org>; Julien > >> >> Grall <julien.grall@arm.com>; Jan Beulich <jbeulich@suse.com>; Roger Pau Monne > >> > <roger.pau@citrix.com> > >> >> Subject: [Xen-devel] [PATCH v2 01/12] pci: introduce a devfn field to > >> > pci_sbdf_t > >> >> > >> >> This is equivalent to the current extfunc field in term of contents. > >> >> > >> >> Switch the two current users of extfunc to use devfn instead for > >> >> correctness. > >> >> > >> >> No functional change. > >> >> > >> >> Requested-by: Jan Beulich <jbeulich@suse.com> > >> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > >> >> --- > >> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> > >> >> Cc: George Dunlap <George.Dunlap@eu.citrix.com> > >> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> > >> >> Cc: Jan Beulich <jbeulich@suse.com> > >> >> Cc: Julien Grall <julien.grall@arm.com> > >> >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > >> >> Cc: Stefano Stabellini <sstabellini@kernel.org> > >> >> Cc: Tim Deegan <tim@xen.org> > >> >> Cc: Wei Liu <wl@xen.org> > >> >> --- > >> >> Changes since v1: > >> >> - New in this version. > >> >> --- > >> >> NB: Paul suggested to name the function field fn instead of func, so > >> >> that it would match the naming of the devfn field. Sadly the func > >> >> field cannot be aliased to another field using a union because it's a > >> >> bit field, so the only option is to rename func to fn. > >> > > >> > Is that true? Can you not do something like... > >> > > >> > union { > >> > struct { > >> > uint8_t func : 3, > >> > dev : 5; > >> > }; > >> > struct { > >> > uint8_t fn : 3, > >> > pad : 5; > >> > >> And the "pad" field here wouldn't really be necessary. > >> > >> Is there a reason "func" needs to be kept? If so, is there a plan to > >> phase out its use? If so, perhaps fn and dev should be grouped > >> together, and func should become the (temporary) alias? > > > > I think I can prepare a pre-patch to rename func to fn, the users of > > pci_sbdf_t are very limited at this point. If you agree with this I > > will add such a patch at the beginning of the series. > > Well, I'm okay with either, as each has it's up and down sides: > "fn" is more consistent with "devfn", but "func" fits better with > PCI_FUNC() (which is already not really fitting with PCI_DEVFN(), > just like PCI_SLOT() isn't). > > Therefore I wouldn't object to sticking to func, but since Paul > would prefer it to become fn, I'm also okay with that. Of course > just a single, consistently used name for the field as the final > result of the series would be very desirable. I'm fine with fn also. Then let me prepare v3. Thanks, Roger.
>>> On 06.06.19 at 11:01, <roger.pau@citrix.com> wrote: > --- a/xen/include/xen/pci.h > +++ b/xen/include/xen/pci.h > @@ -49,7 +49,10 @@ typedef union { > uint8_t func : 3, > dev : 5; > }; > - uint8_t extfunc; > + union { > + uint8_t extfunc, > + devfn; Would mind also switching these two around, seeing that "devfn" is the far more often used/needed/wanted name of the field? Jan
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c index 82607bdb9a..4f1f95ab69 100644 --- a/xen/drivers/vpci/vpci.c +++ b/xen/drivers/vpci/vpci.c @@ -327,7 +327,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size) } /* Find the PCI dev matching the address. */ - pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.extfunc); + pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn); if ( !pdev ) return vpci_read_hw(sbdf, reg, size); @@ -432,7 +432,7 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size, * Find the PCI dev matching the address. * Passthrough everything that's not trapped. */ - pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.extfunc); + pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn); if ( !pdev ) { vpci_write_hw(sbdf, reg, size, data); diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h index 8b21e8dc84..ec98274675 100644 --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -49,7 +49,10 @@ typedef union { uint8_t func : 3, dev : 5; }; - uint8_t extfunc; + union { + uint8_t extfunc, + devfn; + }; }; uint8_t bus; };
This is equivalent to the current extfunc field in term of contents. Switch the two current users of extfunc to use devfn instead for correctness. No functional change. Requested-by: Jan Beulich <jbeulich@suse.com> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Julien Grall <julien.grall@arm.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Tim Deegan <tim@xen.org> Cc: Wei Liu <wl@xen.org> --- Changes since v1: - New in this version. --- NB: Paul suggested to name the function field fn instead of func, so that it would match the naming of the devfn field. Sadly the func field cannot be aliased to another field using a union because it's a bit field, so the only option is to rename func to fn. I don't have a strong opinion, but if there's consensus it should be done after this patch IMO and not later in the series, as more occurrences of sbdf.func will appear. --- xen/drivers/vpci/vpci.c | 4 ++-- xen/include/xen/pci.h | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-)