Re: [PATCH 01/15] pcie: Set default and supported MaxReadReq to 512

[Knut Omang]

On Tue, 2021-10-26 at 16:36 +0200, Lukasz Maniak wrote:
> On Thu, Oct 07, 2021 at 06:12:41PM -0400, Michael S. Tsirkin wrote:
> > On Thu, Oct 07, 2021 at 06:23:52PM +0200, Lukasz Maniak wrote:
> > > From: Knut Omang <knut.omang@oracle.com>
> > > 
> > > Make the default PCI Express Capability for PCIe devices set
> > > MaxReadReq to 512.
> > 
> > code says 256
> > 
> > > Tyipcal modern devices people would want to
> > 
> > 
> > typo
> > 
> > > emulate or simulate would want this. The previous value would
> > > cause warnings from the root port driver on some kernels.
> > 
> > 
> > which specifically?
> > 
> > > 
> > > Signed-off-by: Knut Omang <knuto@ifi.uio.no>
> > 
> > we can't make changes like this unconditionally, this will
> > break migration across versions.
> > Pls tie this to a machine version.
> > 
> > Thanks!
> > > ---
> > >   hw/pci/pcie.c | 5 ++++-
> > >   1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > index 6e95d82903..c1a12f3744 100644
> > > --- a/hw/pci/pcie.c
> > > +++ b/hw/pci/pcie.c
> > > @@ -62,8 +62,9 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t 
> > > type, uint8_t
> > > version)
> > >        * Functions conforming to the ECN, PCI Express Base
> > >        * Specification, Revision 1.1., or subsequent PCI Express Base
> > >        * Specification revisions.
> > > +     *  + set max payload size to 256, which seems to be a common value
> > >        */
> > > -    pci_set_long(exp_cap + PCI_EXP_DEVCAP, PCI_EXP_DEVCAP_RBER);
> > > +    pci_set_long(exp_cap + PCI_EXP_DEVCAP, PCI_EXP_DEVCAP_RBER | (0x1 &
> > > PCI_EXP_DEVCAP_PAYLOAD));
> > >   
> > >       pci_set_long(exp_cap + PCI_EXP_LNKCAP,
> > >                    (port << PCI_EXP_LNKCAP_PN_SHIFT) |
> > > @@ -179,6 +180,8 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset,
> > >       pci_set_long(exp_cap + PCI_EXP_DEVCAP2,
> > >                    PCI_EXP_DEVCAP2_EFF | PCI_EXP_DEVCAP2_EETLPP);
> > >   
> > > +    pci_set_word(exp_cap + PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_READRQ_256B);
> > > +
> > >       pci_set_word(dev->wmask + pos + PCI_EXP_DEVCTL2, 
> > > PCI_EXP_DEVCTL2_EETLPPB);
> > >   
> > >       if (dev->cap_present & QEMU_PCIE_EXTCAP_INIT) {
> > > -- 
> > > 2.25.1
> > 
> 
> Hi Michael,
> 
> The reason Knut keeps rebasing this fix along with SR-IOV patch is not
> clear for us.

Sorry for the slow response - I seem to have messed up my mail filters so this
thread slipped past my attention.

> Since we have tested the NVMe device without this fix and did not notice
> any issues mentioned by Knut on kernel 5.4.0, we decided to drop it for
> v2.

I agree, let's just drop it.

It was likely in the 3.x kernels I had to relate to back then, 
likely discovered in Oracle Linux given that I did not specifically point to a 
kernel
range already back then.

> However, I have posted your comments to this patch on Knut's github so
> they can be addressed in case Knut decides to resubmit it later though.

Thanks for that ping, Lukasz, and great to see the patch finally being used in a
functional device!

Knut

> Thanks,
> Lukasz