From: "Jan Beulich" <JBeulich@suse.com> To: Roger Pau Monne <roger.pau@citrix.com> Cc: Kevin Tian <kevin.tian@intel.com>, Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wei.liu2@citrix.com>, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, George Dunlap <George.Dunlap@eu.citrix.com>, Andrew Cooper <andrew.cooper3@citrix.com>, Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>, Julien Grall <julien.grall@arm.com>, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>, xen-devel <xen-devel@lists.xenproject.org>, Brian Woods <brian.woods@amd.com> Subject: Re: [PATCH 3/5] pci: switch pci_conf_{read/write} to use pci_sbdf_t Date: Fri, 24 May 2019 04:01:23 -0600 [thread overview] Message-ID: <5CE7C0F30200007800231EEB@prv1-mh.provo.novell.com> (raw) In-Reply-To: <20190510161056.48648-4-roger.pau@citrix.com> >>> On 10.05.19 at 18:10, <roger.pau@citrix.com> 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@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
WARNING: multiple messages have this Message-ID (diff)
From: "Jan Beulich" <JBeulich@suse.com> To: "Roger Pau Monne" <roger.pau@citrix.com> Cc: Kevin Tian <kevin.tian@intel.com>, Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wei.liu2@citrix.com>, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, George Dunlap <George.Dunlap@eu.citrix.com>, Andrew Cooper <andrew.cooper3@citrix.com>, Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>, Julien Grall <julien.grall@arm.com>, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>, xen-devel <xen-devel@lists.xenproject.org>, Brian Woods <brian.woods@amd.com> Subject: Re: [Xen-devel] [PATCH 3/5] pci: switch pci_conf_{read/write} to use pci_sbdf_t Date: Fri, 24 May 2019 04:01:23 -0600 [thread overview] Message-ID: <5CE7C0F30200007800231EEB@prv1-mh.provo.novell.com> (raw) Message-ID: <20190524100123.-lRM44QioXVjDBzrH-jBoAF4wxyetVTWpmV3qQfDlo0@z> (raw) In-Reply-To: <20190510161056.48648-4-roger.pau@citrix.com> >>> On 10.05.19 at 18:10, <roger.pau@citrix.com> 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@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2019-05-24 10:01 UTC|newest] Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-05-10 16:10 [PATCH 0/5] pci: expand usage of pci_sbdf_t Roger Pau Monne 2019-05-10 16:10 ` [Xen-devel] " Roger Pau Monne 2019-05-10 16:10 ` [PATCH 1/5] pci: use pci_sbdf_t in pci_dev Roger Pau Monne 2019-05-10 16:10 ` [Xen-devel] " Roger Pau Monne 2019-05-10 16:16 ` Andrew Cooper 2019-05-10 16:16 ` [Xen-devel] " Andrew Cooper 2019-05-13 6:25 ` Jan Beulich 2019-05-13 6:25 ` [Xen-devel] " Jan Beulich 2019-05-13 7:53 ` Roger Pau Monné 2019-05-13 7:53 ` [Xen-devel] " Roger Pau Monné 2019-05-23 15:29 ` Jan Beulich 2019-05-23 15:29 ` [Xen-devel] " Jan Beulich 2019-05-27 15:51 ` Roger Pau Monné 2019-05-27 15:51 ` [Xen-devel] " Roger Pau Monné 2019-05-10 16:10 ` [PATCH 2/5] pci: use function generation macros for pci_config_{write, read}<size> Roger Pau Monne 2019-05-10 16:10 ` [Xen-devel] " Roger Pau Monne 2019-05-24 9:10 ` Jan Beulich 2019-05-24 9:10 ` [Xen-devel] " Jan Beulich 2019-05-24 9:29 ` Andrew Cooper 2019-05-24 9:29 ` [Xen-devel] " Andrew Cooper 2019-05-27 16:08 ` Roger Pau Monné 2019-05-27 16:08 ` [Xen-devel] " Roger Pau Monné 2019-05-28 8:54 ` Jan Beulich 2019-05-28 8:54 ` [Xen-devel] " Jan Beulich 2019-05-10 16:10 ` [PATCH 3/5] pci: switch pci_conf_{read/write} to use pci_sbdf_t Roger Pau Monne 2019-05-10 16:10 ` [Xen-devel] " Roger Pau Monne 2019-05-24 9:40 ` Andrew Cooper 2019-05-24 9:40 ` [Xen-devel] " Andrew Cooper 2019-05-24 10:01 ` Jan Beulich [this message] 2019-05-24 10:01 ` Jan Beulich 2019-05-27 16:44 ` Roger Pau Monné 2019-05-27 16:44 ` [Xen-devel] " Roger Pau Monné 2019-05-28 8:51 ` Jan Beulich 2019-05-28 8:51 ` [Xen-devel] " Jan Beulich 2019-05-28 10:05 ` Roger Pau Monné 2019-05-28 10:05 ` [Xen-devel] " Roger Pau Monné 2019-05-28 10:38 ` Jan Beulich 2019-05-28 10:38 ` [Xen-devel] " Jan Beulich 2019-05-10 16:10 ` [PATCH 4/5] print: introduce a format specifier for pci_sbdf_t Roger Pau Monne 2019-05-10 16:10 ` [Xen-devel] " Roger Pau Monne 2019-05-24 10:36 ` Jan Beulich 2019-05-24 10:36 ` [Xen-devel] " Jan Beulich 2019-05-24 10:59 ` Andrew Cooper 2019-05-24 10:59 ` [Xen-devel] " Andrew Cooper 2019-05-24 11:16 ` Jan Beulich 2019-05-24 11:16 ` [Xen-devel] " Jan Beulich 2019-05-27 15:48 ` Roger Pau Monné 2019-05-27 15:48 ` [Xen-devel] " Roger Pau Monné 2019-05-27 15:58 ` Jan Beulich 2019-05-27 15:58 ` [Xen-devel] " Jan Beulich 2019-05-10 16:10 ` [PATCH 5/5] pci: switch PCI capabilities related functions to use pci_sbdf_t Roger Pau Monne 2019-05-10 16:10 ` [Xen-devel] " Roger Pau Monne 2019-05-24 10:52 ` Jan Beulich 2019-05-24 10:52 ` [Xen-devel] " Jan Beulich
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=5CE7C0F30200007800231EEB@prv1-mh.provo.novell.com \ --to=jbeulich@suse.com \ --cc=George.Dunlap@eu.citrix.com \ --cc=Ian.Jackson@eu.citrix.com \ --cc=andrew.cooper3@citrix.com \ --cc=brian.woods@amd.com \ --cc=julien.grall@arm.com \ --cc=kevin.tian@intel.com \ --cc=konrad.wilk@oracle.com \ --cc=roger.pau@citrix.com \ --cc=sstabellini@kernel.org \ --cc=suravee.suthikulpanit@amd.com \ --cc=tim@xen.org \ --cc=wei.liu2@citrix.com \ --cc=xen-devel@lists.xenproject.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.