Re: [Qemu-devel] [PATCH 2/7] megasas: Enable MSI-X support

[Paolo Bonzini]

Il 17/04/2014 17:01, Michael S. Tsirkin ha scritto:
> On Thu, Apr 17, 2014 at 03:41:56PM +0200, Hannes Reinecke wrote:
> > On 04/17/2014 02:07 PM, Andreas Färber wrote:
> > > Am 17.04.2014 08:20, schrieb Hannes Reinecke:
> > > > On 04/16/2014 07:52 PM, Michael S. Tsirkin wrote:
> > > > > On Wed, Apr 16, 2014 at 07:47:37PM +0200, Andreas Färber wrote:
> > > > > > Am 16.04.2014 19:40, schrieb Michael S. Tsirkin:
> > > > > > > On Wed, Apr 16, 2014 at 06:48:08PM +0200, Andreas Färber wrote:
> > > > > > > > Am 16.04.2014 18:32, schrieb Alexander Graf:
> > > > > > > > > On 16.04.14 16:44, Hannes Reinecke wrote:
> > > > > > > > > > MSI-X support has been fixed in qemu, so we can enable it again.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Hannes Reinecke <address@hidden>
> > > > > > > > > > ---
> > > > > > > > > >   hw/scsi/megasas.c | 19 ++++++-------------
> > > > > > > > > >   1 file changed, 6 insertions(+), 13 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> > > > > > > > > > index 1781525..df45286 100644
> > > > > > > > > > --- a/hw/scsi/megasas.c
> > > > > > > > > > +++ b/hw/scsi/megasas.c
> > > > > > > > > > @@ -2084,7 +2084,8 @@ static const VMStateDescription vmstate_megasas = {
> > > > > > > > > >       .minimum_version_id = 0,
> > > > > > > > > >       .minimum_version_id_old = 0,
> > > > > > > > > >       .fields      = (VMStateField[]) {
> > > > > > > > > > -        VMSTATE_PCI_DEVICE(parent_obj, MegasasState),
> > > > > > > > > > +        VMSTATE_PCIE_DEVICE(parent_obj, MegasasState),
> > > > > > > > > > +        VMSTATE_MSIX(parent_obj, MegasasState),
> > > > > > > > >
> > > > > > > > > This requires a version change for vmstate, no?
> > > > > > > >
> > > > > > > > The PCI -> PCIE change yes. mst might have objections for bumping an x86
> > > > > > > > PCI device?
> > > > > > >
> > > > > > > Yes.
> > > > > > > It should be possible to make this conditional on having a pcie
> > > > > > > capability, and disable pcie capability for old pc versions.
> > > > > >
> > > > > > I did have a patch merging their vmstate_ structs based on some test in
> > > > > > the series I ping'ed, unfortunately the difference was mostly config
> > > > > > space size, so not immediately obvious how to set via compat_props, but
> > > > > > likely solvable somehow.
> > > > >
> > > > > Check pci_is_express ?
> > > > >
> > > > > > > > The MSIX addition should be safe AFAICT since old versions would not
> > > > > > > > enable MSI-X.
> > > > > > >
> > > > > > > Yes but I don't see a code like this - where's code in PC
> > > > > > > disabling msix for old versions?
> > > > > >
> > > > > > The default value for use_msix (use-msix?) in v2 is already false, so no
> > > > > > need to set it to false again in compat_props?
> > > > > >
> > > > > > Andreas
> > > > >
> > > > > Ah, I didn't notice that. Sure, if it's off by default there's
> > > > > no need for compat code.
> > > > And the net result is ... what?
> > > > Do I need to change the code?
> > >
> > > Yes, NAK for this patch as is.
> > >
> > > Why are you changing VMSTATE_PCI_DEVICE() to VMSTATE_PCIE_DEVICE()? Was
> > > it always a PCIe device and you're fixing that now? Then put it into its
> > > own patch with proper explanation, setting pdc->is_express = 1 *and*
> > > assure there via some compatibility property (?) that migration from
> > > 2.0.0-rc3 to the patched version succeeds (which would probably still
> > > break backwards migration that mst was interested in though).
> > The thing was always PCIe; it's just that at the time I've
> > originally implemented it MSI-X support was broken, and PCIe even
> > more so. So I figured to take the easy way out and implement it as a
> > normal PCI device.
> >
> > I'm happy to continue with having the original implementation at
> > PCI, and only moving to PCIe with the gen2 emulation.

Ok, for now I'm applying this patch without the PCI->PCIE change.

Paolo