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

Submitted by Jan Kiszka on June 8, 2011, 10:26 a.m.

Details

Message ID 0074e8926b2a8c93a8ef6aab9071d3e7e882a6c3.1307528800.git.jan.kiszka@siemens.com
State New
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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