Re: [SeaBIOS] [PATCH v3 2/5] hw/pci: introduce pcie-pci-bridge device

[Michael S. Tsirkin]

On Mon, Jul 31, 2017 at 09:40:41PM +0300, Alexander Bezzubikov wrote:
> > > +typedef struct PCIEPCIBridge {
> > > /*< private >*/
> > > PCIBridge parent_obj;
> > > bool msi_enable;
> >
> > Please rename the msi_enable property to "msi" in order
> > to be aligned with the existent PCIBridgeDev and
> > consider making it OnOffAuto for the same reason.
> > (I am not sure about the last part though, we have
> >  no meaning for "auto" here)
>
> Agreed about "msi", but OnOffAuto looks weird to me
> as we always want MSI to be enabled.

Why even have a property then? Can't you enable it unconditionally?

[PATCH v3 2/5] hw/pci: introduce pcie-pci-bridge device

[Marcel Apfelbaum]

On 01/08/2017 18:32, Michael S. Tsirkin wrote:
> On Mon, Jul 31, 2017 at 09:40:41PM +0300, Alexander Bezzubikov wrote:
> > > > +typedef struct PCIEPCIBridge {
> > > > /*< private >*/
> > > > PCIBridge parent_obj;
> > > > bool msi_enable;
> > >
> > > Please rename the msi_enable property to "msi" in order
> > > to be aligned with the existent PCIBridgeDev and
> > > consider making it OnOffAuto for the same reason.
> > > (I am not sure about the last part though, we have
> > >   no meaning for "auto" here)
> >
> > Agreed about "msi", but OnOffAuto looks weird to me
> > as we always want MSI to be enabled.

Hi Michael,
> Why even have a property then? Can't you enable it unconditionally?

Because of a current bug in Linux kernel:
    https://www.spinics.net/lists/linux-pci/msg63052.html
msi will not work until the patch is merged. Even when
it will be merged, not all linux kernels will contain the patch.

Disabling msi is a workaround for the above case.

Thanks,
Marcel

[PATCH v3 2/5] hw/pci: introduce pcie-pci-bridge device

[Michael S. Tsirkin]

On Tue, Aug 01, 2017 at 06:45:13PM +0300, Marcel Apfelbaum wrote:
> On 01/08/2017 18:32, Michael S. Tsirkin wrote:
> > On Mon, Jul 31, 2017 at 09:40:41PM +0300, Alexander Bezzubikov wrote:
> > > > > +typedef struct PCIEPCIBridge {
> > > > > /*< private >*/
> > > > > PCIBridge parent_obj;
> > > > > bool msi_enable;
> > > >
> > > > Please rename the msi_enable property to "msi" in order
> > > > to be aligned with the existent PCIBridgeDev and
> > > > consider making it OnOffAuto for the same reason.
> > > > (I am not sure about the last part though, we have
> > > >   no meaning for "auto" here)
> > >
> > > Agreed about "msi", but OnOffAuto looks weird to me
> > > as we always want MSI to be enabled.
>
> Hi Michael,
> > Why even have a property then? Can't you enable it unconditionally?
>
> Because of a current bug in Linux kernel:
>    https://www.spinics.net/lists/linux-pci/msg63052.html
> msi will not work until the patch is merged. Even when
> it will be merged, not all linux kernels will contain the patch.

You should Cc stable to make sure they all gain it eventually.
> Disabling msi is a workaround for the above case.
>
> Thanks,
> Marcel

Really enabling MSI without bus master is a bug that I'm not 100% sure
it even worth working around. But I guess it's not too bad to have a
work-around given it's this simple.

[PATCH v3 2/5] hw/pci: introduce pcie-pci-bridge device

[Marcel Apfelbaum]

On 01/08/2017 18:51, Michael S. Tsirkin wrote:
> On Tue, Aug 01, 2017 at 06:45:13PM +0300, Marcel Apfelbaum wrote:
> > On 01/08/2017 18:32, Michael S. Tsirkin wrote:
> > > On Mon, Jul 31, 2017 at 09:40:41PM +0300, Alexander Bezzubikov wrote:
> > > > > > +typedef struct PCIEPCIBridge {
> > > > > > /*< private >*/
> > > > > > PCIBridge parent_obj;
> > > > > > bool msi_enable;
> > > > >
> > > > > Please rename the msi_enable property to "msi" in order
> > > > > to be aligned with the existent PCIBridgeDev and
> > > > > consider making it OnOffAuto for the same reason.
> > > > > (I am not sure about the last part though, we have
> > > > >    no meaning for "auto" here)
> > > >
> > > > Agreed about "msi", but OnOffAuto looks weird to me
> > > > as we always want MSI to be enabled.
> >
> > Hi Michael,
> > > Why even have a property then? Can't you enable it unconditionally?
> >
> > Because of a current bug in Linux kernel:
> >     https://www.spinics.net/lists/linux-pci/msg63052.html
> > msi will not work until the patch is merged. Even when
> > it will be merged, not all linux kernels will contain the patch.
>
> You should Cc stable to make sure they all gain it eventually.

Right! thanks, we missed cc-ing stable.
Added stable to the mail thread.
Marcel
> > Disabling msi is a workaround for the above case.
> >
> > Thanks,
> > Marcel
>
> Really enabling MSI without bus master is a bug that I'm not 100% sure
> it even worth working around. But I guess it's not too bad to have a
> work-around given it's this simple.