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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 10/05/2019 17:10, Roger Pau Monne wrote:
> pci_dev already uses pci_sbdf_t, so propagate the usage of the type to
> pci_conf functions in order to shorten the calls when made from a
> pci_dev struct.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.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: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Brian Woods <brian.woods@amd.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> ---
>  xen/arch/x86/cpu/amd.c                     |  27 ++--
>  xen/arch/x86/dmi_scan.c                    |   9 +-
>  xen/arch/x86/mm.c                          |   2 +-
>  xen/arch/x86/msi.c                         | 177 +++++++++------------
>  xen/arch/x86/oprofile/op_model_athlon.c    |  12 +-
>  xen/arch/x86/x86_64/mmconf-fam10h.c        |  13 +-
>  xen/arch/x86/x86_64/mmconfig-shared.c      |  26 +--
>  xen/arch/x86/x86_64/pci.c                  |  32 ++--
>  xen/drivers/acpi/reboot.c                  |   8 +-
>  xen/drivers/char/ehci-dbgp.c               |  75 +++++----
>  xen/drivers/char/ns16550.c                 |  80 +++++-----
>  xen/drivers/passthrough/amd/iommu_detect.c |   3 +-
>  xen/drivers/passthrough/amd/iommu_init.c   |  26 +--
>  xen/drivers/passthrough/ats.h              |   4 +-
>  xen/drivers/passthrough/pci.c              | 106 +++++-------
>  xen/drivers/passthrough/vtd/dmar.c         |  18 ++-
>  xen/drivers/passthrough/vtd/quirks.c       |  69 ++++----
>  xen/drivers/passthrough/x86/ats.c          |  15 +-
>  xen/drivers/pci/pci.c                      |  43 +++--
>  xen/drivers/video/vga.c                    |  21 +--
>  xen/drivers/vpci/header.c                  |  53 ++----
>  xen/drivers/vpci/msi.c                     |   6 +-
>  xen/drivers/vpci/msix.c                    |  12 +-
>  xen/drivers/vpci/vpci.c                    |  42 ++---
>  xen/include/xen/pci.h                      |  29 ++--
>  25 files changed, 444 insertions(+), 464 deletions(-)
>
> diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
> index e19a5ead3e..014d88925c 100644
> --- 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;

This looks suspiciously like it wants to use MASK_EXTR()

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

What is wrong with something like:

pci_sbdf_t pci = PCI_SBDF_T(0, 0, 0x18 + node, 3);

IMO, the resulting code would be more logical to read as
pci_conf_read8(pci, ...) (or perhaps dev?).  sbdf it a little awkward.

> +
>  		/* PMM7: bus=0, dev=0x18+node, function=0x3, register=0x87. */
> -		pmm7 = pci_conf_read8(0, 0, 0x18+node, 0x3, 0x87);
> +		pmm7 = pci_conf_read8(sbdf, 0x87);
>  		/* Invalid read means we've updated every Northbridge. */
>  		if (pmm7 == 0xFF)
>  			break;
>  		pmm7 &= 0xFC; /* clear pmm7[1:0] */
> -		pci_conf_write8(0, 0, 0x18+node, 0x3, 0x87, pmm7);
> +		pci_conf_write8(sbdf, 0x87, pmm7);
>  		printk ("AMD: Disabling C1 Clock Ramping Node #%x\n", node);
>  	}
>  }
> @@ -696,8 +702,13 @@ static void init_amd(struct cpuinfo_x86 *c)
>  
>  	if (c->x86 == 0x16 && c->x86_model <= 0xf) {
>  		if (c == &boot_cpu_data) {
> -			l = pci_conf_read32(0, 0, 0x18, 0x3, 0x58);
> -			h = pci_conf_read32(0, 0, 0x18, 0x3, 0x5c);
> +			const pci_sbdf_t sbdf = {
> +				.dev = 0x18,
> +				.func = 0x3,
> +			};
> +
> +			l = pci_conf_read32(sbdf, 0x58);
> +			h = pci_conf_read32(sbdf, 0x5c);
>  			if ((l & 0x1f) | (h & 0x1))
>  				printk(KERN_WARNING
>  				       "Applying workaround for erratum 792: %s%s%s\n",
> @@ -706,12 +717,10 @@ static void init_amd(struct cpuinfo_x86 *c)
>  				       (h & 0x1) ? "clearing D18F3x5C[0]" : "");
>  
>  			if (l & 0x1f)
> -				pci_conf_write32(0, 0, 0x18, 0x3, 0x58,
> -						 l & ~0x1f);
> +				pci_conf_write32(sbdf, 0x58, l & ~0x1f);
>  
>  			if (h & 0x1)
> -				pci_conf_write32(0, 0, 0x18, 0x3, 0x5c,
> -						 h & ~0x1);
> +				pci_conf_write32(sbdf, 0x5c, h & ~0x1);
>  		}
>  
>  		rdmsrl(MSR_AMD64_LS_CFG, value);
> diff --git a/xen/arch/x86/dmi_scan.c b/xen/arch/x86/dmi_scan.c
> index fcdf2d3952..59557fa57b 100644
> --- a/xen/arch/x86/dmi_scan.c
> +++ b/xen/arch/x86/dmi_scan.c
> @@ -468,16 +468,19 @@ static __init int broken_toshiba_keyboard(struct dmi_blacklist *d)
>  static int __init ich10_bios_quirk(struct dmi_system_id *d)
>  {
>      u32 port, smictl;
> +    const pci_sbdf_t sbdf = {
> +	.dev = 0x1f,
> +    };
>  
> -    if ( pci_conf_read16(0, 0, 0x1f, 0, PCI_VENDOR_ID) != 0x8086 )
> +    if ( pci_conf_read16(sbdf, PCI_VENDOR_ID) != 0x8086 )
>          return 0;
>  
> -    switch ( pci_conf_read16(0, 0, 0x1f, 0, PCI_DEVICE_ID) ) {
> +    switch ( pci_conf_read16(sbdf, PCI_DEVICE_ID) ) {
>      case 0x3a14:
>      case 0x3a16:
>      case 0x3a18:
>      case 0x3a1a:
> -        port = (pci_conf_read16(0, 0, 0x1f, 0, 0x40) & 0xff80) + 0x30;
> +        port = (pci_conf_read16(sbdf, 0x40) & 0xff80) + 0x30;
>          smictl = inl(port);
>          /* turn off LEGACY_USB{,2}_EN if enabled */
>          if ( smictl & 0x20008 )
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 45fadbab61..37d8141ed2 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5984,7 +5984,7 @@ const struct platform_bad_page *__init get_platform_badpages(unsigned int *array
>      }
>  
>      *array_size = ARRAY_SIZE(snb_bad_pages);
> -    igd_id = pci_conf_read32(0, 0, 2, 0, 0);
> +    igd_id = pci_conf_read32(PCI_SBDF_T(0, 0, 2, 0), 0);
>      if ( IS_SNB_GFX(igd_id) )
>          return snb_bad_pages;
>  
> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index f30f592ee2..ad4a72d56b 100644
> --- 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);

Can drop the !! and brackets.

~Andrew

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