Re: [Qemu-devel] [PATCH 4/6] msi: Invoke msi/msix_reset from PCI core

[Jan Kiszka]

On 2011-12-04 15:24, Michael S. Tsirkin wrote:
> On Sun, Dec 04, 2011 at 02:22:12PM +0100, Jan Kiszka wrote:
>> From: Jan Kiszka <address@hidden>
>>
>> There is no point in pushing this burden to the devices, they may rather
>> forget to call them (like intel-hda and ahci ATM). Instead, reset
>> functions are now called from pci_device_reset and pci_bridge_reset.
>> They do nothing if the MSI/MSI-X is not in use.
>>
>> CC: Alexander Graf <address@hidden>
>> CC: Gerd Hoffmann <address@hidden>
>> CC: Isaku Yamahata <address@hidden>
>> Signed-off-by: Jan Kiszka <address@hidden>
> 
> What makes me unhappy with this proposal is that msix_write_config, for
> example, becomes in fact an internal interface. So devices should be
> calling some functions like msix_init from msix.h, but not others like
> msix_write_config.
> 
> It used to be simple: devices should call msix_.
> Now, how are devices to figure it out?
> 
> E.g. the comment near msix_write_config says:
> /* Handle MSI-X capability config write. */

That should be aligned to msi_write_config's comment.

My goal is to reduce the number of calls devices have to do in order to
use MSI. We have quite a few correct examples by now, so it should not
be too hard to figure out what to do to use standard MSI[X] services.

Maybe a PCI skeleton device model would help further. Or up-to-date
documentation, thought that may be even harder. ;)

> 
> This puts it at level 11 on Rusty's misuse scale:
> Read the documentation and you will get it wrong.
> 
> So I tried writing a wapper, something like pci_capability.h, that would
> hide the detail and handle all capabilities seamlessly.  Where I got
> stuck was migration though, format is ordered so we can't just move the
> fields around.  So I decided to wait until we switch to an unordered
> format, then it'll become easy.
> 
> Thoughts?

MSI-X save/restore is, well, unfortunate. Just like the whole PCI layer
in this regard. But I don't think that should block this particular step
as it frees device models from an unneeded burden.

Jan

signature.asc
Description: OpenPGP digital signature