Re: MSI-X fix
On Thu, May 30, 2019 at 08:25:39PM +0200, Mark Kettenis wrote:
> I started implementing MSI-X support for arm64 adn tried to use re(4)
> for testing. This failed miserably and when I tried to use MSI-X with
> re(4) on amd64 I noticed it didn't work either.
>
> Turns out the Realtek hardware doesn't support 64-bit access to MSI-X
> table. Reads return 0x and writes get ignored.
> Looking at the relevant documentation I noticed that the message
> address is actually split into two 32-bit "DWORD" entries. Which in
> turn suggests that the Realtek folks have the standard on their side...
>
> Diff below fixes the issue. I did already add a "MAU32" define for
> this, even though it wasn't quite right ;). I also took the
> opportunity to add the "function mask" bit that we'll probably need in
> the future if we start being clever with using multiple vectors and
> ave a need to guarantee atomicity of making changes to the MSI-X
> config.
>
> ok?
I had wondered about the MAU32 define when I was looking at MSI-X
suspend/resume. Anyway, this makes sense to me and works with mcx(4),
so ok jmatthew@
>
>
> Index: dev/pci/pcireg.h
> ===
> RCS file: /cvs/src/sys/dev/pci/pcireg.h,v
> retrieving revision 1.56
> diff -u -p -r1.56 pcireg.h
> --- dev/pci/pcireg.h 3 Aug 2018 22:18:13 - 1.56
> +++ dev/pci/pcireg.h 30 May 2019 18:06:39 -
> @@ -617,6 +617,7 @@ typedef u_int8_t pci_revision_t;
> * Extended Message Signaled Interrups; access via capability pointer.
> */
> #define PCI_MSIX_MC_MSIXE0x8000
> +#define PCI_MSIX_MC_FM 0x4000
> #define PCI_MSIX_MC_TBLSZ_MASK 0x07ff
> #define PCI_MSIX_MC_TBLSZ_SHIFT 16
> #define PCI_MSIX_MC_TBLSZ(reg) \
> @@ -626,7 +627,7 @@ typedef u_int8_t pci_revision_t;
> #define PCI_MSIX_TABLE_OFF ~(PCI_MSIX_TABLE_BIR)
>
> #define PCI_MSIX_MA(i) ((i) * 16 + 0)
> -#define PCI_MSIX_MAU32(i)((i) * 16 + 0)
> +#define PCI_MSIX_MAU32(i)((i) * 16 + 4)
> #define PCI_MSIX_MD(i) ((i) * 16 + 8)
> #define PCI_MSIX_VC(i) ((i) * 16 + 12)
> #define PCI_MSIX_VC_MASK0x0001
> Index: arch/amd64/pci/pci_machdep.c
> ===
> RCS file: /cvs/src/sys/arch/amd64/pci/pci_machdep.c,v
> retrieving revision 1.69
> diff -u -p -r1.69 pci_machdep.c
> --- arch/amd64/pci/pci_machdep.c 19 Aug 2018 08:23:47 - 1.69
> +++ arch/amd64/pci/pci_machdep.c 30 May 2019 18:06:39 -
> @@ -475,7 +475,8 @@ msix_addroute(struct pic *pic, struct cp
> _bus_space_map(memt, base + offset, tblsz * 16, 0, ))
> panic("%s: cannot map registers", __func__);
>
> - bus_space_write_8(memt, memh, PCI_MSIX_MA(entry), addr);
> + bus_space_write_4(memt, memh, PCI_MSIX_MA(entry), addr);
> + bus_space_write_4(memt, memh, PCI_MSIX_MAU32(entry), 0);
> bus_space_write_4(memt, memh, PCI_MSIX_MD(entry), vec);
> bus_space_barrier(memt, memh, PCI_MSIX_MA(entry), 16,
> BUS_SPACE_BARRIER_WRITE);
>
Re: MSI-X fix
> Date: Thu, 30 May 2019 12:43:17 -0700
> From: Mike Larkin
>
> On Thu, May 30, 2019 at 08:25:39PM +0200, Mark Kettenis wrote:
> > I started implementing MSI-X support for arm64 adn tried to use re(4)
> > for testing. This failed miserably and when I tried to use MSI-X with
> > re(4) on amd64 I noticed it didn't work either.
> >
> > Turns out the Realtek hardware doesn't support 64-bit access to MSI-X
> > table. Reads return 0x and writes get ignored.
> > Looking at the relevant documentation I noticed that the message
> > address is actually split into two 32-bit "DWORD" entries. Which in
> > turn suggests that the Realtek folks have the standard on their side...
> >
> > Diff below fixes the issue. I did already add a "MAU32" define for
> > this, even though it wasn't quite right ;). I also took the
> > opportunity to add the "function mask" bit that we'll probably need in
> > the future if we start being clever with using multiple vectors and
> > ave a need to guarantee atomicity of making changes to the MSI-X
> > config.
> >
> > ok?
> >
>
> This reads ok to me. I believe the APIC address can be changed by
> writing to the APICBASE MSR, and from what I can tell there's no requirement
> that the base be < 4GB. I know we don't do that today, but should we write
> the high bits of 'addr' instead of writing '0' to make things future proof?
>
> Or is this just busywork?
Probably it is at this point. This MSI-X implementation is
bug-compatible with MSI. MSI has the issue that there might be
devices that only support a 32-bit address. So in practice I think
current systems will always have the APIC at the fixed address of
0xffe0. That may change in the future at which point we should
review this code.
My arm64 code does what you suggest.
> > Index: dev/pci/pcireg.h
> > ===
> > RCS file: /cvs/src/sys/dev/pci/pcireg.h,v
> > retrieving revision 1.56
> > diff -u -p -r1.56 pcireg.h
> > --- dev/pci/pcireg.h3 Aug 2018 22:18:13 - 1.56
> > +++ dev/pci/pcireg.h30 May 2019 18:06:39 -
> > @@ -617,6 +617,7 @@ typedef u_int8_t pci_revision_t;
> > * Extended Message Signaled Interrups; access via capability pointer.
> > */
> > #define PCI_MSIX_MC_MSIXE 0x8000
> > +#define PCI_MSIX_MC_FM 0x4000
> > #define PCI_MSIX_MC_TBLSZ_MASK 0x07ff
> > #define PCI_MSIX_MC_TBLSZ_SHIFT16
> > #define PCI_MSIX_MC_TBLSZ(reg) \
> > @@ -626,7 +627,7 @@ typedef u_int8_t pci_revision_t;
> > #define PCI_MSIX_TABLE_OFF~(PCI_MSIX_TABLE_BIR)
> >
> > #define PCI_MSIX_MA(i) ((i) * 16 + 0)
> > -#define PCI_MSIX_MAU32(i) ((i) * 16 + 0)
> > +#define PCI_MSIX_MAU32(i) ((i) * 16 + 4)
> > #define PCI_MSIX_MD(i) ((i) * 16 + 8)
> > #define PCI_MSIX_VC(i) ((i) * 16 + 12)
> > #define PCI_MSIX_VC_MASK 0x0001
> > Index: arch/amd64/pci/pci_machdep.c
> > ===
> > RCS file: /cvs/src/sys/arch/amd64/pci/pci_machdep.c,v
> > retrieving revision 1.69
> > diff -u -p -r1.69 pci_machdep.c
> > --- arch/amd64/pci/pci_machdep.c19 Aug 2018 08:23:47 - 1.69
> > +++ arch/amd64/pci/pci_machdep.c30 May 2019 18:06:39 -
> > @@ -475,7 +475,8 @@ msix_addroute(struct pic *pic, struct cp
> > _bus_space_map(memt, base + offset, tblsz * 16, 0, ))
> > panic("%s: cannot map registers", __func__);
> >
> > - bus_space_write_8(memt, memh, PCI_MSIX_MA(entry), addr);
> > + bus_space_write_4(memt, memh, PCI_MSIX_MA(entry), addr);
> > + bus_space_write_4(memt, memh, PCI_MSIX_MAU32(entry), 0);
> > bus_space_write_4(memt, memh, PCI_MSIX_MD(entry), vec);
> > bus_space_barrier(memt, memh, PCI_MSIX_MA(entry), 16,
> > BUS_SPACE_BARRIER_WRITE);
> >
>
Re: MSI-X fix
On Thu, May 30, 2019 at 08:25:39PM +0200, Mark Kettenis wrote:
> I started implementing MSI-X support for arm64 adn tried to use re(4)
> for testing. This failed miserably and when I tried to use MSI-X with
> re(4) on amd64 I noticed it didn't work either.
>
> Turns out the Realtek hardware doesn't support 64-bit access to MSI-X
> table. Reads return 0x and writes get ignored.
> Looking at the relevant documentation I noticed that the message
> address is actually split into two 32-bit "DWORD" entries. Which in
> turn suggests that the Realtek folks have the standard on their side...
>
> Diff below fixes the issue. I did already add a "MAU32" define for
> this, even though it wasn't quite right ;). I also took the
> opportunity to add the "function mask" bit that we'll probably need in
> the future if we start being clever with using multiple vectors and
> ave a need to guarantee atomicity of making changes to the MSI-X
> config.
>
> ok?
>
This reads ok to me. I believe the APIC address can be changed by
writing to the APICBASE MSR, and from what I can tell there's no requirement
that the base be < 4GB. I know we don't do that today, but should we write
the high bits of 'addr' instead of writing '0' to make things future proof?
Or is this just busywork?
Either way ok mlarkin.
-ml
>
> Index: dev/pci/pcireg.h
> ===
> RCS file: /cvs/src/sys/dev/pci/pcireg.h,v
> retrieving revision 1.56
> diff -u -p -r1.56 pcireg.h
> --- dev/pci/pcireg.h 3 Aug 2018 22:18:13 - 1.56
> +++ dev/pci/pcireg.h 30 May 2019 18:06:39 -
> @@ -617,6 +617,7 @@ typedef u_int8_t pci_revision_t;
> * Extended Message Signaled Interrups; access via capability pointer.
> */
> #define PCI_MSIX_MC_MSIXE0x8000
> +#define PCI_MSIX_MC_FM 0x4000
> #define PCI_MSIX_MC_TBLSZ_MASK 0x07ff
> #define PCI_MSIX_MC_TBLSZ_SHIFT 16
> #define PCI_MSIX_MC_TBLSZ(reg) \
> @@ -626,7 +627,7 @@ typedef u_int8_t pci_revision_t;
> #define PCI_MSIX_TABLE_OFF ~(PCI_MSIX_TABLE_BIR)
>
> #define PCI_MSIX_MA(i) ((i) * 16 + 0)
> -#define PCI_MSIX_MAU32(i)((i) * 16 + 0)
> +#define PCI_MSIX_MAU32(i)((i) * 16 + 4)
> #define PCI_MSIX_MD(i) ((i) * 16 + 8)
> #define PCI_MSIX_VC(i) ((i) * 16 + 12)
> #define PCI_MSIX_VC_MASK0x0001
> Index: arch/amd64/pci/pci_machdep.c
> ===
> RCS file: /cvs/src/sys/arch/amd64/pci/pci_machdep.c,v
> retrieving revision 1.69
> diff -u -p -r1.69 pci_machdep.c
> --- arch/amd64/pci/pci_machdep.c 19 Aug 2018 08:23:47 - 1.69
> +++ arch/amd64/pci/pci_machdep.c 30 May 2019 18:06:39 -
> @@ -475,7 +475,8 @@ msix_addroute(struct pic *pic, struct cp
> _bus_space_map(memt, base + offset, tblsz * 16, 0, ))
> panic("%s: cannot map registers", __func__);
>
> - bus_space_write_8(memt, memh, PCI_MSIX_MA(entry), addr);
> + bus_space_write_4(memt, memh, PCI_MSIX_MA(entry), addr);
> + bus_space_write_4(memt, memh, PCI_MSIX_MAU32(entry), 0);
> bus_space_write_4(memt, memh, PCI_MSIX_MD(entry), vec);
> bus_space_barrier(memt, memh, PCI_MSIX_MA(entry), 16,
> BUS_SPACE_BARRIER_WRITE);
>
