Re: [Xen-devel] [PATCH] pci: constify domain parameter of pci_get_pdev_by_domain

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Sep 08, 2017 at 10:00:41AM -0600, Jan Beulich wrote:
> >>> On 08.09.17 at 16:30, <roger.pau@xxxxxxxxxx> wrote:
> > On Fri, Sep 08, 2017 at 03:15:40PM +0100, Andrew Cooper wrote:
> >> On 08/09/17 14:34, Roger Pau Monne wrote:
> >> > While there fix the indentation.
> >> >
> >> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >> > ---
> >> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> >> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> > Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> >> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> >> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> >> > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> >> > Cc: Tim Deegan <tim@xxxxxxx>
> >> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> >> > ---
> >> >  xen/drivers/passthrough/pci.c | 4 ++--
> >> >  xen/include/xen/pci.h         | 4 ++--
> >> >  2 files changed, 4 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/xen/drivers/passthrough/pci.c 
> >> > b/xen/drivers/passthrough/pci.c
> >> > index 74829e5748..469dfc6c3d 100644
> >> > --- a/xen/drivers/passthrough/pci.c
> >> > +++ b/xen/drivers/passthrough/pci.c
> >> > @@ -532,8 +532,8 @@ struct pci_dev *pci_get_real_pdev(int seg, int bus, 
> >> > int 
> > devfn)
> >> >      return pdev;
> >> >  }
> >> >  
> >> > -struct pci_dev *pci_get_pdev_by_domain(
> >> > -    struct domain *d, int seg, int bus, int devfn)
> >> > +struct pci_dev *pci_get_pdev_by_domain(const struct domain *d, int seg,
> >> > +                                       int bus, int devfn)
> >> 
> >> I know this isn't strictly related to the patch, but having 3 register
> >> parameters (and load of places elsewhere) here is extremely wasteful for
> >> register scheduling.  Could we introduce:
> >> 
> >> typedef union {
> >>     uint32_t sbdf;
> >>     struct {
> >>         uint16_t  f : 3,  /* Function */
> >>                   d : 5,  /* Device   */
> >>                   b : 8;  /* Bus      */
> >>         uint16_t s; /* Segment  */
> >>     };
> >> } pci_sbdf_t;
> >> 
> >> and try to start using it?
> > 
> > This is going to be kind of awkward to use with extended
> > functions...
> > 
> > It's not like what we have now (devfn) is much better, but if we
> > switch to something else I would like to be able to get the correct
> > function value for both normal and extended function devices from the
> > same field.
> 
> Why not unionize f and d with a uint8_t extfn (I'd also prefer the
> other two to be named func and dev)?

So something like:

typedef union {
    uint32_t sbdf;
    struct {
        union {
            struct {
                uint8_t  func : 3, /* Function */
                         slot : 5; /* Device   */
            };
            uint8_t      extfunc;  /* Extended function */
        };
        uint8_t          bus;      /* Bus      */
        uint16_t         seg;      /* Segment  */
    };
} pci_sbdf_t;

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel