mbox series

[RFC,00/12] Add Xue - console over USB 3 Debug Capability

Message ID cover.07846d9c1900bd8c905a05e7afb214b4cf4ab881.1654486751.git-series.marmarek@invisiblethingslab.com (mailing list archive)
Headers show
Series Add Xue - console over USB 3 Debug Capability | expand

Message

Marek Marczykowski-Górecki June 6, 2022, 3:40 a.m. UTC
This is integration of https://github.com/connojd/xue into mainline Xen.
This patch series includes several patches that I made in the process, some are
very loosely related.

The RFC status is to collect feedback on the shape of this series, specifically:

1. The actual Xue driver is a header-only library. Most of the code is in a
series of inline functions in xue.h. I kept it this way, to ease integrating
Xue updates. That's also why I preserved its original code style. Is it okay,
or should I move the code to a .c file?

2. The xue.h file includes bindings for several other environments too (EFI,
Linux, ...). This is unused code, behind #ifdef. Again, I kept it to ease
updating. Should I remove it?

3. The adding of IOMMU reserverd memory is necessary even if "hiding" device
from dom0. Otherwise, VT-d will deny DMA. That's probably not the most elegant
solution, but Xen doesn't have seem to have provisions for devices doing DMA
into Xen's memory.

4. To preserve authorship, I included unmodified "drivers/char: Add support for
Xue USB3 debugger" commit from Connor, and only added my changes on top. This
means, with that this commit, the driver doesn't work yet (but it does
compile). Is it okay, or should I combine fixes into that commit and somehow
mark authorship in the commit message?

5. The last patch(es) enable using the controller by dom0, even when Xen
uses DbC part. That's possible, because the capability was designed
specifically to allow separate driver handle it, in parallel to unmodified xhci
driver (separate set of registers, pretending the port is "disconnected" for
the main xhci driver etc). It works with Linux dom0, although requires an awful
hack - re-enabling bus mastering behind dom0's backs. Is it okay to leave this
functionality as is, or guard it behind some cmdline option, or maybe remove
completely?

Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Paul Durrant <paul@xen.org>
Cc: Kevin Tian <kevin.tian@intel.com>

Connor Davis (1):
  drivers/char: Add support for Xue USB3 debugger

Marek Marczykowski-Górecki (11):
  xue: annotate functions with cf_check
  xue: reset XHCI ports when initializing dbc
  xue: add support for selecting specific xhci
  ehci-dbgp: fix selecting n-th ehci controller
  console: support multiple serial console simultaneously
  IOMMU: add common API for device reserved memory
  IOMMU/VT-d: wire common device reserved memory API
  IOMMU/AMD: wire common device reserved memory API
  xue: mark DMA buffers as reserved for the device
  xue: prevent dom0 (or other domain) from using the device
  xue: allow driving the reset of XHCI by a domain while Xen uses DbC

 docs/misc/xen-command-line.pandoc        |    5 +-
 xen/arch/x86/include/asm/fixmap.h        |    4 +-
 xen/arch/x86/setup.c                     |    5 +-
 xen/drivers/char/Makefile                |    1 +-
 xen/drivers/char/console.c               |   58 +-
 xen/drivers/char/ehci-dbgp.c             |    2 +-
 xen/drivers/char/xue.c                   |  197 ++-
 xen/drivers/passthrough/amd/iommu_acpi.c |   16 +-
 xen/drivers/passthrough/iommu.c          |   40 +-
 xen/drivers/passthrough/vtd/dmar.c       |  203 +-
 xen/include/xen/iommu.h                  |   11 +-
 xen/include/xue.h                        | 1942 +++++++++++++++++++++++-
 12 files changed, 2387 insertions(+), 97 deletions(-)
 create mode 100644 xen/drivers/char/xue.c
 create mode 100644 xen/include/xue.h

base-commit: 49dd52fb1311dadab29f6634d0bc1f4c022c357a

Comments

Tamas K Lengyel June 6, 2022, 1:18 p.m. UTC | #1
> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Marek
> Marczykowski-Górecki
> Sent: Sunday, June 5, 2022 11:40 PM
> To: xen-devel@lists.xenproject.org
> Cc: Marczykowski, Marek <marmarek@invisiblethingslab.com>; Cooper, Andrew
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Beulich, Jan <JBeulich@suse.com>; Julien Grall <julien@xen.org>; Stefano
> Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Pau Monné, Roger
> <roger.pau@citrix.com>; Paul Durrant <paul@xen.org>; Tian, Kevin
> <kevin.tian@intel.com>
> Subject: [RFC PATCH 00/12] Add Xue - console over USB 3 Debug Capability
> 
> This is integration of https://github.com/connojd/xue into mainline Xen.
> This patch series includes several patches that I made in the process, some are
> very loosely related.
> 
> The RFC status is to collect feedback on the shape of this series, specifically:
> 
> 1. The actual Xue driver is a header-only library. Most of the code is in a series of
> inline functions in xue.h. I kept it this way, to ease integrating Xue updates.
> That's also why I preserved its original code style. Is it okay, or should I move the
> code to a .c file?
> 
> 2. The xue.h file includes bindings for several other environments too (EFI, Linux,
> ...). This is unused code, behind #ifdef. Again, I kept it to ease updating. Should I
> remove it?
> 
> 3. The adding of IOMMU reserverd memory is necessary even if "hiding" device
> from dom0. Otherwise, VT-d will deny DMA. That's probably not the most
> elegant solution, but Xen doesn't have seem to have provisions for devices doing
> DMA into Xen's memory.
> 
> 4. To preserve authorship, I included unmodified "drivers/char: Add support for
> Xue USB3 debugger" commit from Connor, and only added my changes on top.
> This means, with that this commit, the driver doesn't work yet (but it does
> compile). Is it okay, or should I combine fixes into that commit and somehow
> mark authorship in the commit message?
> 
> 5. The last patch(es) enable using the controller by dom0, even when Xen uses
> DbC part. That's possible, because the capability was designed specifically to
> allow separate driver handle it, in parallel to unmodified xhci driver (separate set
> of registers, pretending the port is "disconnected" for the main xhci driver etc).
> It works with Linux dom0, although requires an awful hack - re-enabling bus
> mastering behind dom0's backs. Is it okay to leave this functionality as is, or
> guard it behind some cmdline option, or maybe remove completely?

Happy to see this effort, it's been long overdue to get this feature upstream! If you have a git branch somewhere I'm happy to test it out. I already have tested Xue before on my NUC and it was working well.

Thanks,
Tamas
Andrew Cooper June 6, 2022, 1:18 p.m. UTC | #2
On 06/06/2022 04:40, Marek Marczykowski-Górecki wrote:
> This is integration of https://github.com/connojd/xue into mainline Xen.
> This patch series includes several patches that I made in the process, some are
> very loosely related.

Thanks for looking into this.  CC'ing Connor just so he's aware.

>
> The RFC status is to collect feedback on the shape of this series, specifically:
>
> 1. The actual Xue driver is a header-only library. Most of the code is in a
> series of inline functions in xue.h. I kept it this way, to ease integrating
> Xue updates. That's also why I preserved its original code style. Is it okay,
> or should I move the code to a .c file?

