diff mbox series

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

Message ID 20190510161056.48648-3-roger.pau@citrix.com (mailing list archive)
State Superseded
Headers show
Series pci: expand usage of pci_sbdf_t | expand

Commit Message

Roger Pau Monné May 10, 2019, 4:10 p.m. UTC 
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(-)

Comments

Jan Beulich May 24, 2019, 9:10 a.m. UTC | #1 
>>> 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>
Andrew Cooper May 24, 2019, 9:29 a.m. UTC | #2 
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
Roger Pau Monné May 27, 2019, 4:08 p.m. UTC | #3 
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.
Jan Beulich May 28, 2019, 8:54 a.m. UTC | #4 
>>> 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
diff mbox series

Patch

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);
-    }
-}