Re: [Xen-devel] [PATCH 3/5] pci: switch pci_conf_{read/write} to use pci_sbdf_t

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

On Fri, May 24, 2019 at 04:01:23AM -0600, Jan Beulich wrote:
> >>> On 10.05.19 at 18:10, <roger.pau@xxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/cpu/amd.c
> > +++ b/xen/arch/x86/cpu/amd.c
> > @@ -855,20 +859,22 @@ static void _ns16550_resume(struct serial_port *port)
> >  {
> >  #ifdef CONFIG_HAS_PCI
> >      struct ns16550 *uart = port->uart;
> > +    const pci_sbdf_t sbdf = {
> > +        .bus = uart->ps_bdf[0],
> > +        .dev = uart->ps_bdf[1],
> > +        .func = uart->ps_bdf[2],
> > +    };
> 
> In cases like this one, is there any particular reason you don't use the
> macro you introduce?

At first I preferred this explicit field based initialization because
it makes it easier to read IMO, but I will switch to use the macro.

> > --- a/xen/include/xen/pci.h
> > +++ b/xen/include/xen/pci.h
> > @@ -58,6 +58,11 @@ typedef union {
> >      };
> >  } pci_sbdf_t;
> >  
> > +#define PCI_SBDF_T(s, b, d, f) \
> > +    ((pci_sbdf_t) { .seg = (s), .bus = (b), .dev = (d), .func = (f) })
> 
> I'd prefer if the _T suffix could be omitted. Afaics there's no use of the
> existing PCI_SBDF() anywhere in the tree, so this should be fine. For
> the 2nd macro below I can't easily tell whether the few existing used
> have all disappeared by now, but it seems likely.

I can see about dropping the _T suffix, but I think there's likely
some overlap between the introduction of PCI_SBDF_T and the last user
of the current PCI_SBDF helpers, so maybe it's fine to use the _T
suffix at first and have one final patch that removes it?

> Also I'm afraid initializers of this kind will break the build with old gcc.

I thought we dropped support for such old versions of gcc, is that not
the case?

If not this is all a no-go.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel