[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH 00/12] Add Xue - console over USB 3 Debug Capability


  • To: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
  • Date: Mon, 6 Jun 2022 13:18:45 +0000
  • Accept-language: en-GB, en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=HoN4d8nMMn+mRNEtlPcCAAQtSQmw+GmljetBKUYnKPs=; b=F/G747CHGteW6x+OsTnJ5s08WWFAajcCflr+qdp4GAOri8WEHcFFv0l1l7sLavIe+fNswJUWO8o4lbYDD0ifDI85J6zH8aMbplU+60SV9dUCwaHwiwKs3eQaniAO6N2HEYqn46hIGkBmDTZi7qTf4c5zGRZzli43L0k3LCXcXQttp1OJwuX6lMlsUaY0l9gDtWlybYlIp3Oxsl2yTulooUjcJBkH4LuxtJwPr/1RvaSMf/1TXvVCSHwFScdbhrM6lESzpGKfu/R+mHML35Tg/lSTNa/USlCIRvWfhZegdvgqHGUga16Kay3tYckpOkK51sZz/goO2JJXEs5qLBltwA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=blSG7GesJIm5gHtS3GhmP8gf84T8tlPqhIgOdcUEnt3jbOhaKbb6wh+pWesVqcfWFYyLPwk3S9p0rT5apmbebya+pbPdYOf5uP+CEvjjs4WafoYwapek4r+NGN+qJm4H1x2QWEHMQ8hCCIcSugFMvqKNbQETdlPTF8V4pbxvB2//+63DsrfGEfozppqiGNOcr7NyTy8aIH/ey2lTP+w2oe1TW2K7pcVoKi83Tt56QJq9OukXtc2jERBxaGzQ53xGpXz8RChaM+A2nVBpEzyOBovW/6V+czxzyxQAz8wTSotmi91SPWR6q/QvAsMpnUNA075p4mMqMVCa8tTAfIx/6g==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
  • Cc: George Dunlap <George.Dunlap@xxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Roger Pau Monne <roger.pau@xxxxxxxxxx>, Paul Durrant <paul@xxxxxxx>, Kevin Tian <kevin.tian@xxxxxxxxx>, Connor Davis <connojdavis@xxxxxxxxx>
  • Delivery-date: Mon, 06 Jun 2022 13:19:12 +0000
  • Ironport-data: A9a23:EtfUDqxLoS32tP4DVPt6t+fIxyrEfRIJ4+MujC+fZmUNrF6WrkUPx 2MaUG3XMqmMNDf2f9F3Po7j9EgB7ZXXyYUwGQZuqyAxQypGp/SeCIXCJC8cHc8zwu4v7q5Dx 59DAjUVBJlsFhcwnj/0bv656yMUOZigHtIQMsadUsxKbVIiGX5JZS5LwbZj2NY22YXhWWthh PupyyHhEA79s9JLGjp8B5Kr8HuDa9yr5Vv0FnRnDRx6lAe2e0s9VfrzFonoR5fMeaFGH/bSe gr25OrRElU1XfsaIojNfr7TKiXmS1NJVOSEoiI+t6OK2nCuqsGuu0qS2TV1hUp/0l20c95NJ Npll5idUwEyGLD1ussxQTZ4Nn5EBYlL9+qSSZS/mZT7I0zuVVLJmq0rJmdpeIoS96BwHH1E8 uEeJHYVdBefiumqwbW9DO5xmsAkK8qtN4Qa0p1i5WiBUbB6HtacG+OTvYAwMDQY36iiGd73Y cYDZCUpRxPHexBVYX8cCY4knffujX76G9FdgA3P//ttvTeMpOB3+Ir1PtiJINWwf4YLwV+U9 mnjzk/DJChPYbRzzhLAqBpAnNTnmCrhXYsIGb6Q9/h0gUaSzGgeFB0XU1SgpfCzzEW5Xrp3O 0ESvyYjs6U23EiqVcXmGQ21pmaeuRwRUMYWFPc1gCmPwKfJ5weSBkAfUyVMLtchsaceRyEu1 1KPt8PkA3poqrL9YWmG6r6eoDe2OC4UBWwPfykJSU0C+daLiJ43pgLCSJBkCqHdpsX8BDXY0 z2M6i8kiN07jsMV1qP94VHOhRqtoITESkg+4QC/dmmi6AV+ZoKseY2zwVfe5PdEao2eSzG8U GMsnsGf6KUEC86LnSnUGOEVRujxuLCCLSHWhkNpE9857TOx9nW/fIdWpjZjOENuNcVCcjjsC KPOhT5sCFZoFCPCRcdKj0iZUazGEYCI+QzZa83p
  • Ironport-hdrordr: A9a23:v6szGqjCT9HmMYSNB0lqtAH753BQX4V23DAbv31ZSRFFG/FwyP rCoB1L73XJYWgqM03IwerwQ5VpQRvnhP1ICRF4B8bsYOCUghrTEGgE1/qt/9SAIVyzygc578 tdmsdFebrN5DRB7PoSpTPIa+rIo+P3sZxA592uqUuFJDsCA84P0+46MHfjLqQcfnglOXNNLu v52iMxnUvERZ14VKSGL0hAe9KGi8zAlZrgbxJDLQUg8hOygTSh76O/OwSE3z8FOgk/gosKwC zgqUjU96+ju/a0xlv3zGnI9albn9Pn159qGNGMsM4IMT/h4zzYJriJGofy+Qzdktvfr2rCo+ O85SvI+P4Dsk85S1vF5ScFHTOQiArGpUWSkmNwykGT3PARDAhKd/apw7gpMicxonBQwu2Vms hwrh2knosSAhXakCvn4d/UExlsi0qvuHIn1fUelnpFTOIlGfRsRCMkjTFo+bo7bWvHAbocYa FT5QDnlYNrWELfa2qcsnhkwdSqUHh2FhCaQlIassjQ1zRNhnh2w0YR2cRaxx47hd8AYogB4/ 6BPrVjlblIQMNTZaVhBP0ZSc/yDmDWWxrDPG+bPFyiHqAaPHDGrYLx/dwOlauXUY1NyIF3lI XKUVteu2J3c0XyCdeW1JkO6RzJSHXVZ0Wa9iif3ekPhlTRfsuaDcTYciFeryKJmYRtPuTLH/ CuJZlRH/jvaWPzBIch5XyLZ6Vv
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHYeVc0shRplu45PUy7PQfpSV53Bq1CXTeA
  • Thread-topic: [RFC PATCH 00/12] Add Xue - console over USB 3 Debug Capability

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.