It doesn't matter much if it's a .h or .c file.  It could perfectly
easily live as xen/drivers/char/xue.h and included only by xue.c.  (More
specifically, it doesn't want to live as xen/include/xue.h)

That said, as soon as you get to patch 2, it's no longer unmodified from
upstream, and with patch 3, we're gaining concrete functionality that
upstream doesn't have.

>
> 2. The xue.h file includes bindings for several other environments too (EFI,
> Linux, ...). This is unused code, behind #ifdef. Again, I kept it to ease
> updating. Should I remove it?

Drop please.  Xen is an embedded environment, and support other
environments is a waste of space and time.

I'm slowly ripping out other examples.

> 3. The adding of IOMMU reserverd memory is necessary even if "hiding" device
> from dom0. Otherwise, VT-d will deny DMA. That's probably not the most elegant
> solution, but Xen doesn't have seem to have provisions for devices doing DMA
> into Xen's memory.

I think that's to be expected, as the device should end up in quarantine.

That said, the model is broken for devices that Xen exclusively uses,
which includes IOMMU devices.  IOMMUs don't have any kind of applicable
IOMMU context, and things used exclusively by Xen don't want to be in
the general quarantine pool, because then all malicious devices can
interfere with the ringbuffer.

> 4. To preserve authorship, I included unmodified "drivers/char: Add support for
> Xue USB3 debugger" commit from Connor, and only added my changes on top. This
> means, with that this commit, the driver doesn't work yet (but it does
> compile). Is it okay, or should I combine fixes into that commit and somehow
> mark authorship in the commit message?

That depends on how much changes.  Other options are a dual SoB with
Connor still as the author (I typically do this for substantial code
movement, programmatic changes, etc), or for a major rewrite, changing
authorship and being very clear in the commit message where the code
originated.

> 5. The last patch(es) enable using the controller by dom0, even when Xen
> uses DbC part. That's possible, because the capability was designed
> specifically to allow separate driver handle it, in parallel to unmodified xhci
> driver (separate set of registers, pretending the port is "disconnected" for
> the main xhci driver etc). It works with Linux dom0, although requires an awful
> hack - re-enabling bus mastering behind dom0's backs. Is it okay to leave this
> functionality as is, or guard it behind some cmdline option, or maybe remove
> completely?

"Xen is configured to use USB3 debugging" is the only relevant signal. 
We do not want anything else.  If this triggers hacks for dom0, then fine.

OOI, how does the dual driver stack work in Linux?  At a minimum they've
surely got to coordinate device resets.

