| Message ID | 1397659459-10069-3-git-send-email-hare@suse.de |
|---|---|
| State | New |
| Headers | show |
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 <hare@suse.de> > --- > 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? Alex
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 <hare@suse.de> >> --- >> 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? The MSIX addition should be safe AFAICT since old versions would not enable MSI-X. Regards, Andreas
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 <hare@suse.de> > >> --- > >> 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. > The MSIX addition should be safe AFAICT since old versions would not > enable MSI-X. > > Regards, > Andreas Yes but I don't see a code like this - where's code in PC disabling msix for old versions? > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
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 <hare@suse.de> >>>> --- >>>> 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. >> 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
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 <hare@suse.de> > >>>> --- > >>>> 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. > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
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 <hare@suse.de> >>>>>> --- >>>>>> 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? Cheers, Hannes
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 <hare@suse.de> >>>>>>> --- >>>>>>> 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). Anyway, this MSI-X patch adding only VMSTATE_MSIX() after VMSTATE_PCI[E]_DEVICE() then becomes trivially correct IMO. If you wanted to turn on MSI-X by default as in v1, then you would need a trivial three-line struct entry in include/hw/i386/pc.h:PC_COMPAT_2_0 setting the property to the old default of false. Cheers, Andreas
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 <hare@suse.de> >>>>>>>> --- >>>>>>>> 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. Would that be okay? Cheers, Hannes
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 <hare@suse.de> > >>>>>>>> --- > >>>>>>>> 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. > > Would that be okay? > > Cheers, > > Hannes And then gen1 and gen2 get different names? That's okay. Alternatively, I think it's reasonable to add a property which specifies PCI or PCIE. You then force compatible behaviour for old machine types, see e.g. PC_COMPAT_XX macros in include/hw/i386/pc.h > -- > Dr. Hannes Reinecke zSeries & Storage > hare@suse.de +49 911 74053 688 > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg > GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
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 <hare@suse.de> >>>>>>>>>> --- >>>>>>>>>> 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
On 04/28/2014 01:45 PM, Paolo Bonzini wrote: >> Hannes Reinecke wrote: >>> 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. > Hmm. Better let me send a new patch which reverts the PCI->PCIE change and make only megasas-gen2 a PCIE board. Ok with you? Cheers, Hannes
Il 28/04/2014 13:49, Hannes Reinecke ha scritto: >> >> Ok, for now I'm applying this patch without the PCI->PCIE change. >> > Hmm. Better let me send a new patch which reverts the PCI->PCIE change > and make only megasas-gen2 a PCIE board. If I push patches 1-3 and 7 to scsi-next, you can start from there. Paolo
On 04/28/2014 01:51 PM, Paolo Bonzini wrote: > Il 28/04/2014 13:49, Hannes Reinecke ha scritto: >>> >>> Ok, for now I'm applying this patch without the PCI->PCIE change. >>> >> Hmm. Better let me send a new patch which reverts the PCI->PCIE >> change >> and make only megasas-gen2 a PCIE board. > > If I push patches 1-3 and 7 to scsi-next, you can start from there. > Ok. Let me know when you're done. Cheers, Hannes
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), VMSTATE_INT32(fw_state, MegasasState), VMSTATE_INT32(intr_mask, MegasasState), @@ -2100,9 +2101,7 @@ static void megasas_scsi_uninit(PCIDevice *d) { MegasasState *s = MEGASAS(d); -#ifdef USE_MSIX - msix_uninit(d, &s->mmio_io); -#endif + msix_uninit(d, &s->mmio_io, &s->mmio_io); memory_region_destroy(&s->mmio_io); memory_region_destroy(&s->port_io); memory_region_destroy(&s->queue_io); @@ -2141,15 +2140,11 @@ static int megasas_scsi_init(PCIDevice *dev) memory_region_init_io(&s->queue_io, OBJECT(s), &megasas_queue_ops, s, "megasas-queue", 0x40000); -#ifdef USE_MSIX - /* MSI-X support is currently broken */ if (megasas_use_msix(s) && - msix_init(dev, 15, &s->mmio_io, 0, 0x2000)) { + msix_init(dev, 15, &s->mmio_io, 0, 0x2000, + &s->mmio_io, 0, 0x3800, 0x68)) { s->flags &= ~MEGASAS_MASK_USE_MSIX; } -#else - s->flags &= ~MEGASAS_MASK_USE_MSIX; -#endif bar_type = PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64; pci_register_bar(dev, 0, bar_type, &s->mmio_io); @@ -2168,7 +2163,7 @@ static int megasas_scsi_init(PCIDevice *dev) s->sas_addr |= PCI_FUNC(dev->devfn); } if (!s->hba_serial) { - s->hba_serial = g_strdup(MEGASAS_HBA_SERIAL); + s->hba_serial = g_strdup(MEGASAS_HBA_SERIAL); } if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) { s->fw_sge = MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE; @@ -2213,10 +2208,8 @@ static Property megasas_properties[] = { MEGASAS_DEFAULT_FRAMES), DEFINE_PROP_STRING("hba_serial", MegasasState, hba_serial), DEFINE_PROP_UINT64("sas_address", MegasasState, sas_addr, 0), -#ifdef USE_MSIX DEFINE_PROP_BIT("use_msix", MegasasState, flags, MEGASAS_FLAG_USE_MSIX, false), -#endif DEFINE_PROP_BIT("use_jbod", MegasasState, flags, MEGASAS_FLAG_USE_JBOD, false), DEFINE_PROP_END_OF_LIST(),
MSI-X support has been fixed in qemu, so we can enable it again. Signed-off-by: Hannes Reinecke <hare@suse.de> --- hw/scsi/megasas.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)