Re: [Xen-devel] [PATCH v2 01/12] pci: introduce a devfn field to pci_sbdf_t

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf Of 
> Roger Pau Monne
> Sent: 06 June 2019 10:02
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Konrad 
> Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>; Andrew 
> Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Tim 
> (Xen.org) <tim@xxxxxxx>; Julien
> Grall <julien.grall@xxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Roger Pau 
> Monne <roger.pau@xxxxxxxxxx>
> 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@xxxxxxxx>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Wei Liu <wl@xxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel