[Xen-devel] [PATCH 2/5] pci: use function generation macros for pci_config_{write, read}<size>

Roger Pau Monne posted 5 patches 1 year, 11 months ago

[Xen-devel] [PATCH 2/5] pci: use function generation macros for pci_config_{write, read}<size>

Posted by Roger Pau Monne 1 year, 11 months ago
This avoids code duplication between the helpers.

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>
---
 xen/arch/x86/x86_64/pci.c | 140 ++++++++++++++++----------------------
 1 file changed, 57 insertions(+), 83 deletions(-)

diff --git a/xen/arch/x86/x86_64/pci.c b/xen/arch/x86/x86_64/pci.c
index 6e3f5cf203..4f77beb119 100644
--- a/xen/arch/x86/x86_64/pci.c
+++ b/xen/arch/x86/x86_64/pci.c
@@ -11,95 +11,69 @@
 #define PCI_CONF_ADDRESS(bus, dev, func, reg) \
     (0x80000000 | (bus << 16) | (dev << 11) | (func << 8) | (reg & ~3))
 
-uint8_t pci_conf_read8(
-    unsigned int seg, unsigned int bus, unsigned int dev, unsigned int func,
-    unsigned int reg)
-{
-    u32 value;
-
-    if ( seg || reg > 255 )
-    {
-        pci_mmcfg_read(seg, bus, PCI_DEVFN(dev, func), reg, 1, &value);
-        return value;
-    }
-    else
-    {
-        BUG_ON((bus > 255) || (dev > 31) || (func > 7));
-        return pci_conf_read(PCI_CONF_ADDRESS(bus, dev, func, reg), reg & 3, 1);
+#define GEN_PCI_CONF_READ(s)                                                   \
+    uint ## s ## _t pci_conf_read ## s (unsigned int seg, unsigned int bus,    \
+                                        unsigned int dev, unsigned int func,   \
+                                        unsigned int reg)                      \
+    {                                                                          \
+        uint32_t value;                                                        \
+                                                                               \
+        BUILD_BUG_ON(s != 8 && s != 16 && s != 32);                            \
+        if ( seg || reg > 255 )                                                \
+            pci_mmcfg_read(seg, bus, PCI_DEVFN(dev, func), reg, s / 8, &value);\
+        else                                                                   \
+        {                                                                      \
+            BUG_ON((bus > 255) || (dev > 31) || (func > 7));                   \
+            value = pci_conf_read(PCI_CONF_ADDRESS(bus, dev, func, reg),       \
+                                  reg & (4 - s / 8), s / 8);                   \
+        }                                                                      \
+                                                                               \
+        return value;                                                          \
     }
-}
 
-uint16_t pci_conf_read16(
-    unsigned int seg, unsigned int bus, unsigned int dev, unsigned int func,
-    unsigned int reg)
-{
-    u32 value;
+/* Grep fodder */
+#define pci_conf_read8
+#define pci_conf_read16
+#define pci_conf_read32
 
-    if ( seg || reg > 255 )
-    {
-        pci_mmcfg_read(seg, bus, PCI_DEVFN(dev, func), reg, 2, &value);
-        return value;
-    }
-    else
-    {
-        BUG_ON((bus > 255) || (dev > 31) || (func > 7));
-        return pci_conf_read(PCI_CONF_ADDRESS(bus, dev, func, reg), reg & 2, 2);
-    }
-}
+#undef pci_conf_read8
+#undef pci_conf_read16
+#undef pci_conf_read32
 
-uint32_t pci_conf_read32(
-    unsigned int seg, unsigned int bus, unsigned int dev, unsigned int func,
-    unsigned int reg)
-{
-    u32 value;
+GEN_PCI_CONF_READ(8)
+GEN_PCI_CONF_READ(16)
+GEN_PCI_CONF_READ(32)
 
-    if ( seg || reg > 255 )
-    {
-        pci_mmcfg_read(seg, bus, PCI_DEVFN(dev, func), reg, 4, &value);
-        return value;
-    }
-    else
-    {
-        BUG_ON((bus > 255) || (dev > 31) || (func > 7));
-        return pci_conf_read(PCI_CONF_ADDRESS(bus, dev, func, reg), 0, 4);
-    }
-}
+#undef GEN_PCI_CONF_READ
 
