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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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
> @@ -417,15 +417,21 @@ static void disable_c1_ramping(void)
>       int node, nr_nodes;
>  
>       /* Read the number of nodes from the first Northbridge. */
> -     nr_nodes = ((pci_conf_read32(0, 0, 0x18, 0x0, 0x60)>>4)&0x07)+1;
> +     nr_nodes = ((pci_conf_read32(PCI_SBDF_T(0, 0, 0x18, 0),
> +                                  0x60)>>4)&0x07)+1;

Could you please add the missing blanks here as you touch this anyway?

>       for (node = 0; node < nr_nodes; node++) {
> +             const pci_sbdf_t sbdf = {
> +                     .dev = 0x18  + node,
> +                     .func = 0x3

Just like you do above, dropping the unnecessary 0x from this last line
would be nice. (Same again further down.)

> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -124,29 +124,20 @@ static void msix_put_fixmap(struct arch_msix *msix, int 
> idx)
>  
>  static bool memory_decoded(const struct pci_dev *dev)
>  {
> -    uint8_t bus, slot, func;
> +    pci_sbdf_t sbdf = dev->sbdf;
>  
> -    if ( !dev->info.is_virtfn )
> +    if ( dev->info.is_virtfn )
>      {
> -        bus = dev->sbdf.bus;
> -        slot = dev->sbdf.dev;
> -        func = dev->sbdf.func;
> -    }
> -    else
> -    {
> -        bus = dev->info.physfn.bus;
> -        slot = PCI_SLOT(dev->info.physfn.devfn);
> -        func = PCI_FUNC(dev->info.physfn.devfn);
> +        sbdf.bus = dev->info.physfn.bus;
> +        sbdf.extfunc = dev->info.physfn.devfn;
>      }
>  
> -    return !!(pci_conf_read16(dev->sbdf.seg, bus, slot, func, PCI_COMMAND) &
> -              PCI_COMMAND_MEMORY);
> +    return !!(pci_conf_read16(sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY);

Take the opportunity and also drop the pointless !! (and parentheses)?

> @@ -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?

>      if ( uart->bar )
>      {

Also it looks like the variable could move into this more narrow scope.

> -       pci_conf_write32(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2],
> -                        PCI_BASE_ADDRESS_0 + uart->bar_idx*4, uart->bar);
> +       pci_conf_write32(sbdf, PCI_BASE_ADDRESS_0 + uart->bar_idx*4, 
> uart->bar);

Ideally add the missing blanks again (many more below)?

> @@ -356,10 +356,16 @@ static int __init acpi_parse_dev_scope(
>          switch ( acpi_scope->entry_type )
>          {
>          case ACPI_DMAR_SCOPE_TYPE_BRIDGE:
> -            sec_bus = pci_conf_read8(seg, bus, path->dev, path->fn,
> -                                     PCI_SECONDARY_BUS);
> -            sub_bus = pci_conf_read8(seg, bus, path->dev, path->fn,
> -                                     PCI_SUBORDINATE_BUS);
> +        {
> +            const pci_sbdf_t sbdf = {
> +                .seg = seg,
> +                .bus = bus,
> +                .dev = path->dev,
> +                .func = path->fn,
> +            };
> +
> +            sec_bus = pci_conf_read8(sbdf, PCI_SECONDARY_BUS);
> +            sub_bus = pci_conf_read8(sbdf, PCI_SUBORDINATE_BUS);
>              if ( iommu_verbose )
>                  printk(VTDPREFIX
>                         " bridge: %04x:%02x:%02x.%u start=%x sec=%x sub=%x\n",
> @@ -368,7 +374,7 @@ static int __init acpi_parse_dev_scope(
>  
>              dmar_scope_add_buses(scope, sec_bus, sub_bus);
>              break;
> -
> +        }
>          case ACPI_DMAR_SCOPE_TYPE_HPET:

Please don't lose the blank line.

> --- a/xen/drivers/passthrough/vtd/quirks.c
> +++ b/xen/drivers/passthrough/vtd/quirks.c
> @@ -61,6 +61,14 @@ static bool_t __read_mostly is_snb_gfx;
>  static u8 *__read_mostly igd_reg_va;
>  static spinlock_t igd_lock;
>  
> +static const pci_sbdf_t igd_sbdf = {
> +    .dev = IGD_DEV,
> +};
> +
> +static const pci_sbdf_t ioh_sbdf = {
> +    .dev = IOH_DEV,
> +};

There's only a single use of this, and in an __init function. On one
hand we certainly expect the compiler to not emit to .rodata here
in the first place. But then - can we rely on this? If not, this would
want to become __initconst. So on the whole I think I'd prefer if
you used the initializer macro instead, making IGD_DEV and
IOH_DEV both invocations of that macro. That's then also better
in line with uses of the macro elsewhere in this file.

> --- 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.

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

> +#define PCI_SBDF3_T(s, b, e) \
> +    ((pci_sbdf_t) { .seg = (s), .bus = (b), .extfunc = (e) })

Same remark as on the earlier patch regarding extfunc.

On the whole, again seeing the size of this patch, splitting this up would
probably have helped. At least doing reads and writes separately should
have been possible.

Jan


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