Patchwork [2/7] msi: Guard msi/msix_write_config with msi_present

login
register
mail settings
Submitter Jan Kiszka
Date June 8, 2011, 10:26 a.m.
Message ID <0074e8926b2a8c93a8ef6aab9071d3e7e882a6c3.1307528800.git.jan.kiszka@siemens.com>
Download mbox | patch
Permalink /patch/99398/
State New
Headers show

Comments

Jan Kiszka - June 8, 2011, 10:26 a.m.
Terminate msi/msix_write_config early if support is not enabled. This
allows to remove checks at the caller site if MSI is optional.

CC: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/intel-hda.c |    6 +-----
 hw/msi.c       |    3 ++-
 hw/msix.c      |    2 +-
 3 files changed, 4 insertions(+), 7 deletions(-)
Gerd Hoffmann - June 8, 2011, 12:28 p.m.
> @@ -1173,12 +1173,8 @@ static int intel_hda_exit(PCIDevice *pci)
>   static void intel_hda_write_config(PCIDevice *pci, uint32_t addr,
>                                      uint32_t val, int len)
>   {
> -    IntelHDAState *d = DO_UPCAST(IntelHDAState, pci, pci);
> -
>       pci_default_write_config(pci, addr, val, len);
> -    if (d->msi) {
> -        msi_write_config(pci, addr, val, len);
> -    }
> +    msi_write_config(pci, addr, val, len);
>   }

Nothing device specific left in there now.  If msi_write_config() checks 
itself whenever msi is enabled or not we could call it from 
pci_default_write_config(), then zap this function altogether, no?

cheers,
   Gerd
Jan Kiszka - June 8, 2011, 12:33 p.m.
On 2011-06-08 14:28, Gerd Hoffmann wrote:
>> @@ -1173,12 +1173,8 @@ static int intel_hda_exit(PCIDevice *pci)
>>   static void intel_hda_write_config(PCIDevice *pci, uint32_t addr,
>>                                      uint32_t val, int len)
>>   {
>> -    IntelHDAState *d = DO_UPCAST(IntelHDAState, pci, pci);
>> -
>>       pci_default_write_config(pci, addr, val, len);
>> -    if (d->msi) {
>> -        msi_write_config(pci, addr, val, len);
>> -    }
>> +    msi_write_config(pci, addr, val, len);
>>   }
> 
> Nothing device specific left in there now.  If msi_write_config() checks 
> itself whenever msi is enabled or not we could call it from 
> pci_default_write_config(), then zap this function altogether, no?

Well, we could put those helpers (msi_write_config, msi_reset, maybe
more) into generic PCI code. Then device will only have to overload the
handlers if they do something in addition. Haven't checked the details
yet, though.

Jan
Jan Kiszka - June 8, 2011, 2:32 p.m.
On 2011-06-08 14:33, Jan Kiszka wrote:
> On 2011-06-08 14:28, Gerd Hoffmann wrote:
>>> @@ -1173,12 +1173,8 @@ static int intel_hda_exit(PCIDevice *pci)
>>>   static void intel_hda_write_config(PCIDevice *pci, uint32_t addr,
>>>                                      uint32_t val, int len)
>>>   {
>>> -    IntelHDAState *d = DO_UPCAST(IntelHDAState, pci, pci);
>>> -
>>>       pci_default_write_config(pci, addr, val, len);
>>> -    if (d->msi) {
>>> -        msi_write_config(pci, addr, val, len);
>>> -    }
>>> +    msi_write_config(pci, addr, val, len);
>>>   }
>>
>> Nothing device specific left in there now.  If msi_write_config() checks 
>> itself whenever msi is enabled or not we could call it from 
>> pci_default_write_config(), then zap this function altogether, no?
> 
> Well, we could put those helpers (msi_write_config, msi_reset, maybe
> more) into generic PCI code. Then device will only have to overload the
> handlers if they do something in addition. Haven't checked the details
> yet, though.

Seems to fly.

I'll repost the series to make msi_reset, msi_write_config, and
msi_uninit implicit. Less boilerplate, less potential bugs.

Jan

Patch

diff --git a/hw/intel-hda.c b/hw/intel-hda.c
index 5485745..71843f1 100644
--- a/hw/intel-hda.c
+++ b/hw/intel-hda.c
@@ -1173,12 +1173,8 @@  static int intel_hda_exit(PCIDevice *pci)
 static void intel_hda_write_config(PCIDevice *pci, uint32_t addr,
                                    uint32_t val, int len)
 {
-    IntelHDAState *d = DO_UPCAST(IntelHDAState, pci, pci);
-
     pci_default_write_config(pci, addr, val, len);
-    if (d->msi) {
-        msi_write_config(pci, addr, val, len);
-    }
+    msi_write_config(pci, addr, val, len);
 }
 
 static int intel_hda_post_load(void *opaque, int version)
diff --git a/hw/msi.c b/hw/msi.c
index e8c5607..b7a92c9 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -264,7 +264,8 @@  void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len)
     unsigned int vector;
     uint32_t pending;
 
-    if (!ranges_overlap(addr, len, dev->msi_cap, msi_cap_sizeof(flags))) {
+    if (!msi_present(dev) ||
+        !ranges_overlap(addr, len, dev->msi_cap, msi_cap_sizeof(flags))) {
         return;
     }
 
diff --git a/hw/msix.c b/hw/msix.c
index af40e26..703f082 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -160,7 +160,7 @@  void msix_write_config(PCIDevice *dev, uint32_t addr,
     unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET;
     int vector;
 
-    if (!range_covers_byte(addr, len, enable_pos)) {
+    if (!msix_present(dev) || !range_covers_byte(addr, len, enable_pos)) {
         return;
     }