-void pci_conf_write8(
-    unsigned int seg, unsigned int bus, unsigned int dev, unsigned int func,
-    unsigned int reg, uint8_t data)
-{
-    if ( seg || reg > 255 )
-        pci_mmcfg_write(seg, bus, PCI_DEVFN(dev, func), reg, 1, data);
-    else
-    {
-        BUG_ON((bus > 255) || (dev > 31) || (func > 7));
-        pci_conf_write(PCI_CONF_ADDRESS(bus, dev, func, reg), reg & 3, 1, data);
+#define GEN_PCI_CONF_WRITE(s)                                                  \
+    void pci_conf_write ## s (unsigned int seg, unsigned int bus,              \
+                              unsigned int dev, unsigned int func,             \
+                              unsigned int reg, uint ## s ## _t data)          \
+    {                                                                          \
+        BUILD_BUG_ON(s != 8 && s != 16 && s != 32);                            \
+        if ( seg || reg > 255 )                                                \
+            pci_mmcfg_write(seg, bus, PCI_DEVFN(dev, func), reg, s / 8, data); \
+        else                                                                   \
+        {                                                                      \
+            BUG_ON((bus > 255) || (dev > 31) || (func > 7));                   \
+            pci_conf_write(PCI_CONF_ADDRESS(bus, dev, func, reg),              \
+                           reg & (4 - s / 8), s / 8, data);                    \
+        }                                                                      \
     }
-}
 
-void pci_conf_write16(
-    unsigned int seg, unsigned int bus, unsigned int dev, unsigned int func,
-    unsigned int reg, uint16_t data)
-{
-    if ( seg || reg > 255 )
-        pci_mmcfg_write(seg, bus, PCI_DEVFN(dev, func), reg, 2, data);
-    else
-    {
-        BUG_ON((bus > 255) || (dev > 31) || (func > 7));
-        pci_conf_write(PCI_CONF_ADDRESS(bus, dev, func, reg), reg & 2, 2, data);
-    }
-}
+/* Grep fodder */
+#define pci_conf_write8
+#define pci_conf_write16
+#define pci_conf_write32
+
+#undef pci_conf_write8
+#undef pci_conf_write16
+#undef pci_conf_write32
+
+GEN_PCI_CONF_WRITE(8)
+GEN_PCI_CONF_WRITE(16)
+GEN_PCI_CONF_WRITE(32)
+
+#undef GEN_PCI_CONF_WRITE
 
-void pci_conf_write32(
-    unsigned int seg, unsigned int bus, unsigned int dev, unsigned int func,
-    unsigned int reg, uint32_t data)
-{
-    if ( seg || reg > 255 )
-        pci_mmcfg_write(seg, bus, PCI_DEVFN(dev, func), reg, 4, data);
-    else
-    {
-        BUG_ON((bus > 255) || (dev > 31) || (func > 7));
-        pci_conf_write(PCI_CONF_ADDRESS(bus, dev, func, reg), 0, 4, data);
-    }
-}
-- 
2.17.2 (Apple Git-113)


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

Re: [Xen-devel] [PATCH 2/5] pci: use function generation macros for pci_config_{write, read}<size>

Posted by Andrew Cooper 1 year, 10 months ago
On 10/05/2019 17:10, Roger Pau Monne wrote:
> This avoids code duplication between the helpers.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

-1.  I see this as actively making the code worse, not an improvement.

~Andrew

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

Re: [Xen-devel] [PATCH 2/5] pci: use function generation macros for pci_config_{write, read}<size>

Posted by Roger Pau Monné 1 year, 10 months ago
On Fri, May 24, 2019 at 10:29:26AM +0100, Andrew Cooper wrote:
> On 10/05/2019 17:10, Roger Pau Monne wrote:
> > This avoids code duplication between the helpers.
> >
> > No functional change intended.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> -1.  I see this as actively making the code worse, not an improvement.

Thanks for the feedback. I'm not specially thrilled either way (seeing
Jan provided his RB), the main motivation behind the change was to
avoid having to change the list of parameters to a pci_sbdf_t in each
helper, I find this error prone when the code is the same in all 3
different helpers except for the size difference.

Given Andrew's opinion do you still consider this useful Jan?

Roger.

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

Re: [Xen-devel] [PATCH 2/5] pci: use function generation macros for pci_config_{write, read}<size>

Posted by Jan Beulich 1 year, 10 months ago
>>> On 27.05.19 at 18:08, <roger.pau@citrix.com> wrote:
> On Fri, May 24, 2019 at 10:29:26AM +0100, Andrew Cooper wrote:
>> On 10/05/2019 17:10, Roger Pau Monne wrote:
>> > This avoids code duplication between the helpers.
>> >
>> > No functional change intended.
>> >
>> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> 
>> -1.  I see this as actively making the code worse, not an improvement.
> 
> Thanks for the feedback. I'm not specially thrilled either way (seeing
> Jan provided his RB), the main motivation behind the change was to
> avoid having to change the list of parameters to a pci_sbdf_t in each
> helper, I find this error prone when the code is the same in all 3
> different helpers except for the size difference.
> 
> Given Andrew's opinion do you still consider this useful Jan?

Well, let me put it that way: I'm fine with the change, but I'm also
fine with the code staying as is, seeing Andrew's objection.

Jan


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

Re: [Xen-devel] [PATCH 2/5] pci: use function generation macros for pci_config_{write, read}<size>

Posted by Jan Beulich 1 year, 10 months ago
>>> On 10.05.19 at 18:10, <roger.pau@citrix.com> wrote:
> This avoids code duplication between the helpers.
> 
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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