In an ideal world, dom0 would be fully unaware.  We hide the DbC
controls (so dom0 doesn't get any clever ideas), but we do need to keep
the device active when dom0 wants to reset (which will probably require
a fair chunk of emulation).

Connor did upstream code into Linux to cause it to ignore an
already-active DbC session.  I hope this will cause Linux to be duly
careful with resets, and is probably the easiest way forward.

~Andrew
Marek Marczykowski-Górecki June 6, 2022, 1:37 p.m. UTC | #3
On Mon, Jun 06, 2022 at 01:18:42PM +0000, Lengyel, Tamas wrote:
> Happy to see this effort, it's been long overdue to get this feature upstream! If you have a git branch somewhere I'm happy to test it out. I already have tested Xue before on my NUC and it was working well.

It's here:
https://github.com/marmarek/xen/tree/master-xue
warning: I do force-push to this branch from time to time
Marek Marczykowski-Górecki June 6, 2022, 1:56 p.m. UTC | #4
On Mon, Jun 06, 2022 at 01:18:45PM +0000, Andrew Cooper wrote:
> On 06/06/2022 04:40, Marek Marczykowski-Górecki wrote:
> > This is integration of https://github.com/connojd/xue into mainline Xen.
> > This patch series includes several patches that I made in the process, some are
> > very loosely related.
> 
> Thanks for looking into this.  CC'ing Connor just so he's aware.
> 
> >
> > The RFC status is to collect feedback on the shape of this series, specifically:
> >
> > 1. The actual Xue driver is a header-only library. Most of the code is in a
> > series of inline functions in xue.h. I kept it this way, to ease integrating
> > Xue updates. That's also why I preserved its original code style. Is it okay,
> > or should I move the code to a .c file?
> 
> It doesn't matter much if it's a .h or .c file.  It could perfectly
> easily live as xen/drivers/char/xue.h and included only by xue.c.  (More
> specifically, it doesn't want to live as xen/include/xue.h)
> 
> That said, as soon as you get to patch 2, it's no longer unmodified from
> upstream, and with patch 3, we're gaining concrete functionality that
> upstream doesn't have.
> 
> >
> > 2. The xue.h file includes bindings for several other environments too (EFI,
> > Linux, ...). This is unused code, behind #ifdef. Again, I kept it to ease
> > updating. Should I remove it?
> 
> Drop please.  Xen is an embedded environment, and support other
> environments is a waste of space and time.
> 
> I'm slowly ripping out other examples.

With both the above answers, I'll see how much work will be refactoring it
into Xen-only driver. Dropping other environments should make
xue_ops abstraction unnecessary, which should simplify the code quite a
bit.

> > 3. The adding of IOMMU reserverd memory is necessary even if "hiding" device
> > from dom0. Otherwise, VT-d will deny DMA. That's probably not the most elegant
> > solution, but Xen doesn't have seem to have provisions for devices doing DMA
> > into Xen's memory.
> 
> I think that's to be expected, as the device should end up in quarantine.
> 
> That said, the model is broken for devices that Xen exclusively uses,
> which includes IOMMU devices.  IOMMUs don't have any kind of applicable
> IOMMU context, and things used exclusively by Xen don't want to be in
> the general quarantine pool, because then all malicious devices can
> interfere with the ringbuffer.

That's yet another reason for assigning it to dom0... this way, only
dom0(-assigned devices) can interfere with the ringbuffer. That's still
sub-optimal, but the current granularity of IOMMU configuration in Xen
doesn't allow to do any better.
I'll drop patch 11.

> > 4. To preserve authorship, I included unmodified "drivers/char: Add support for
> > Xue USB3 debugger" commit from Connor, and only added my changes on top. This
> > means, with that this commit, the driver doesn't work yet (but it does
> > compile). Is it okay, or should I combine fixes into that commit and somehow
> > mark authorship in the commit message?
> 
> That depends on how much changes.  Other options are a dual SoB with
> Connor still as the author (I typically do this for substantial code
> movement, programmatic changes, etc), or for a major rewrite, changing
> authorship and being very clear in the commit message where the code
> originated.

If I'll go with the refactor to get rid of xue_ops, then indeed it makes
more sense to create new commit and reference code origin in the commit
message.

> > 5. The last patch(es) enable using the controller by dom0, even when Xen
> > uses DbC part. That's possible, because the capability was designed
> > specifically to allow separate driver handle it, in parallel to unmodified xhci
> > driver (separate set of registers, pretending the port is "disconnected" for
> > the main xhci driver etc). It works with Linux dom0, although requires an awful
> > hack - re-enabling bus mastering behind dom0's backs. Is it okay to leave this
> > functionality as is, or guard it behind some cmdline option, or maybe remove
> > completely?
> 
> "Xen is configured to use USB3 debugging" is the only relevant signal. 
> We do not want anything else.  If this triggers hacks for dom0, then fine.

I'm worried here about depending on specific dom0 behavior. With the
current Linux driver, I needed just the bus mastering hack, but since in
this case dom0 has more or less full control over the controller, there
could be other ways it could disrupt DbC in the future.

> OOI, how does the dual driver stack work in Linux?  At a minimum they've
> surely got to coordinate device resets.

Kind of. The DbC driver (both Linux and Xue) checks if nothing hasn't
disabled DbC in the meantime (for example via device reset). When it
happens, it re-enables it back.
I haven't tried what happens if I try to enable DbC both in Xen and
Linux at the same time, but one of the possibilities is spectacular
explosion. (In theory Xen should win, since I make DbC related MMIO area
read-only.)

> In an ideal world, dom0 would be fully unaware.  We hide the DbC
> controls (so dom0 doesn't get any clever ideas), but we do need to keep
> the device active when dom0 wants to reset (which will probably require
> a fair chunk of emulation).

Yeah, I don't want to go the "emulate almost-reset" way...

> Connor did upstream code into Linux to cause it to ignore an
> already-active DbC session.  I hope this will cause Linux to be duly
> careful with resets, and is probably the easiest way forward.

Not really, see above.

At least it doesn't try to explicitly disable it.