* Re: [virtio-dev] Re: [Qemu-devel] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Siwei Liu @ 2018-06-22 20:00 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, konrad.wilk,
Jakub Kicinski, Samudrala, Sridhar, Cornelia Huck, qemu-devel,
virtualization, Venu Busireddy, Netdev, boris.ostrovsky,
aaron.f.brown, Joao Martins
In-Reply-To: <20180622053141-mutt-send-email-mst@kernel.org>
On Thu, Jun 21, 2018 at 7:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Jun 21, 2018 at 06:21:55PM -0700, Siwei Liu wrote:
>> On Thu, Jun 21, 2018 at 7:59 AM, Cornelia Huck <cohuck@redhat.com> wrote:
>> > On Wed, 20 Jun 2018 22:48:58 +0300
>> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >
>> >> On Wed, Jun 20, 2018 at 06:06:19PM +0200, Cornelia Huck wrote:
>> >> > In any case, I'm not sure anymore why we'd want the extra uuid.
>> >>
>> >> It's mostly so we can have e.g. multiple devices with same MAC
>> >> (which some people seem to want in order to then use
>> >> then with different containers).
>> >>
>> >> But it is also handy for when you assign a PF, since then you
>> >> can't set the MAC.
>> >>
>> >
>> > OK, so what about the following:
>> >
>> > - introduce a new feature bit, VIRTIO_NET_F_STANDBY_UUID that indicates
>> > that we have a new uuid field in the virtio-net config space
>> > - in QEMU, add a property for virtio-net that allows to specify a uuid,
>> > offer VIRTIO_NET_F_STANDBY_UUID if set
>> > - when configuring, set the property to the group UUID of the vfio-pci
>> > device
>>
>> If feature negotiation fails on VIRTIO_NET_F_STANDBY_UUID, is it safe
>> to still expose UUID in the config space on virtio-pci?
>
>
> Yes but guest is not supposed to read it.
>
>> I'm not even sure if it's sane to expose group UUID on the PCI bridge
>> where the corresponding vfio-pci device attached to for a guest which
>> doesn't support the feature (legacy).
>>
>> -Siwei
>
> Yes but you won't add the primary behind such a bridge.
I assume the UUID feature is a new one besides the exiting
VIRTIO_NET_F_STANDBY feature, where I think the current proposal is,
if UUID feature is present and supported by guest, we'll do pairing
based on UUID; if UUID feature present or not supported by guest,
we'll still plug in the VF (if guest supports the _F_STANDBY feature)
but the pairing will be fallback to using MAC address. Are you saying
you don't even want to plug in the primary when the UUID feature is
not supported by guest? Does the feature negotiation UUID have to
depend on STANDBY being set, or the other way around? I thought that
just the absence of STANDBY will prevent primary to get exposed to the
guest.
-Siwei
>
>>
>> > - in the guest, use the uuid from the virtio-net device's config space
>> > if applicable; else, fall back to matching by MAC as done today
>> >
>> > That should work for all virtio transports.
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Siwei Liu @ 2018-06-22 19:43 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alexander Duyck, virtio-dev, qemu-devel, Samudrala, Sridhar,
virtualization
In-Reply-To: <20180622052717-mutt-send-email-mst@kernel.org>
On Thu, Jun 21, 2018 at 7:30 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Jun 21, 2018 at 06:07:18PM -0700, Siwei Liu wrote:
>> On Thu, Jun 21, 2018 at 11:14 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Wed, Jun 13, 2018 at 01:40:59PM +0800, Jason Wang wrote:
>> >>
>> >>
>> >> On 2018年06月13日 12:24, Samudrala, Sridhar wrote:
>> >> > On 6/12/2018 7:38 PM, Jason Wang wrote:
>> >> > >
>> >> > >
>> >> > > On 2018年06月12日 19:54, Michael S. Tsirkin wrote:
>> >> > > > On Wed, Jun 06, 2018 at 10:29:03AM +0800, Jason Wang wrote:
>> >> > > > >
>> >> > > > > On 2018年06月05日 20:33, Michael S. Tsirkin wrote:
>> >> > > > > > I don't think this is sufficient.
>> >> > > > > >
>> >> > > > > > If both primary and standby devices are present, a
>> >> > > > > > legacy guest without
>> >> > > > > > support for the feature might see two devices with same mac and get
>> >> > > > > > confused.
>> >> > > > > >
>> >> > > > > > I think that we should only make primary visible after
>> >> > > > > > guest acked the
>> >> > > > > > backup feature bit.
>> >> > > > > I think we want exactly the reverse? E.g fail the
>> >> > > > > negotiation when guest
>> >> > > > > does not ack backup feature.
>> >> > > > >
>> >> > > > > Otherwise legacy guest won't even have the chance to see
>> >> > > > > primary device in
>> >> > > > > the guest.
>> >> > > > That's by design.
>> >> > >
>> >> > > So management needs to know the capability of guest to set the
>> >> > > backup feature. This looks a chicken or egg problem to me.
>> >> >
>> >> > I don't think so. If the tenant requests 'accelerated datapath feature',
>> >> > the management
>> >> > will set 'standby' feature bit on virtio-net interface and if the guest
>> >> > virtio-net driver
>> >> > supports this feature, then the tenant VM will get that capability via a
>> >> > hot-plugged
>> >> > primary device.
>> >>
>> >> Ok, I thought exactly the reverse because of the commit title is "enable
>> >> virtio_net to act as a standby for a passthru device". But re-read the
>> >> commit log content, I understand the case a little bit. Btw, VF is not
>> >> necessarily faster than virtio-net, especially consider virtio-net may have
>> >> a lot of queues.
>> >
>> > Don't do that then, right?
>>
>> I don't understand. Where did the standby feature come to imply the
>> "accelerated datapath" thing?
>> Isn't failover/standby a generic high
>> availblity term, rather than marry it to the concept of device model
>> specifics? Do we expect scsi to work exactly the same way with
>> "accelerated datapath"?
>
> That's not what I said.
> The semantics are that the primary is always used if present in
> preference to standby.
OK. If this is the only semantics of what "standby" refers to in
general, that is fine.
I just don't want to limit the failover/standby semantics to the
device model specifics, the "accelerated datapath" thing or whatever.
I really don't know where the requirements of the "accelerated
datapath" came from, as the originial problem is migrating vfio
devices which is in match of QEMU's live migration model. Hyper-V has
it's limitation to do 1-netdev should not impact how KVM/QEMU should
be doing it.
> Jason said virtio net is sometimes preferable.
> If that's the case don't make it a standby.
>
> More advanced use-cases do exist and e.g. Alexander Duyck
> suggested using a switch-dev.
The switchdev case would need a new feature bit, right?
-Siwei
> failover isn't it though.
>
> --
> MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [virtio-dev] Re: [Qemu-devel] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Michael S. Tsirkin @ 2018-06-22 19:05 UTC (permalink / raw)
To: Cornelia Huck
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, konrad.wilk,
Jakub Kicinski, Samudrala, Sridhar, qemu-devel, virtualization,
Siwei Liu, Venu Busireddy, Netdev, boris.ostrovsky, aaron.f.brown,
Joao Martins
In-Reply-To: <20180622170955.298900c1.cohuck@redhat.com>
On Fri, Jun 22, 2018 at 05:09:55PM +0200, Cornelia Huck wrote:
> On Thu, 21 Jun 2018 21:20:13 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Thu, Jun 21, 2018 at 04:59:13PM +0200, Cornelia Huck wrote:
> > > OK, so what about the following:
> > >
> > > - introduce a new feature bit, VIRTIO_NET_F_STANDBY_UUID that indicates
> > > that we have a new uuid field in the virtio-net config space
> > > - in QEMU, add a property for virtio-net that allows to specify a uuid,
> > > offer VIRTIO_NET_F_STANDBY_UUID if set
> > > - when configuring, set the property to the group UUID of the vfio-pci
> > > device
> > > - in the guest, use the uuid from the virtio-net device's config space
> > > if applicable; else, fall back to matching by MAC as done today
> > >
> > > That should work for all virtio transports.
> >
> > True. I'm a bit unhappy that it's virtio net specific though
> > since down the road I expect we'll have a very similar feature
> > for scsi (and maybe others).
> >
> > But we do not have a way to have fields that are portable
> > both across devices and transports, and I think it would
> > be a useful addition. How would this work though? Any idea?
>
> Can we introduce some kind of device-independent config space area?
> Pushing back the device-specific config space by a certain value if the
> appropriate feature is negotiated and use that for things like the uuid?
So config moves back and forth?
Reminds me of the msi vector mess we had with pci.
I'd rather have every transport add a new config.
> But regardless of that, I'm not sure whether extending this approach to
> other device types is the way to go. Tying together two different
> devices is creating complicated situations at least in the hypervisor
> (even if it's fairly straightforward in the guest). [I have not come
> around again to look at the "how to handle visibility in QEMU"
> questions due to lack of cycles, sorry about that.]
>
> So, what's the goal of this approach? Only to allow migration with
> vfio-pci, or also to plug in a faster device and use it instead of an
> already attached paravirtualized device?
These are two sides of the same coin, I think the second approach
is closer to what we are doing here.
> What about migration of vfio devices that are not easily replaced by a
> paravirtualized device? I'm thinking of vfio-ccw, where our main (and
> currently only) supported device is dasd (disks) -- which can do a lot
> of specialized things that virtio-blk does not support (and should not
> or even cannot support).
But maybe virtio-scsi can?
> Would it be more helpful to focus on generic
> migration support for vfio instead of going about it device by device?
>
> This network device approach already seems far along, so it makes sense
> to continue with it. But I'm not sure whether we want to spend time and
> energy on that for other device types rather than working on a general
> solution for vfio migration.
I'm inclined to say finalizing this feature would be a good first step
and will teach us how we can move forward.
--
MST
^ permalink raw reply
* Re: [virtio-dev] Re: [Qemu-devel] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Cornelia Huck @ 2018-06-22 15:09 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, konrad.wilk,
Jakub Kicinski, Samudrala, Sridhar, qemu-devel, virtualization,
Siwei Liu, Venu Busireddy, Netdev, boris.ostrovsky, aaron.f.brown,
Joao Martins
In-Reply-To: <20180621211712-mutt-send-email-mst@kernel.org>
On Thu, 21 Jun 2018 21:20:13 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Jun 21, 2018 at 04:59:13PM +0200, Cornelia Huck wrote:
> > OK, so what about the following:
> >
> > - introduce a new feature bit, VIRTIO_NET_F_STANDBY_UUID that indicates
> > that we have a new uuid field in the virtio-net config space
> > - in QEMU, add a property for virtio-net that allows to specify a uuid,
> > offer VIRTIO_NET_F_STANDBY_UUID if set
> > - when configuring, set the property to the group UUID of the vfio-pci
> > device
> > - in the guest, use the uuid from the virtio-net device's config space
> > if applicable; else, fall back to matching by MAC as done today
> >
> > That should work for all virtio transports.
>
> True. I'm a bit unhappy that it's virtio net specific though
> since down the road I expect we'll have a very similar feature
> for scsi (and maybe others).
>
> But we do not have a way to have fields that are portable
> both across devices and transports, and I think it would
> be a useful addition. How would this work though? Any idea?
Can we introduce some kind of device-independent config space area?
Pushing back the device-specific config space by a certain value if the
appropriate feature is negotiated and use that for things like the uuid?
But regardless of that, I'm not sure whether extending this approach to
other device types is the way to go. Tying together two different
devices is creating complicated situations at least in the hypervisor
(even if it's fairly straightforward in the guest). [I have not come
around again to look at the "how to handle visibility in QEMU"
questions due to lack of cycles, sorry about that.]
So, what's the goal of this approach? Only to allow migration with
vfio-pci, or also to plug in a faster device and use it instead of an
already attached paravirtualized device?
What about migration of vfio devices that are not easily replaced by a
paravirtualized device? I'm thinking of vfio-ccw, where our main (and
currently only) supported device is dasd (disks) -- which can do a lot
of specialized things that virtio-blk does not support (and should not
or even cannot support). Would it be more helpful to focus on generic
migration support for vfio instead of going about it device by device?
This network device approach already seems far along, so it makes sense
to continue with it. But I'm not sure whether we want to spend time and
energy on that for other device types rather than working on a general
solution for vfio migration.
^ permalink raw reply
* Re: [virtio-dev] Re: [Qemu-devel] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Michael S. Tsirkin @ 2018-06-22 2:32 UTC (permalink / raw)
To: Siwei Liu
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, konrad.wilk,
Jakub Kicinski, Samudrala, Sridhar, Cornelia Huck, qemu-devel,
virtualization, Venu Busireddy, Netdev, boris.ostrovsky,
aaron.f.brown, Joao Martins
In-Reply-To: <CADGSJ22B39JiS14L0yCYQ720-fZcwyUt1wCX1YO-6Y9FhmDKZg@mail.gmail.com>
On Thu, Jun 21, 2018 at 06:21:55PM -0700, Siwei Liu wrote:
> On Thu, Jun 21, 2018 at 7:59 AM, Cornelia Huck <cohuck@redhat.com> wrote:
> > On Wed, 20 Jun 2018 22:48:58 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> >> On Wed, Jun 20, 2018 at 06:06:19PM +0200, Cornelia Huck wrote:
> >> > In any case, I'm not sure anymore why we'd want the extra uuid.
> >>
> >> It's mostly so we can have e.g. multiple devices with same MAC
> >> (which some people seem to want in order to then use
> >> then with different containers).
> >>
> >> But it is also handy for when you assign a PF, since then you
> >> can't set the MAC.
> >>
> >
> > OK, so what about the following:
> >
> > - introduce a new feature bit, VIRTIO_NET_F_STANDBY_UUID that indicates
> > that we have a new uuid field in the virtio-net config space
> > - in QEMU, add a property for virtio-net that allows to specify a uuid,
> > offer VIRTIO_NET_F_STANDBY_UUID if set
> > - when configuring, set the property to the group UUID of the vfio-pci
> > device
>
> If feature negotiation fails on VIRTIO_NET_F_STANDBY_UUID, is it safe
> to still expose UUID in the config space on virtio-pci?
Yes but guest is not supposed to read it.
> I'm not even sure if it's sane to expose group UUID on the PCI bridge
> where the corresponding vfio-pci device attached to for a guest which
> doesn't support the feature (legacy).
>
> -Siwei
Yes but you won't add the primary behind such a bridge.
>
> > - in the guest, use the uuid from the virtio-net device's config space
> > if applicable; else, fall back to matching by MAC as done today
> >
> > That should work for all virtio transports.
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Michael S. Tsirkin @ 2018-06-22 2:30 UTC (permalink / raw)
To: Siwei Liu
Cc: Alexander Duyck, virtio-dev, qemu-devel, Samudrala, Sridhar,
virtualization
In-Reply-To: <CADGSJ216itm89dUuK_vOfuBsBq5bq4485GqMKjB1JXFD_PLZxA@mail.gmail.com>
On Thu, Jun 21, 2018 at 06:07:18PM -0700, Siwei Liu wrote:
> On Thu, Jun 21, 2018 at 11:14 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Wed, Jun 13, 2018 at 01:40:59PM +0800, Jason Wang wrote:
> >>
> >>
> >> On 2018年06月13日 12:24, Samudrala, Sridhar wrote:
> >> > On 6/12/2018 7:38 PM, Jason Wang wrote:
> >> > >
> >> > >
> >> > > On 2018年06月12日 19:54, Michael S. Tsirkin wrote:
> >> > > > On Wed, Jun 06, 2018 at 10:29:03AM +0800, Jason Wang wrote:
> >> > > > >
> >> > > > > On 2018年06月05日 20:33, Michael S. Tsirkin wrote:
> >> > > > > > I don't think this is sufficient.
> >> > > > > >
> >> > > > > > If both primary and standby devices are present, a
> >> > > > > > legacy guest without
> >> > > > > > support for the feature might see two devices with same mac and get
> >> > > > > > confused.
> >> > > > > >
> >> > > > > > I think that we should only make primary visible after
> >> > > > > > guest acked the
> >> > > > > > backup feature bit.
> >> > > > > I think we want exactly the reverse? E.g fail the
> >> > > > > negotiation when guest
> >> > > > > does not ack backup feature.
> >> > > > >
> >> > > > > Otherwise legacy guest won't even have the chance to see
> >> > > > > primary device in
> >> > > > > the guest.
> >> > > > That's by design.
> >> > >
> >> > > So management needs to know the capability of guest to set the
> >> > > backup feature. This looks a chicken or egg problem to me.
> >> >
> >> > I don't think so. If the tenant requests 'accelerated datapath feature',
> >> > the management
> >> > will set 'standby' feature bit on virtio-net interface and if the guest
> >> > virtio-net driver
> >> > supports this feature, then the tenant VM will get that capability via a
> >> > hot-plugged
> >> > primary device.
> >>
> >> Ok, I thought exactly the reverse because of the commit title is "enable
> >> virtio_net to act as a standby for a passthru device". But re-read the
> >> commit log content, I understand the case a little bit. Btw, VF is not
> >> necessarily faster than virtio-net, especially consider virtio-net may have
> >> a lot of queues.
> >
> > Don't do that then, right?
>
> I don't understand. Where did the standby feature come to imply the
> "accelerated datapath" thing?
> Isn't failover/standby a generic high
> availblity term, rather than marry it to the concept of device model
> specifics? Do we expect scsi to work exactly the same way with
> "accelerated datapath"?
That's not what I said.
The semantics are that the primary is always used if present in
preference to standby.
Jason said virtio net is sometimes preferable.
If that's the case don't make it a standby.
More advanced use-cases do exist and e.g. Alexander Duyck
suggested using a switch-dev. failover isn't it though.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v6 3/3] x86: paravirt: make native_save_fl extern inline
From: Ingo Molnar @ 2018-06-22 2:24 UTC (permalink / raw)
To: Nick Desaulniers
Cc: kstewart, andrea.parri, linux-efi, brijesh.singh, jan.kiszka,
jpoimboe, will.deacon, jarkko.sakkinen, virtualization,
yamada.masahiro, manojgupta, hpa, boris.ostrovsky, tweek,
mawilcox, x86, akataria, ghackmann, mingo, astrachan, rientjes,
geert, thomas.lendacky, arnd, linux-kbuild, pombredanne, rostedt,
acme, caoj.fnst, aryabinin, sedat.dilek, tglx, jgross,
michal.lkml, tstellar, gregkh, mka, ard.bieshe
In-Reply-To: <20180621162324.36656-4-ndesaulniers@google.com>
* Nick Desaulniers <ndesaulniers@google.com> wrote:
> native_save_fl() is marked static inline, but by using it as
> a function pointer in arch/x86/kernel/paravirt.c, it MUST be outlined.
> --- a/arch/x86/include/asm/irqflags.h
> +++ b/arch/x86/include/asm/irqflags.h
> @@ -13,7 +13,7 @@
> * Interrupt control:
> */
>
> -static inline unsigned long native_save_fl(void)
> +extern inline unsigned long native_save_fl(void)
> {
> unsigned long flags;
>
What's the code generation effect of this on say a defconfig kernel vmlinux with
paravirt enabled?
Thanks,
Ingo
^ permalink raw reply
* Re: [virtio-dev] Re: [Qemu-devel] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Siwei Liu @ 2018-06-22 1:21 UTC (permalink / raw)
To: Cornelia Huck
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, Michael S. Tsirkin,
Jakub Kicinski, Samudrala, Sridhar, konrad.wilk, qemu-devel,
virtualization, Venu Busireddy, Netdev, boris.ostrovsky,
aaron.f.brown, Joao Martins
In-Reply-To: <20180621165913.7e3f4faa.cohuck@redhat.com>
On Thu, Jun 21, 2018 at 7:59 AM, Cornelia Huck <cohuck@redhat.com> wrote:
> On Wed, 20 Jun 2018 22:48:58 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> On Wed, Jun 20, 2018 at 06:06:19PM +0200, Cornelia Huck wrote:
>> > In any case, I'm not sure anymore why we'd want the extra uuid.
>>
>> It's mostly so we can have e.g. multiple devices with same MAC
>> (which some people seem to want in order to then use
>> then with different containers).
>>
>> But it is also handy for when you assign a PF, since then you
>> can't set the MAC.
>>
>
> OK, so what about the following:
>
> - introduce a new feature bit, VIRTIO_NET_F_STANDBY_UUID that indicates
> that we have a new uuid field in the virtio-net config space
> - in QEMU, add a property for virtio-net that allows to specify a uuid,
> offer VIRTIO_NET_F_STANDBY_UUID if set
> - when configuring, set the property to the group UUID of the vfio-pci
> device
If feature negotiation fails on VIRTIO_NET_F_STANDBY_UUID, is it safe
to still expose UUID in the config space on virtio-pci?
I'm not even sure if it's sane to expose group UUID on the PCI bridge
where the corresponding vfio-pci device attached to for a guest which
doesn't support the feature (legacy).
-Siwei
> - in the guest, use the uuid from the virtio-net device's config space
> if applicable; else, fall back to matching by MAC as done today
>
> That should work for all virtio transports.
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Siwei Liu @ 2018-06-22 1:07 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alexander Duyck, virtio-dev, qemu-devel, Samudrala, Sridhar,
virtualization
In-Reply-To: <20180621211359-mutt-send-email-mst@kernel.org>
On Thu, Jun 21, 2018 at 11:14 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Jun 13, 2018 at 01:40:59PM +0800, Jason Wang wrote:
>>
>>
>> On 2018年06月13日 12:24, Samudrala, Sridhar wrote:
>> > On 6/12/2018 7:38 PM, Jason Wang wrote:
>> > >
>> > >
>> > > On 2018年06月12日 19:54, Michael S. Tsirkin wrote:
>> > > > On Wed, Jun 06, 2018 at 10:29:03AM +0800, Jason Wang wrote:
>> > > > >
>> > > > > On 2018年06月05日 20:33, Michael S. Tsirkin wrote:
>> > > > > > I don't think this is sufficient.
>> > > > > >
>> > > > > > If both primary and standby devices are present, a
>> > > > > > legacy guest without
>> > > > > > support for the feature might see two devices with same mac and get
>> > > > > > confused.
>> > > > > >
>> > > > > > I think that we should only make primary visible after
>> > > > > > guest acked the
>> > > > > > backup feature bit.
>> > > > > I think we want exactly the reverse? E.g fail the
>> > > > > negotiation when guest
>> > > > > does not ack backup feature.
>> > > > >
>> > > > > Otherwise legacy guest won't even have the chance to see
>> > > > > primary device in
>> > > > > the guest.
>> > > > That's by design.
>> > >
>> > > So management needs to know the capability of guest to set the
>> > > backup feature. This looks a chicken or egg problem to me.
>> >
>> > I don't think so. If the tenant requests 'accelerated datapath feature',
>> > the management
>> > will set 'standby' feature bit on virtio-net interface and if the guest
>> > virtio-net driver
>> > supports this feature, then the tenant VM will get that capability via a
>> > hot-plugged
>> > primary device.
>>
>> Ok, I thought exactly the reverse because of the commit title is "enable
>> virtio_net to act as a standby for a passthru device". But re-read the
>> commit log content, I understand the case a little bit. Btw, VF is not
>> necessarily faster than virtio-net, especially consider virtio-net may have
>> a lot of queues.
>
> Don't do that then, right?
I don't understand. Where did the standby feature come to imply the
"accelerated datapath" thing? Isn't failover/standby a generic high
availblity term, rather than marry it to the concept of device model
specifics? Do we expect scsi to work exactly the same way with
"accelerated datapath"?
-Siwei
>
>> >
>> >
>> > >
>> > > >
>> > > > > > And on reset or when backup is cleared in some other way, unplug the
>> > > > > > primary.
>> > > > > What if guest does not support hotplug?
>> > > > It shouldn't ack the backup feature then and it's a good point.
>> > > > We should both document this and check kernel config has
>> > > > hotplug support. Sridhar could you take a look pls?
>> > > >
>> > > > > > Something like the below will do the job:
>> > > > > >
>> > > > > > Primary device is added with a special "primary-failover" flag.
>> > > > > > A virtual machine is then initialized with just a standby virtio
>> > > > > > device. Primary is not yet added.
>> > > > > A question is how to do the matching? Qemu knows nothing about e.g mac
>> > > > > address of a pass-through device I believe?
>> > > > Supply a flag to the VFIO when it's added, this way QEMU will know.
>> > > >
>> > > > > > Later QEMU detects that guest driver device set DRIVER_OK.
>> > > > > > It then exposes the primary device to the guest, and triggers
>> > > > > > a device addition event (hot-plug event) for it.
>> > > > > Do you mean we won't have primary device in the initial qemu cli?
>> > > > No, that's not what I mean.
>> > > >
>> > > > I mean management will supply a flag to VFIO and then
>> > > >
>> > > >
>> > > > - VFIO defers exposing
>> > > > primary to guest until guest acks the feature bit.
>> > > > - When we see guest ack, initiate hotplug.
>> > > > - On reboot, hide it again.
>> > > > - On reset without reboot, request hot-unplug and on eject hide
>> > > > it again.
>> > >
>> > > This sounds much like a kind of bonding in qemu.
>> > >
>> > > >
>> > > > > > If QEMU detects guest driver removal, it initiates a
>> > > > > > hot-unplug sequence
>> > > > > > to remove the primary driver. In particular, if QEMU detects guest
>> > > > > > re-initialization (e.g. by detecting guest reset) it
>> > > > > > immediately removes
>> > > > > > the primary device.
>> > > > > I believe guest failover module should handle this gracefully?
>> > > > It can't control enumeration order, if primary is enumerated before
>> > > > standby then guest will load its driver and it's too late
>> > > > when failover driver is loaded.
>> > >
>> > > Well, even if we can delay the visibility of primary until
>> > > DRIVER_OK, there still be a race I think? And it looks to me it's
>> > > still a bug of guest:
>> > >
>> > > E.g primary could be probed before failover_register() in guest.
>> > > Then we will miss the enslaving of primary forever.
>> >
>> > That is not an issue. Even if the primary is probed before failover
>> > driver, it will
>> > enslave the primary via the call to failover_existing_slave_register()
>> > as part of
>> > failover_register() routine.
>>
>> Aha I get it. So the enumeration order is not an issue.
>>
>> Consider primary may still be seen by guest kernel even if we delay its
>> visibility, I wonder whether we can control the lifecycle of primary through
>> driver but not qemu. This can simplify a lot of things.
>>
>> Thanks
>>
>> >
>> > >
>> > > Thanks
>> > >
>> > > >
>> > > > > Thanks
>> > > > >
>> > > > > > We can move some of this code to management as well,
>> > > > > > architecturally it
>> > > > > > does not make too much sense but it might be easier
>> > > > > > implementation-wise.
>> > > > > >
>> > > > > > HTH
>> > > > > >
>> > > > > > On Mon, Jun 04, 2018 at 06:41:48PM -0700, Samudrala, Sridhar wrote:
>> > > > > > > Ping on this patch now that the kernel patches are
>> > > > > > > accepted into davem's net-next tree.
>> > > > > > > https://patchwork.ozlabs.org/cover/920005/
>> > > > > > >
>> > > > > > >
>> > > > > > > On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:
>> > > > > > > > This feature bit can be used by hypervisor to
>> > > > > > > > indicate virtio_net device to
>> > > > > > > > act as a standby for another device with the same MAC address.
>> > > > > > > >
>> > > > > > > > I tested this with a small change to the patch
>> > > > > > > > to mark the STANDBY feature 'true'
>> > > > > > > > by default as i am using libvirt to start the VMs.
>> > > > > > > > Is there a way to pass the newly added feature
>> > > > > > > > bit 'standby' to qemu via libvirt
>> > > > > > > > XML file?
>> > > > > > > >
>> > > > > > > > Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>> > > > > > > > ---
>> > > > > > > > hw/net/virtio-net.c | 2 ++
>> > > > > > > > include/standard-headers/linux/virtio_net.h | 3 +++
>> > > > > > > > 2 files changed, 5 insertions(+)
>> > > > > > > >
>> > > > > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> > > > > > > > index 90502fca7c..38b3140670 100644
>> > > > > > > > --- a/hw/net/virtio-net.c
>> > > > > > > > +++ b/hw/net/virtio-net.c
>> > > > > > > > @@ -2198,6 +2198,8 @@ static Property virtio_net_properties[] = {
>> > > > > > > > true),
>> > > > > > > > DEFINE_PROP_INT32("speed", VirtIONet,
>> > > > > > > > net_conf.speed, SPEED_UNKNOWN),
>> > > > > > > > DEFINE_PROP_STRING("duplex", VirtIONet,
>> > > > > > > > net_conf.duplex_str),
>> > > > > > > > + DEFINE_PROP_BIT64("standby", VirtIONet,
>> > > > > > > > host_features, VIRTIO_NET_F_STANDBY,
>> > > > > > > > + false),
>> > > > > > > > DEFINE_PROP_END_OF_LIST(),
>> > > > > > > > };
>> > > > > > > > diff --git
>> > > > > > > > a/include/standard-headers/linux/virtio_net.h
>> > > > > > > > b/include/standard-headers/linux/virtio_net.h
>> > > > > > > > index e9f255ea3f..01ec09684c 100644
>> > > > > > > > --- a/include/standard-headers/linux/virtio_net.h
>> > > > > > > > +++ b/include/standard-headers/linux/virtio_net.h
>> > > > > > > > @@ -57,6 +57,9 @@
>> > > > > > > > * Steering */
>> > > > > > > > #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
>> > > > > > > > +#define VIRTIO_NET_F_STANDBY 62 /* Act
>> > > > > > > > as standby for another device
>> > > > > > > > + * with the same MAC.
>> > > > > > > > + */
>> > > > > > > > #define VIRTIO_NET_F_SPEED_DUPLEX 63 /*
>> > > > > > > > Device set linkspeed and duplex */
>> > > > > > > > #ifndef VIRTIO_NET_NO_LEGACY
>> > >
>> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v2 3/5] iommu/virtio: Add probe request
From: Michael S. Tsirkin @ 2018-06-22 0:55 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: mark.rutland, devicetree, jayachandran.nair, lorenzo.pieralisi,
tnowicki, kvm, virtio-dev, joro, will.deacon, virtualization,
marc.zyngier, iommu, robh+dt, eric.auger, robin.murphy, kvmarm
In-Reply-To: <20180621190655.56391-4-jean-philippe.brucker@arm.com>
On Thu, Jun 21, 2018 at 08:06:53PM +0100, Jean-Philippe Brucker wrote:
> When the device offers the probe feature, send a probe request for each
> device managed by the IOMMU. Extract RESV_MEM information. When we
> encounter a MSI doorbell region, set it up as a IOMMU_RESV_MSI region.
> This will tell other subsystems that there is no need to map the MSI
> doorbell in the virtio-iommu, because MSIs bypass it.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> ---
> drivers/iommu/virtio-iommu.c | 149 ++++++++++++++++++++++++++++--
> include/uapi/linux/virtio_iommu.h | 37 ++++++++
> 2 files changed, 180 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index ea0242d8624b..29ce9f4398ef 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -48,6 +48,7 @@ struct viommu_dev {
> struct iommu_domain_geometry geometry;
> u64 pgsize_bitmap;
> u8 domain_bits;
> + u32 probe_size;
> };
>
> struct viommu_mapping {
> @@ -69,8 +70,10 @@ struct viommu_domain {
> };
>
> struct viommu_endpoint {
> + struct device *dev;
> struct viommu_dev *viommu;
> struct viommu_domain *vdomain;
> + struct list_head resv_regions;
> };
>
> struct viommu_request {
> @@ -121,6 +124,9 @@ static off_t viommu_get_req_offset(struct viommu_dev *viommu,
> {
> size_t tail_size = sizeof(struct virtio_iommu_req_tail);
>
> + if (req->type == VIRTIO_IOMMU_T_PROBE)
> + return len - viommu->probe_size - tail_size;
> +
> return len - tail_size;
> }
>
> @@ -404,6 +410,103 @@ static int viommu_replay_mappings(struct viommu_domain *vdomain)
> return ret;
> }
>
> +static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
> + struct virtio_iommu_probe_resv_mem *mem,
> + size_t len)
> +{
> + struct iommu_resv_region *region = NULL;
> + unsigned long prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> +
> + u64 start = le64_to_cpu(mem->start);
> + u64 end = le64_to_cpu(mem->end);
> + size_t size = end - start + 1;
> +
> + if (len < sizeof(*mem))
> + return -EINVAL;
> +
> + switch (mem->subtype) {
> + default:
> + if (mem->subtype != VIRTIO_IOMMU_RESV_MEM_T_RESERVED &&
> + mem->subtype != VIRTIO_IOMMU_RESV_MEM_T_MSI)
> + dev_warn(vdev->dev, "unknown resv mem subtype 0x%x\n",
> + mem->subtype);
> + /* Fall-through */
> + case VIRTIO_IOMMU_RESV_MEM_T_RESERVED:
> + region = iommu_alloc_resv_region(start, size, 0,
> + IOMMU_RESV_RESERVED);
> + break;
> + case VIRTIO_IOMMU_RESV_MEM_T_MSI:
> + region = iommu_alloc_resv_region(start, size, prot,
> + IOMMU_RESV_MSI);
> + break;
> + }
> +
> + list_add(&vdev->resv_regions, ®ion->list);
> + return 0;
> +}
> +
> +static int viommu_probe_endpoint(struct viommu_dev *viommu, struct device *dev)
> +{
> + int ret;
> + u16 type, len;
> + size_t cur = 0;
> + size_t probe_len;
> + struct virtio_iommu_req_probe *probe;
> + struct virtio_iommu_probe_property *prop;
> + struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> + struct viommu_endpoint *vdev = fwspec->iommu_priv;
> +
> + if (!fwspec->num_ids)
> + return -EINVAL;
> +
> + probe_len = sizeof(*probe) + viommu->probe_size +
> + sizeof(struct virtio_iommu_req_tail);
> + probe = kzalloc(probe_len, GFP_KERNEL);
> + if (!probe)
> + return -ENOMEM;
> +
> + probe->head.type = VIRTIO_IOMMU_T_PROBE;
> + /*
> + * For now, assume that properties of an endpoint that outputs multiple
> + * IDs are consistent. Only probe the first one.
> + */
> + probe->endpoint = cpu_to_le32(fwspec->ids[0]);
> +
> + ret = viommu_send_req_sync(viommu, probe, probe_len);
> + if (ret)
> + goto out_free;
> +
> + prop = (void *)probe->properties;
> + type = le16_to_cpu(prop->type) & VIRTIO_IOMMU_PROBE_T_MASK;
> +
> + while (type != VIRTIO_IOMMU_PROBE_T_NONE &&
> + cur < viommu->probe_size) {
> + len = le16_to_cpu(prop->length);
> +
> + switch (type) {
> + case VIRTIO_IOMMU_PROBE_T_RESV_MEM:
> + ret = viommu_add_resv_mem(vdev, (void *)prop->value, len);
> + break;
> + default:
> + dev_dbg(dev, "unknown viommu prop 0x%x\n", type);
> + }
> +
> + if (ret)
> + dev_err(dev, "failed to parse viommu prop 0x%x\n", type);
> +
> + cur += sizeof(*prop) + len;
> + if (cur >= viommu->probe_size)
> + break;
> +
> + prop = (void *)probe->properties + cur;
> + type = le16_to_cpu(prop->type) & VIRTIO_IOMMU_PROBE_T_MASK;
> + }
> +
> +out_free:
> + kfree(probe);
> + return ret;
> +}
> +
> /* IOMMU API */
>
> static struct iommu_domain *viommu_domain_alloc(unsigned type)
> @@ -627,15 +730,33 @@ static void viommu_iotlb_sync(struct iommu_domain *domain)
>
> static void viommu_get_resv_regions(struct device *dev, struct list_head *head)
> {
> - struct iommu_resv_region *region;
> + struct iommu_resv_region *entry, *new_entry, *msi = NULL;
> + struct viommu_endpoint *vdev = dev->iommu_fwspec->iommu_priv;
> int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>
> - region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH, prot,
> - IOMMU_RESV_SW_MSI);
> - if (!region)
> - return;
> + list_for_each_entry(entry, &vdev->resv_regions, list) {
> + /*
> + * If the device registered a bypass MSI windows, use it.
> + * Otherwise add a software-mapped region
> + */
> + if (entry->type == IOMMU_RESV_MSI)
> + msi = entry;
> +
> + new_entry = kmemdup(entry, sizeof(*entry), GFP_KERNEL);
> + if (!new_entry)
> + return;
> + list_add_tail(&new_entry->list, head);
> + }
> +
> + if (!msi) {
> + msi = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
> + prot, IOMMU_RESV_SW_MSI);
> + if (!msi)
> + return;
> +
> + list_add_tail(&msi->list, head);
> + }
>
> - list_add_tail(®ion->list, head);
> iommu_dma_get_resv_regions(dev, head);
> }
>
> @@ -683,9 +804,18 @@ static int viommu_add_device(struct device *dev)
> if (!vdev)
> return -ENOMEM;
>
> + vdev->dev = dev;
> vdev->viommu = viommu;
> + INIT_LIST_HEAD(&vdev->resv_regions);
> fwspec->iommu_priv = vdev;
>
> + if (viommu->probe_size) {
> + /* Get additional information for this endpoint */
> + ret = viommu_probe_endpoint(viommu, dev);
> + if (ret)
> + return ret;
> + }
> +
> ret = iommu_device_link(&viommu->iommu, dev);
> if (ret)
> goto err_free_dev;
> @@ -708,6 +838,7 @@ static int viommu_add_device(struct device *dev)
> iommu_device_unlink(&viommu->iommu, dev);
>
> err_free_dev:
> + viommu_put_resv_regions(dev, &vdev->resv_regions);
> kfree(vdev);
>
> return ret;
> @@ -725,6 +856,7 @@ static void viommu_remove_device(struct device *dev)
>
> iommu_group_remove_device(dev);
> iommu_device_unlink(&vdev->viommu->iommu, dev);
> + viommu_put_resv_regions(dev, &vdev->resv_regions);
> kfree(vdev);
> }
>
> @@ -821,6 +953,10 @@ static int viommu_probe(struct virtio_device *vdev)
> struct virtio_iommu_config, domain_bits,
> &viommu->domain_bits);
>
> + virtio_cread_feature(vdev, VIRTIO_IOMMU_F_PROBE,
> + struct virtio_iommu_config, probe_size,
> + &viommu->probe_size);
> +
> viommu->geometry = (struct iommu_domain_geometry) {
> .aperture_start = input_start,
> .aperture_end = input_end,
> @@ -902,6 +1038,7 @@ static unsigned int features[] = {
> VIRTIO_IOMMU_F_MAP_UNMAP,
> VIRTIO_IOMMU_F_DOMAIN_BITS,
> VIRTIO_IOMMU_F_INPUT_RANGE,
> + VIRTIO_IOMMU_F_PROBE,
> };
>
> static struct virtio_device_id id_table[] = {
> diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
> index f4fafa0b41a7..2fd802a860cd 100644
> --- a/include/uapi/linux/virtio_iommu.h
> +++ b/include/uapi/linux/virtio_iommu.h
> @@ -14,6 +14,7 @@
> #define VIRTIO_IOMMU_F_DOMAIN_BITS 1
> #define VIRTIO_IOMMU_F_MAP_UNMAP 2
> #define VIRTIO_IOMMU_F_BYPASS 3
> +#define VIRTIO_IOMMU_F_PROBE 4
>
> struct virtio_iommu_config {
> /* Supported page sizes */
> @@ -25,6 +26,9 @@ struct virtio_iommu_config {
> } input_range;
> /* Max domain ID size */
> __u8 domain_bits;
> + __u8 padding[3];
> + /* Probe buffer size */
> + __u32 probe_size;
> } __packed;
>
> /* Request types */
> @@ -32,6 +36,7 @@ struct virtio_iommu_config {
> #define VIRTIO_IOMMU_T_DETACH 0x02
> #define VIRTIO_IOMMU_T_MAP 0x03
> #define VIRTIO_IOMMU_T_UNMAP 0x04
> +#define VIRTIO_IOMMU_T_PROBE 0x05
>
> /* Status types */
> #define VIRTIO_IOMMU_S_OK 0x00
> @@ -105,6 +110,37 @@ struct virtio_iommu_req_unmap {
> struct virtio_iommu_req_tail tail;
> } __packed;
>
> +#define VIRTIO_IOMMU_RESV_MEM_T_RESERVED 0
> +#define VIRTIO_IOMMU_RESV_MEM_T_MSI 1
> +
> +struct virtio_iommu_probe_resv_mem {
> + __u8 subtype;
> + __u8 reserved[3];
> + __le64 start;
> + __le64 end;
> +} __packed;
start/end are not aligned you need to pad more.
> +
> +#define VIRTIO_IOMMU_PROBE_T_NONE 0
> +#define VIRTIO_IOMMU_PROBE_T_RESV_MEM 1
> +
> +#define VIRTIO_IOMMU_PROBE_T_MASK 0xfff
> +
> +struct virtio_iommu_probe_property {
> + __le16 type;
> + __le16 length;
> + __u8 value[];
> +} __packed;
> +
> +struct virtio_iommu_req_probe {
> + struct virtio_iommu_req_head head;
> + __le32 endpoint;
> + __u8 reserved[64];
> +
> + __u8 properties[];
> +
> + /* Tail follows the variable-length properties array (no padding) */
> +} __packed;
> +
> union virtio_iommu_req {
> struct virtio_iommu_req_head head;
>
> @@ -112,6 +148,7 @@ union virtio_iommu_req {
> struct virtio_iommu_req_detach detach;
> struct virtio_iommu_req_map map;
> struct virtio_iommu_req_unmap unmap;
> + struct virtio_iommu_req_probe probe;
> };
>
> #endif
> --
> 2.17.0
^ permalink raw reply
* Re: [PATCH v2 2/5] iommu: Add virtio-iommu driver
From: Michael S. Tsirkin @ 2018-06-22 0:51 UTC (permalink / raw)
To: Jean-Philippe Brucker
Cc: mark.rutland, devicetree, jayachandran.nair, lorenzo.pieralisi,
tnowicki, kvm, virtio-dev, joro, will.deacon, virtualization,
marc.zyngier, iommu, robh+dt, eric.auger, robin.murphy, kvmarm
In-Reply-To: <20180621190655.56391-3-jean-philippe.brucker@arm.com>
On Thu, Jun 21, 2018 at 08:06:52PM +0100, Jean-Philippe Brucker wrote:
> The virtio IOMMU is a para-virtualized device, allowing to send IOMMU
> requests such as map/unmap over virtio-mmio transport without emulating
> page tables. This implementation handles ATTACH, DETACH, MAP and UNMAP
> requests.
>
> The bulk of the code transforms calls coming from the IOMMU API into
> corresponding virtio requests. Mappings are kept in an interval tree
> instead of page tables.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> ---
> MAINTAINERS | 6 +
> drivers/iommu/Kconfig | 11 +
> drivers/iommu/Makefile | 1 +
> drivers/iommu/virtio-iommu.c | 929 ++++++++++++++++++++++++++++++
> include/uapi/linux/virtio_ids.h | 1 +
> include/uapi/linux/virtio_iommu.h | 117 ++++
> 6 files changed, 1065 insertions(+)
> create mode 100644 drivers/iommu/virtio-iommu.c
> create mode 100644 include/uapi/linux/virtio_iommu.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 55c08968aaab..bfb09dfa8e02 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15248,6 +15248,12 @@ S: Maintained
> F: drivers/virtio/virtio_input.c
> F: include/uapi/linux/virtio_input.h
>
> +VIRTIO IOMMU DRIVER
> +M: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> +S: Maintained
> +F: drivers/iommu/virtio-iommu.c
> +F: include/uapi/linux/virtio_iommu.h
> +
> VIRTUAL BOX GUEST DEVICE DRIVER
> M: Hans de Goede <hdegoede@redhat.com>
> M: Arnd Bergmann <arnd@arndb.de>
Please add the virtualization mailing list.
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 958417f22020..820709383899 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -412,4 +412,15 @@ config QCOM_IOMMU
> help
> Support for IOMMU on certain Qualcomm SoCs.
>
> +config VIRTIO_IOMMU
> + bool "Virtio IOMMU driver"
> + depends on VIRTIO_MMIO=y
> + select IOMMU_API
> + select INTERVAL_TREE
> + select ARM_DMA_USE_IOMMU if ARM
> + help
> + Para-virtualised IOMMU driver with virtio.
> +
> + Say Y here if you intend to run this kernel as a guest.
> +
> endif # IOMMU_SUPPORT
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 244ad7913a81..b559c6ae81ea 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -33,3 +33,4 @@ obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o
> obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o
> obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
> obj-$(CONFIG_QCOM_IOMMU) += qcom_iommu.o
> +obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> new file mode 100644
> index 000000000000..ea0242d8624b
> --- /dev/null
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -0,0 +1,929 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Virtio driver for the paravirtualized IOMMU
> + *
> + * Copyright (C) 2018 Arm Limited
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/amba/bus.h>
> +#include <linux/delay.h>
> +#include <linux/dma-iommu.h>
> +#include <linux/freezer.h>
> +#include <linux/interval_tree.h>
> +#include <linux/iommu.h>
> +#include <linux/module.h>
> +#include <linux/of_iommu.h>
> +#include <linux/of_platform.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_config.h>
> +#include <linux/virtio_ids.h>
> +#include <linux/wait.h>
> +
> +#include <uapi/linux/virtio_iommu.h>
> +
> +#define MSI_IOVA_BASE 0x8000000
> +#define MSI_IOVA_LENGTH 0x100000
> +
> +#define VIOMMU_REQUEST_VQ 0
> +#define VIOMMU_NR_VQS 1
> +
> +#define VIOMMU_REQUEST_TIMEOUT 10000 /* 10s */
Where does this come from? Do you absolutely have to have
an arbitrary timeout?
> +
> +struct viommu_dev {
> + struct iommu_device iommu;
> + struct device *dev;
> + struct virtio_device *vdev;
> +
> + struct ida domain_ids;
> +
> + struct virtqueue *vqs[VIOMMU_NR_VQS];
> + spinlock_t request_lock;
> + struct list_head requests;
> +
> + /* Device configuration */
> + struct iommu_domain_geometry geometry;
> + u64 pgsize_bitmap;
> + u8 domain_bits;
> +};
> +
> +struct viommu_mapping {
> + phys_addr_t paddr;
> + struct interval_tree_node iova;
> + u32 flags;
> +};
> +
> +struct viommu_domain {
> + struct iommu_domain domain;
> + struct viommu_dev *viommu;
> + struct mutex mutex;
> + unsigned int id;
> +
> + spinlock_t mappings_lock;
> + struct rb_root_cached mappings;
> +
> + unsigned long nr_endpoints;
> +};
> +
> +struct viommu_endpoint {
> + struct viommu_dev *viommu;
> + struct viommu_domain *vdomain;
> +};
> +
> +struct viommu_request {
> + struct list_head list;
> + void *writeback;
> + unsigned int write_offset;
> + unsigned int len;
> + char buf[];
> +};
> +
> +#define to_viommu_domain(domain) \
> + container_of(domain, struct viommu_domain, domain)
> +
> +static int viommu_get_req_errno(void *buf, size_t len)
> +{
> + struct virtio_iommu_req_tail *tail = buf + len - sizeof(*tail);
> +
> + switch (tail->status) {
> + case VIRTIO_IOMMU_S_OK:
> + return 0;
> + case VIRTIO_IOMMU_S_UNSUPP:
> + return -ENOSYS;
> + case VIRTIO_IOMMU_S_INVAL:
> + return -EINVAL;
> + case VIRTIO_IOMMU_S_RANGE:
> + return -ERANGE;
> + case VIRTIO_IOMMU_S_NOENT:
> + return -ENOENT;
> + case VIRTIO_IOMMU_S_FAULT:
> + return -EFAULT;
> + case VIRTIO_IOMMU_S_IOERR:
> + case VIRTIO_IOMMU_S_DEVERR:
> + default:
> + return -EIO;
> + }
> +}
> +
> +static void viommu_set_req_status(void *buf, size_t len, int status)
> +{
> + struct virtio_iommu_req_tail *tail = buf + len - sizeof(*tail);
> +
> + tail->status = status;
> +}
> +
> +static off_t viommu_get_req_offset(struct viommu_dev *viommu,
> + struct virtio_iommu_req_head *req,
> + size_t len)
> +{
> + size_t tail_size = sizeof(struct virtio_iommu_req_tail);
> +
> + return len - tail_size;
> +}
> +
> +/*
> + * __viommu_sync_req - Complete all in-flight requests
> + *
> + * Wait for all added requests to complete. When this function returns, all
> + * requests that were in-flight at the time of the call have completed.
> + */
> +static int __viommu_sync_req(struct viommu_dev *viommu)
> +{
> + int ret = 0;
> + unsigned int len;
> + size_t write_len;
> + struct viommu_request *req;
> + struct virtqueue *vq = viommu->vqs[VIOMMU_REQUEST_VQ];
> + ktime_t timeout = ktime_add_ms(ktime_get(), VIOMMU_REQUEST_TIMEOUT);
> +
> + assert_spin_locked(&viommu->request_lock);
> +
> + virtqueue_kick(vq);
> +
> + while (!list_empty(&viommu->requests)) {
> + len = 0;
> + req = virtqueue_get_buf(vq, &len);
> + if (req == NULL) {
> + if (ktime_before(ktime_get(), timeout))
> + continue;
> +
> + /* After timeout, remove all requests */
> + req = list_first_entry(&viommu->requests,
> + struct viommu_request, list);
> + ret = -ETIMEDOUT;
> + }
> +
> + if (!len)
> + viommu_set_req_status(req->buf, req->len,
> + VIRTIO_IOMMU_S_IOERR);
> +
> + write_len = req->len - req->write_offset;
> + if (req->writeback && len >= write_len)
> + memcpy(req->writeback, req->buf + req->write_offset,
> + write_len);
> +
> + list_del(&req->list);
> + kfree(req);
> + }
> +
> + return ret;
> +}
> +
> +static int viommu_sync_req(struct viommu_dev *viommu)
> +{
> + int ret;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&viommu->request_lock, flags);
> + ret = __viommu_sync_req(viommu);
> + if (ret)
> + dev_dbg(viommu->dev, "could not sync requests (%d)\n", ret);
> + spin_unlock_irqrestore(&viommu->request_lock, flags);
> +
> + return ret;
> +}
> +
> +/*
> + * __viommu_add_request - Add one request to the queue
> + * @buf: pointer to the request buffer
> + * @len: length of the request buffer
> + * @writeback: copy data back to the buffer when the request completes.
> + *
> + * Add a request to the queue. Only synchronize the queue if it's already full.
> + * Otherwise don't kick the queue nor wait for requests to complete.
> + *
> + * When @writeback is true, data written by the device, including the request
> + * status, is copied into @buf after the request completes. This is unsafe if
> + * the caller allocates @buf on stack and drops the lock between add_req() and
> + * sync_req().
> + *
> + * Return 0 if the request was successfully added to the queue.
> + */
> +static int __viommu_add_req(struct viommu_dev *viommu, void *buf, size_t len,
> + bool writeback)
> +{
> + int ret;
> + off_t write_offset;
> + struct viommu_request *req;
> + struct scatterlist top_sg, bottom_sg;
> + struct scatterlist *sg[2] = { &top_sg, &bottom_sg };
> + struct virtqueue *vq = viommu->vqs[VIOMMU_REQUEST_VQ];
> +
> + assert_spin_locked(&viommu->request_lock);
> +
> + write_offset = viommu_get_req_offset(viommu, buf, len);
> + if (!write_offset)
> + return -EINVAL;
> +
> + req = kzalloc(sizeof(*req) + len, GFP_ATOMIC);
> + if (!req)
> + return -ENOMEM;
> +
> + req->len = len;
> + if (writeback) {
> + req->writeback = buf + write_offset;
> + req->write_offset = write_offset;
> + }
> + memcpy(&req->buf, buf, write_offset);
> +
> + sg_init_one(&top_sg, req->buf, write_offset);
> + sg_init_one(&bottom_sg, req->buf + write_offset, len - write_offset);
> +
> + ret = virtqueue_add_sgs(vq, sg, 1, 1, req, GFP_ATOMIC);
> + if (ret == -ENOSPC) {
> + /* If the queue is full, sync and retry */
> + if (!__viommu_sync_req(viommu))
> + ret = virtqueue_add_sgs(vq, sg, 1, 1, req, GFP_ATOMIC);
> + }
> + if (ret)
> + goto err_free;
> +
> + list_add_tail(&req->list, &viommu->requests);
> + return 0;
> +
> +err_free:
> + kfree(req);
> + return ret;
> +}
> +
> +static int viommu_add_req(struct viommu_dev *viommu, void *buf, size_t len)
> +{
> + int ret;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&viommu->request_lock, flags);
> + ret = __viommu_add_req(viommu, buf, len, false);
> + if (ret)
> + dev_dbg(viommu->dev, "could not add request: %d\n", ret);
> + spin_unlock_irqrestore(&viommu->request_lock, flags);
> +
> + return ret;
> +}
> +
> +/*
> + * Send a request and wait for it to complete. Return the request status (as an
> + * errno)
> + */
> +static int viommu_send_req_sync(struct viommu_dev *viommu, void *buf,
> + size_t len)
> +{
> + int ret;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&viommu->request_lock, flags);
> +
> + ret = __viommu_add_req(viommu, buf, len, true);
> + if (ret) {
> + dev_dbg(viommu->dev, "could not add request (%d)\n", ret);
> + goto out_unlock;
> + }
> +
> + ret = __viommu_sync_req(viommu);
> + if (ret) {
> + dev_dbg(viommu->dev, "could not sync requests (%d)\n", ret);
> + /* Fall-through (get the actual request status) */
> + }
> +
> + ret = viommu_get_req_errno(buf, len);
> +out_unlock:
> + spin_unlock_irqrestore(&viommu->request_lock, flags);
> + return ret;
> +}
> +
> +/*
> + * viommu_add_mapping - add a mapping to the internal tree
> + *
> + * On success, return the new mapping. Otherwise return NULL.
> + */
> +static struct viommu_mapping *
> +viommu_add_mapping(struct viommu_domain *vdomain, unsigned long iova,
> + phys_addr_t paddr, size_t size, u32 flags)
> +{
> + unsigned long irqflags;
> + struct viommu_mapping *mapping;
> +
> + mapping = kzalloc(sizeof(*mapping), GFP_ATOMIC);
> + if (!mapping)
> + return NULL;
> +
> + mapping->paddr = paddr;
> + mapping->iova.start = iova;
> + mapping->iova.last = iova + size - 1;
> + mapping->flags = flags;
> +
> + spin_lock_irqsave(&vdomain->mappings_lock, irqflags);
> + interval_tree_insert(&mapping->iova, &vdomain->mappings);
> + spin_unlock_irqrestore(&vdomain->mappings_lock, irqflags);
> +
> + return mapping;
> +}
> +
> +/*
> + * viommu_del_mappings - remove mappings from the internal tree
> + *
> + * @vdomain: the domain
> + * @iova: start of the range
> + * @size: size of the range. A size of 0 corresponds to the entire address
> + * space.
> + *
> + * On success, returns the number of unmapped bytes (>= size)
> + */
> +static size_t viommu_del_mappings(struct viommu_domain *vdomain,
> + unsigned long iova, size_t size)
> +{
> + size_t unmapped = 0;
> + unsigned long flags;
> + unsigned long last = iova + size - 1;
> + struct viommu_mapping *mapping = NULL;
> + struct interval_tree_node *node, *next;
> +
> + spin_lock_irqsave(&vdomain->mappings_lock, flags);
> + next = interval_tree_iter_first(&vdomain->mappings, iova, last);
> + while (next) {
> + node = next;
> + mapping = container_of(node, struct viommu_mapping, iova);
> + next = interval_tree_iter_next(node, iova, last);
> +
> + /* Trying to split a mapping? */
> + if (mapping->iova.start < iova)
> + break;
> +
> + /*
> + * Note that for a partial range, this will return the full
> + * mapping so we avoid sending split requests to the device.
> + */
> + unmapped += mapping->iova.last - mapping->iova.start + 1;
> +
> + interval_tree_remove(node, &vdomain->mappings);
> + kfree(mapping);
> + }
> + spin_unlock_irqrestore(&vdomain->mappings_lock, flags);
> +
> + return unmapped;
> +}
> +
> +/*
> + * viommu_replay_mappings - re-send MAP requests
> + *
> + * When reattaching a domain that was previously detached from all endpoints,
> + * mappings were deleted from the device. Re-create the mappings available in
> + * the internal tree.
> + */
> +static int viommu_replay_mappings(struct viommu_domain *vdomain)
> +{
> + int ret;
> + unsigned long flags;
> + struct viommu_mapping *mapping;
> + struct interval_tree_node *node;
> + struct virtio_iommu_req_map map;
> +
> + spin_lock_irqsave(&vdomain->mappings_lock, flags);
> + node = interval_tree_iter_first(&vdomain->mappings, 0, -1UL);
> + while (node) {
> + mapping = container_of(node, struct viommu_mapping, iova);
> + map = (struct virtio_iommu_req_map) {
> + .head.type = VIRTIO_IOMMU_T_MAP,
> + .domain = cpu_to_le32(vdomain->id),
> + .virt_start = cpu_to_le64(mapping->iova.start),
> + .virt_end = cpu_to_le64(mapping->iova.last),
> + .phys_start = cpu_to_le64(mapping->paddr),
> + .flags = cpu_to_le32(mapping->flags),
> + };
> +
> + ret = viommu_send_req_sync(vdomain->viommu, &map, sizeof(map));
> + if (ret)
> + break;
> +
> + node = interval_tree_iter_next(node, 0, -1UL);
> + }
> + spin_unlock_irqrestore(&vdomain->mappings_lock, flags);
> +
> + return ret;
> +}
> +
> +/* IOMMU API */
> +
> +static struct iommu_domain *viommu_domain_alloc(unsigned type)
> +{
> + struct viommu_domain *vdomain;
> +
> + if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
> + return NULL;
> +
> + vdomain = kzalloc(sizeof(*vdomain), GFP_KERNEL);
> + if (!vdomain)
> + return NULL;
> +
> + mutex_init(&vdomain->mutex);
> + spin_lock_init(&vdomain->mappings_lock);
> + vdomain->mappings = RB_ROOT_CACHED;
> +
> + if (type == IOMMU_DOMAIN_DMA &&
> + iommu_get_dma_cookie(&vdomain->domain)) {
> + kfree(vdomain);
> + return NULL;
> + }
> +
> + return &vdomain->domain;
> +}
> +
> +static int viommu_domain_finalise(struct viommu_dev *viommu,
> + struct iommu_domain *domain)
> +{
> + int ret;
> + struct viommu_domain *vdomain = to_viommu_domain(domain);
> + /* ida limits size to 31 bits. A value of 0 means "max" */
> + unsigned int max_domain = viommu->domain_bits >= 31 ? 0 :
> + 1U << viommu->domain_bits;
> +
> + vdomain->viommu = viommu;
> +
> + domain->pgsize_bitmap = viommu->pgsize_bitmap;
> + domain->geometry = viommu->geometry;
> +
> + ret = ida_simple_get(&viommu->domain_ids, 0, max_domain, GFP_KERNEL);
> + if (ret >= 0)
> + vdomain->id = (unsigned int)ret;
> +
> + return ret > 0 ? 0 : ret;
> +}
> +
> +static void viommu_domain_free(struct iommu_domain *domain)
> +{
> + struct viommu_domain *vdomain = to_viommu_domain(domain);
> +
> + iommu_put_dma_cookie(domain);
> +
> + /* Free all remaining mappings (size 2^64) */
> + viommu_del_mappings(vdomain, 0, 0);
> +
> + if (vdomain->viommu)
> + ida_simple_remove(&vdomain->viommu->domain_ids, vdomain->id);
> +
> + kfree(vdomain);
> +}
> +
> +static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev)
> +{
> + int i;
> + int ret = 0;
> + struct virtio_iommu_req_attach req;
> + struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> + struct viommu_endpoint *vdev = fwspec->iommu_priv;
> + struct viommu_domain *vdomain = to_viommu_domain(domain);
> +
> + mutex_lock(&vdomain->mutex);
> + if (!vdomain->viommu) {
> + /*
> + * Initialize the domain proper now that we know which viommu
> + * owns it.
> + */
> + ret = viommu_domain_finalise(vdev->viommu, domain);
> + } else if (vdomain->viommu != vdev->viommu) {
> + dev_err(dev, "cannot attach to foreign vIOMMU\n");
> + ret = -EXDEV;
> + }
> + mutex_unlock(&vdomain->mutex);
> +
> + if (ret)
> + return ret;
> +
> + /*
> + * In the virtio-iommu device, when attaching the endpoint to a new
> + * domain, it is detached from the old one and, if as as a result the
> + * old domain isn't attached to any endpoint, all mappings are removed
> + * from the old domain and it is freed.
> + *
> + * In the driver the old domain still exists, and its mappings will be
> + * recreated if it gets reattached to an endpoint. Otherwise it will be
> + * freed explicitly.
> + *
> + * vdev->vdomain is protected by group->mutex
> + */
> + if (vdev->vdomain)
> + vdev->vdomain->nr_endpoints--;
> +
> + req = (struct virtio_iommu_req_attach) {
> + .head.type = VIRTIO_IOMMU_T_ATTACH,
> + .domain = cpu_to_le32(vdomain->id),
> + };
> +
> + for (i = 0; i < fwspec->num_ids; i++) {
> + req.endpoint = cpu_to_le32(fwspec->ids[i]);
> +
> + ret = viommu_send_req_sync(vdomain->viommu, &req, sizeof(req));
> + if (ret)
> + return ret;
> + }
> +
> + if (!vdomain->nr_endpoints) {
> + /*
> + * This endpoint is the first to be attached to the domain.
> + * Replay existing mappings (e.g. SW MSI).
> + */
> + ret = viommu_replay_mappings(vdomain);
> + if (ret)
> + return ret;
> + }
> +
> + vdomain->nr_endpoints++;
> + vdev->vdomain = vdomain;
> +
> + return 0;
> +}
> +
> +static int viommu_map(struct iommu_domain *domain, unsigned long iova,
> + phys_addr_t paddr, size_t size, int prot)
> +{
> + int ret;
> + int flags;
> + struct viommu_mapping *mapping;
> + struct virtio_iommu_req_map map;
> + struct viommu_domain *vdomain = to_viommu_domain(domain);
> +
> + flags = (prot & IOMMU_READ ? VIRTIO_IOMMU_MAP_F_READ : 0) |
> + (prot & IOMMU_WRITE ? VIRTIO_IOMMU_MAP_F_WRITE : 0) |
> + (prot & IOMMU_MMIO ? VIRTIO_IOMMU_MAP_F_MMIO : 0);
> +
> + mapping = viommu_add_mapping(vdomain, iova, paddr, size, flags);
> + if (!mapping)
> + return -ENOMEM;
> +
> + map = (struct virtio_iommu_req_map) {
> + .head.type = VIRTIO_IOMMU_T_MAP,
> + .domain = cpu_to_le32(vdomain->id),
> + .virt_start = cpu_to_le64(iova),
> + .phys_start = cpu_to_le64(paddr),
> + .virt_end = cpu_to_le64(iova + size - 1),
> + .flags = cpu_to_le32(flags),
> + };
> +
> + if (!vdomain->nr_endpoints)
> + return 0;
> +
> + ret = viommu_send_req_sync(vdomain->viommu, &map, sizeof(map));
> + if (ret)
> + viommu_del_mappings(vdomain, iova, size);
> +
> + return ret;
> +}
> +
> +static size_t viommu_unmap(struct iommu_domain *domain, unsigned long iova,
> + size_t size)
> +{
> + int ret = 0;
> + size_t unmapped;
> + struct virtio_iommu_req_unmap unmap;
> + struct viommu_domain *vdomain = to_viommu_domain(domain);
> +
> + unmapped = viommu_del_mappings(vdomain, iova, size);
> + if (unmapped < size)
> + return 0;
> +
> + /* Device already removed all mappings after detach. */
> + if (!vdomain->nr_endpoints)
> + return unmapped;
> +
> + unmap = (struct virtio_iommu_req_unmap) {
> + .head.type = VIRTIO_IOMMU_T_UNMAP,
> + .domain = cpu_to_le32(vdomain->id),
> + .virt_start = cpu_to_le64(iova),
> + .virt_end = cpu_to_le64(iova + unmapped - 1),
> + };
> +
> + ret = viommu_add_req(vdomain->viommu, &unmap, sizeof(unmap));
> + return ret ? 0 : unmapped;
> +}
> +
> +static phys_addr_t viommu_iova_to_phys(struct iommu_domain *domain,
> + dma_addr_t iova)
> +{
> + u64 paddr = 0;
> + unsigned long flags;
> + struct viommu_mapping *mapping;
> + struct interval_tree_node *node;
> + struct viommu_domain *vdomain = to_viommu_domain(domain);
> +
> + spin_lock_irqsave(&vdomain->mappings_lock, flags);
> + node = interval_tree_iter_first(&vdomain->mappings, iova, iova);
> + if (node) {
> + mapping = container_of(node, struct viommu_mapping, iova);
> + paddr = mapping->paddr + (iova - mapping->iova.start);
> + }
> + spin_unlock_irqrestore(&vdomain->mappings_lock, flags);
> +
> + return paddr;
> +}
> +
> +static void viommu_iotlb_sync(struct iommu_domain *domain)
> +{
> + struct viommu_domain *vdomain = to_viommu_domain(domain);
> +
> + viommu_sync_req(vdomain->viommu);
> +}
> +
> +static void viommu_get_resv_regions(struct device *dev, struct list_head *head)
> +{
> + struct iommu_resv_region *region;
> + int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> +
> + region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH, prot,
> + IOMMU_RESV_SW_MSI);
> + if (!region)
> + return;
> +
> + list_add_tail(®ion->list, head);
> + iommu_dma_get_resv_regions(dev, head);
> +}
> +
> +static void viommu_put_resv_regions(struct device *dev, struct list_head *head)
> +{
> + struct iommu_resv_region *entry, *next;
> +
> + list_for_each_entry_safe(entry, next, head, list)
> + kfree(entry);
> +}
> +
> +static struct iommu_ops viommu_ops;
> +static struct virtio_driver virtio_iommu_drv;
> +
> +static int viommu_match_node(struct device *dev, void *data)
> +{
> + return dev->parent->fwnode == data;
> +}
> +
> +static struct viommu_dev *viommu_get_by_fwnode(struct fwnode_handle *fwnode)
> +{
> + struct device *dev = driver_find_device(&virtio_iommu_drv.driver, NULL,
> + fwnode, viommu_match_node);
> + put_device(dev);
> +
> + return dev ? dev_to_virtio(dev)->priv : NULL;
> +}
> +
> +static int viommu_add_device(struct device *dev)
> +{
> + int ret;
> + struct iommu_group *group;
> + struct viommu_endpoint *vdev;
> + struct viommu_dev *viommu = NULL;
> + struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +
> + if (!fwspec || fwspec->ops != &viommu_ops)
> + return -ENODEV;
> +
> + viommu = viommu_get_by_fwnode(fwspec->iommu_fwnode);
> + if (!viommu)
> + return -ENODEV;
> +
> + vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
> + if (!vdev)
> + return -ENOMEM;
> +
> + vdev->viommu = viommu;
> + fwspec->iommu_priv = vdev;
> +
> + ret = iommu_device_link(&viommu->iommu, dev);
> + if (ret)
> + goto err_free_dev;
> +
> + /*
> + * Last step creates a default domain and attaches to it. Everything
> + * must be ready.
> + */
> + group = iommu_group_get_for_dev(dev);
> + if (IS_ERR(group)) {
> + ret = PTR_ERR(group);
> + goto err_unlink_dev;
> + }
> +
> + iommu_group_put(group);
> +
> + return PTR_ERR_OR_ZERO(group);
> +
> +err_unlink_dev:
> + iommu_device_unlink(&viommu->iommu, dev);
> +
> +err_free_dev:
> + kfree(vdev);
> +
> + return ret;
> +}
> +
> +static void viommu_remove_device(struct device *dev)
> +{
> + struct viommu_endpoint *vdev;
> + struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +
> + if (!fwspec || fwspec->ops != &viommu_ops)
> + return;
> +
> + vdev = fwspec->iommu_priv;
> +
> + iommu_group_remove_device(dev);
> + iommu_device_unlink(&vdev->viommu->iommu, dev);
> + kfree(vdev);
> +}
> +
> +static struct iommu_group *viommu_device_group(struct device *dev)
> +{
> + if (dev_is_pci(dev))
> + return pci_device_group(dev);
> + else
> + return generic_device_group(dev);
> +}
> +
> +static int viommu_of_xlate(struct device *dev, struct of_phandle_args *args)
> +{
> + return iommu_fwspec_add_ids(dev, args->args, 1);
> +}
> +
> +static struct iommu_ops viommu_ops = {
> + .domain_alloc = viommu_domain_alloc,
> + .domain_free = viommu_domain_free,
> + .attach_dev = viommu_attach_dev,
> + .map = viommu_map,
> + .unmap = viommu_unmap,
> + .map_sg = default_iommu_map_sg,
> + .iova_to_phys = viommu_iova_to_phys,
> + .iotlb_sync = viommu_iotlb_sync,
> + .add_device = viommu_add_device,
> + .remove_device = viommu_remove_device,
> + .device_group = viommu_device_group,
> + .get_resv_regions = viommu_get_resv_regions,
> + .put_resv_regions = viommu_put_resv_regions,
> + .of_xlate = viommu_of_xlate,
> +};
> +
> +static int viommu_init_vqs(struct viommu_dev *viommu)
> +{
> + struct virtio_device *vdev = dev_to_virtio(viommu->dev);
> + const char *name = "request";
> + void *ret;
> +
> + ret = virtio_find_single_vq(vdev, NULL, name);
> + if (IS_ERR(ret)) {
> + dev_err(viommu->dev, "cannot find VQ\n");
> + return PTR_ERR(ret);
> + }
> +
> + viommu->vqs[VIOMMU_REQUEST_VQ] = ret;
> +
> + return 0;
> +}
> +
> +static int viommu_probe(struct virtio_device *vdev)
> +{
> + struct device *parent_dev = vdev->dev.parent;
> + struct viommu_dev *viommu = NULL;
> + struct device *dev = &vdev->dev;
> + u64 input_start = 0;
> + u64 input_end = -1UL;
> + int ret;
Please validate that device has VIRTIO_F_VERSION_1 -
we don't ever want new devices to grow legacy
interfaces.
> +
> + viommu = devm_kzalloc(dev, sizeof(*viommu), GFP_KERNEL);
> + if (!viommu)
> + return -ENOMEM;
> +
> + spin_lock_init(&viommu->request_lock);
> + ida_init(&viommu->domain_ids);
> + viommu->dev = dev;
> + viommu->vdev = vdev;
> + INIT_LIST_HEAD(&viommu->requests);
> +
> + ret = viommu_init_vqs(viommu);
> + if (ret)
> + return ret;
> +
> + virtio_cread(vdev, struct virtio_iommu_config, page_size_mask,
> + &viommu->pgsize_bitmap);
> +
> + if (!viommu->pgsize_bitmap) {
> + ret = -EINVAL;
> + goto err_free_vqs;
> + }
> +
> + viommu->domain_bits = 32;
> +
> + /* Optional features */
> + virtio_cread_feature(vdev, VIRTIO_IOMMU_F_INPUT_RANGE,
> + struct virtio_iommu_config, input_range.start,
> + &input_start);
> +
> + virtio_cread_feature(vdev, VIRTIO_IOMMU_F_INPUT_RANGE,
> + struct virtio_iommu_config, input_range.end,
> + &input_end);
> +
> + virtio_cread_feature(vdev, VIRTIO_IOMMU_F_DOMAIN_BITS,
> + struct virtio_iommu_config, domain_bits,
> + &viommu->domain_bits);
> +
> + viommu->geometry = (struct iommu_domain_geometry) {
> + .aperture_start = input_start,
> + .aperture_end = input_end,
> + .force_aperture = true,
> + };
> +
> + viommu_ops.pgsize_bitmap = viommu->pgsize_bitmap;
> +
> + virtio_device_ready(vdev);
> +
> + ret = iommu_device_sysfs_add(&viommu->iommu, dev, NULL, "%s",
> + virtio_bus_name(vdev));
> + if (ret)
> + goto err_free_vqs;
> +
> + iommu_device_set_ops(&viommu->iommu, &viommu_ops);
> + iommu_device_set_fwnode(&viommu->iommu, parent_dev->fwnode);
> +
> + iommu_device_register(&viommu->iommu);
> +
> +#ifdef CONFIG_PCI
> + if (pci_bus_type.iommu_ops != &viommu_ops) {
> + pci_request_acs();
> + ret = bus_set_iommu(&pci_bus_type, &viommu_ops);
> + if (ret)
> + goto err_unregister;
> + }
> +#endif
> +#ifdef CONFIG_ARM_AMBA
> + if (amba_bustype.iommu_ops != &viommu_ops) {
> + ret = bus_set_iommu(&amba_bustype, &viommu_ops);
> + if (ret)
> + goto err_unregister;
> + }
> +#endif
> + if (platform_bus_type.iommu_ops != &viommu_ops) {
> + ret = bus_set_iommu(&platform_bus_type, &viommu_ops);
> + if (ret)
> + goto err_unregister;
> + }
> +
> + vdev->priv = viommu;
> +
> + dev_info(dev, "input address: %u bits\n",
> + order_base_2(viommu->geometry.aperture_end));
> + dev_info(dev, "page mask: %#llx\n", viommu->pgsize_bitmap);
> +
> + return 0;
> +
> +err_unregister:
> + iommu_device_sysfs_remove(&viommu->iommu);
> + iommu_device_unregister(&viommu->iommu);
> +err_free_vqs:
> + vdev->config->del_vqs(vdev);
> +
> + return ret;
> +}
> +
> +static void viommu_remove(struct virtio_device *vdev)
> +{
> + struct viommu_dev *viommu = vdev->priv;
> +
> + iommu_device_sysfs_remove(&viommu->iommu);
> + iommu_device_unregister(&viommu->iommu);
> +
> + /* Stop all virtqueues */
> + vdev->config->reset(vdev);
> + vdev->config->del_vqs(vdev);
> +
> + dev_info(&vdev->dev, "device removed\n");
> +}
> +
> +static void viommu_config_changed(struct virtio_device *vdev)
> +{
> + dev_warn(&vdev->dev, "config changed\n");
> +}
> +
> +static unsigned int features[] = {
> + VIRTIO_IOMMU_F_MAP_UNMAP,
> + VIRTIO_IOMMU_F_DOMAIN_BITS,
> + VIRTIO_IOMMU_F_INPUT_RANGE,
> +};
> +
> +static struct virtio_device_id id_table[] = {
> + { VIRTIO_ID_IOMMU, VIRTIO_DEV_ANY_ID },
> + { 0 },
> +};
> +
> +static struct virtio_driver virtio_iommu_drv = {
> + .driver.name = KBUILD_MODNAME,
> + .driver.owner = THIS_MODULE,
> + .id_table = id_table,
> + .feature_table = features,
> + .feature_table_size = ARRAY_SIZE(features),
> + .probe = viommu_probe,
> + .remove = viommu_remove,
> + .config_changed = viommu_config_changed,
> +};
> +
> +module_virtio_driver(virtio_iommu_drv);
> +
> +IOMMU_OF_DECLARE(viommu, "virtio,mmio");
> +
> +MODULE_DESCRIPTION("Virtio IOMMU driver");
> +MODULE_AUTHOR("Jean-Philippe Brucker <jean-philippe.brucker@arm.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> index 6d5c3b2d4f4d..cfe47c5d9a56 100644
> --- a/include/uapi/linux/virtio_ids.h
> +++ b/include/uapi/linux/virtio_ids.h
> @@ -43,5 +43,6 @@
> #define VIRTIO_ID_INPUT 18 /* virtio input */
> #define VIRTIO_ID_VSOCK 19 /* virtio vsock transport */
> #define VIRTIO_ID_CRYPTO 20 /* virtio crypto */
> +#define VIRTIO_ID_IOMMU 23 /* virtio IOMMU */
>
> #endif /* _LINUX_VIRTIO_IDS_H */
> diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
> new file mode 100644
> index 000000000000..f4fafa0b41a7
> --- /dev/null
> +++ b/include/uapi/linux/virtio_iommu.h
> @@ -0,0 +1,117 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Virtio-iommu definition v0.7
> + *
> + * Copyright (C) 2018 Arm Ltd.
> + */
> +#ifndef _UAPI_LINUX_VIRTIO_IOMMU_H
> +#define _UAPI_LINUX_VIRTIO_IOMMU_H
> +
> +#include <linux/types.h>
> +
> +/* Feature bits */
> +#define VIRTIO_IOMMU_F_INPUT_RANGE 0
> +#define VIRTIO_IOMMU_F_DOMAIN_BITS 1
> +#define VIRTIO_IOMMU_F_MAP_UNMAP 2
> +#define VIRTIO_IOMMU_F_BYPASS 3
> +
> +struct virtio_iommu_config {
> + /* Supported page sizes */
> + __u64 page_size_mask;
> + /* Supported IOVA range */
> + struct virtio_iommu_range {
> + __u64 start;
> + __u64 end;
> + } input_range;
> + /* Max domain ID size */
> + __u8 domain_bits;
> +} __packed;
Please pad structs so each field and all of it are size aligned and avoid __packed.
Applies below too.
> +
> +/* Request types */
> +#define VIRTIO_IOMMU_T_ATTACH 0x01
> +#define VIRTIO_IOMMU_T_DETACH 0x02
> +#define VIRTIO_IOMMU_T_MAP 0x03
> +#define VIRTIO_IOMMU_T_UNMAP 0x04
> +
> +/* Status types */
> +#define VIRTIO_IOMMU_S_OK 0x00
> +#define VIRTIO_IOMMU_S_IOERR 0x01
> +#define VIRTIO_IOMMU_S_UNSUPP 0x02
> +#define VIRTIO_IOMMU_S_DEVERR 0x03
> +#define VIRTIO_IOMMU_S_INVAL 0x04
> +#define VIRTIO_IOMMU_S_RANGE 0x05
> +#define VIRTIO_IOMMU_S_NOENT 0x06
> +#define VIRTIO_IOMMU_S_FAULT 0x07
> +
> +struct virtio_iommu_req_head {
> + __u8 type;
> + __u8 reserved[3];
> +} __packed;
> +
> +struct virtio_iommu_req_tail {
> + __u8 status;
> + __u8 reserved[3];
> +} __packed;
> +
> +struct virtio_iommu_req_attach {
> + struct virtio_iommu_req_head head;
> +
> + __le32 domain;
> + __le32 endpoint;
> + __le32 reserved;
> +
> + struct virtio_iommu_req_tail tail;
> +} __packed;
> +
> +struct virtio_iommu_req_detach {
> + struct virtio_iommu_req_head head;
> +
> + __le32 endpoint;
> + __le32 reserved;
> +
> + struct virtio_iommu_req_tail tail;
> +} __packed;
> +
> +#define VIRTIO_IOMMU_MAP_F_READ (1 << 0)
> +#define VIRTIO_IOMMU_MAP_F_WRITE (1 << 1)
> +#define VIRTIO_IOMMU_MAP_F_EXEC (1 << 2)
> +#define VIRTIO_IOMMU_MAP_F_MMIO (1 << 3)
> +
> +#define VIRTIO_IOMMU_MAP_F_MASK (VIRTIO_IOMMU_MAP_F_READ | \
> + VIRTIO_IOMMU_MAP_F_WRITE | \
> + VIRTIO_IOMMU_MAP_F_EXEC | \
> + VIRTIO_IOMMU_MAP_F_MMIO)
> +
> +struct virtio_iommu_req_map {
> + struct virtio_iommu_req_head head;
> +
> + __le32 domain;
> + __le64 virt_start;
> + __le64 virt_end;
> + __le64 phys_start;
> + __le32 flags;
> +
> + struct virtio_iommu_req_tail tail;
> +} __packed;
> +
> +struct virtio_iommu_req_unmap {
> + struct virtio_iommu_req_head head;
> +
> + __le32 domain;
> + __le64 virt_start;
> + __le64 virt_end;
> + __le32 reserved;
> +
> + struct virtio_iommu_req_tail tail;
> +} __packed;
> +
> +union virtio_iommu_req {
> + struct virtio_iommu_req_head head;
> +
> + struct virtio_iommu_req_attach attach;
> + struct virtio_iommu_req_detach detach;
> + struct virtio_iommu_req_map map;
> + struct virtio_iommu_req_unmap unmap;
> +};
Such unions are problematic: if you add an option of a larger structure
down the road, do all requests grow automatically?
> +
> +#endif
> --
> 2.17.0
^ permalink raw reply
* [PATCH v2 5/5] vfio: Allow type-1 IOMMU instantiation for ARM
From: Jean-Philippe Brucker @ 2018-06-21 19:06 UTC (permalink / raw)
To: iommu, kvm, virtualization, virtio-dev, devicetree, kvmarm
Cc: mark.rutland, jayachandran.nair, lorenzo.pieralisi, tnowicki, mst,
marc.zyngier, joro, will.deacon, eric.auger, robh+dt,
robin.murphy
In-Reply-To: <20180621190655.56391-1-jean-philippe.brucker@arm.com>
ARM platforms may implement several kinds of IOMMUs (various SMMU or
SMMUv3 implementations, virtio-iommu). They are all type-1, so
automatically select VFIO_IOMMU_TYPE1 on ARM if IOMMU is selected.
Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
---
drivers/vfio/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index c84333eb5eb5..9de5ed38da83 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -21,7 +21,7 @@ config VFIO_VIRQFD
menuconfig VFIO
tristate "VFIO Non-Privileged userspace driver framework"
depends on IOMMU_API
- select VFIO_IOMMU_TYPE1 if (X86 || S390 || ARM_SMMU || ARM_SMMU_V3)
+ select VFIO_IOMMU_TYPE1 if (X86 || S390 || ARM || ARM64)
select ANON_INODES
help
VFIO provides a framework for secure userspace device drivers.
--
2.17.0
^ permalink raw reply related
* [PATCH v2 4/5] iommu/virtio: Add event queue
From: Jean-Philippe Brucker @ 2018-06-21 19:06 UTC (permalink / raw)
To: iommu, kvm, virtualization, virtio-dev, devicetree, kvmarm
Cc: mark.rutland, jayachandran.nair, lorenzo.pieralisi, tnowicki, mst,
marc.zyngier, joro, will.deacon, eric.auger, robh+dt,
robin.murphy
In-Reply-To: <20180621190655.56391-1-jean-philippe.brucker@arm.com>
The event queue offers a way for the device to report access faults from
endpoints. It is implemented on virtqueue #1. Whenever the host needs to
signal a fault, it fills one of the buffers offered by the guest and
interrupts it.
Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
---
drivers/iommu/virtio-iommu.c | 116 +++++++++++++++++++++++++++---
include/uapi/linux/virtio_iommu.h | 18 +++++
2 files changed, 125 insertions(+), 9 deletions(-)
diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 29ce9f4398ef..c075404e97d6 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -29,7 +29,8 @@
#define MSI_IOVA_LENGTH 0x100000
#define VIOMMU_REQUEST_VQ 0
-#define VIOMMU_NR_VQS 1
+#define VIOMMU_EVENT_VQ 1
+#define VIOMMU_NR_VQS 2
#define VIOMMU_REQUEST_TIMEOUT 10000 /* 10s */
@@ -43,6 +44,7 @@ struct viommu_dev {
struct virtqueue *vqs[VIOMMU_NR_VQS];
spinlock_t request_lock;
struct list_head requests;
+ void *evts;
/* Device configuration */
struct iommu_domain_geometry geometry;
@@ -84,6 +86,15 @@ struct viommu_request {
char buf[];
};
+#define VIOMMU_FAULT_RESV_MASK 0xffffff00
+
+struct viommu_event {
+ union {
+ u32 head;
+ struct virtio_iommu_fault fault;
+ };
+};
+
#define to_viommu_domain(domain) \
container_of(domain, struct viommu_domain, domain)
@@ -507,6 +518,69 @@ static int viommu_probe_endpoint(struct viommu_dev *viommu, struct device *dev)
return ret;
}
+static int viommu_fault_handler(struct viommu_dev *viommu,
+ struct virtio_iommu_fault *fault)
+{
+ char *reason_str;
+
+ u8 reason = fault->reason;
+ u32 flags = le32_to_cpu(fault->flags);
+ u32 endpoint = le32_to_cpu(fault->endpoint);
+ u64 address = le64_to_cpu(fault->address);
+
+ switch (reason) {
+ case VIRTIO_IOMMU_FAULT_R_DOMAIN:
+ reason_str = "domain";
+ break;
+ case VIRTIO_IOMMU_FAULT_R_MAPPING:
+ reason_str = "page";
+ break;
+ case VIRTIO_IOMMU_FAULT_R_UNKNOWN:
+ default:
+ reason_str = "unknown";
+ break;
+ }
+
+ /* TODO: find EP by ID and report_iommu_fault */
+ if (flags & VIRTIO_IOMMU_FAULT_F_ADDRESS)
+ dev_err_ratelimited(viommu->dev, "%s fault from EP %u at %#llx [%s%s%s]\n",
+ reason_str, endpoint, address,
+ flags & VIRTIO_IOMMU_FAULT_F_READ ? "R" : "",
+ flags & VIRTIO_IOMMU_FAULT_F_WRITE ? "W" : "",
+ flags & VIRTIO_IOMMU_FAULT_F_EXEC ? "X" : "");
+ else
+ dev_err_ratelimited(viommu->dev, "%s fault from EP %u\n",
+ reason_str, endpoint);
+ return 0;
+}
+
+static void viommu_event_handler(struct virtqueue *vq)
+{
+ int ret;
+ unsigned int len;
+ struct scatterlist sg[1];
+ struct viommu_event *evt;
+ struct viommu_dev *viommu = vq->vdev->priv;
+
+ while ((evt = virtqueue_get_buf(vq, &len)) != NULL) {
+ if (len > sizeof(*evt)) {
+ dev_err(viommu->dev,
+ "invalid event buffer (len %u != %zu)\n",
+ len, sizeof(*evt));
+ } else if (!(evt->head & VIOMMU_FAULT_RESV_MASK)) {
+ viommu_fault_handler(viommu, &evt->fault);
+ }
+
+ sg_init_one(sg, evt, sizeof(*evt));
+ ret = virtqueue_add_inbuf(vq, sg, 1, evt, GFP_ATOMIC);
+ if (ret)
+ dev_err(viommu->dev, "could not add event buffer\n");
+ }
+
+ if (!virtqueue_kick(vq))
+ dev_err(viommu->dev, "kick failed\n");
+}
+
/* IOMMU API */
static struct iommu_domain *viommu_domain_alloc(unsigned type)
@@ -893,16 +967,35 @@ static struct iommu_ops viommu_ops = {
static int viommu_init_vqs(struct viommu_dev *viommu)
{
struct virtio_device *vdev = dev_to_virtio(viommu->dev);
- const char *name = "request";
- void *ret;
+ const char *names[] = { "request", "event" };
+ vq_callback_t *callbacks[] = {
+ NULL, /* No async requests */
+ viommu_event_handler,
+ };
- ret = virtio_find_single_vq(vdev, NULL, name);
- if (IS_ERR(ret)) {
- dev_err(viommu->dev, "cannot find VQ\n");
- return PTR_ERR(ret);
- }
+ return virtio_find_vqs(vdev, VIOMMU_NR_VQS, viommu->vqs, callbacks,
+ names, NULL);
+}
- viommu->vqs[VIOMMU_REQUEST_VQ] = ret;
+static int viommu_fill_evtq(struct viommu_dev *viommu)
+{
+ int i, ret;
+ struct scatterlist sg[1];
+ struct viommu_event *evts;
+ struct virtqueue *vq = viommu->vqs[VIOMMU_EVENT_VQ];
+ size_t nr_evts = vq->num_free;
+
+ viommu->evts = evts = devm_kmalloc_array(viommu->dev, nr_evts,
+ sizeof(*evts), GFP_KERNEL);
+ if (!evts)
+ return -ENOMEM;
+
+ for (i = 0; i < nr_evts; i++) {
+ sg_init_one(sg, &evts[i], sizeof(*evts));
+ ret = virtqueue_add_inbuf(vq, sg, 1, &evts[i], GFP_KERNEL);
+ if (ret)
+ return ret;
+ }
return 0;
}
@@ -967,6 +1060,11 @@ static int viommu_probe(struct virtio_device *vdev)
virtio_device_ready(vdev);
+ /* Populate the event queue with buffers */
+ ret = viommu_fill_evtq(viommu);
+ if (ret)
+ goto err_free_vqs;
+
ret = iommu_device_sysfs_add(&viommu->iommu, dev, NULL, "%s",
virtio_bus_name(vdev));
if (ret)
diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
index 2fd802a860cd..4e501d34267a 100644
--- a/include/uapi/linux/virtio_iommu.h
+++ b/include/uapi/linux/virtio_iommu.h
@@ -151,4 +151,22 @@ union virtio_iommu_req {
struct virtio_iommu_req_probe probe;
};
+/* Fault types */
+#define VIRTIO_IOMMU_FAULT_R_UNKNOWN 0
+#define VIRTIO_IOMMU_FAULT_R_DOMAIN 1
+#define VIRTIO_IOMMU_FAULT_R_MAPPING 2
+
+#define VIRTIO_IOMMU_FAULT_F_READ (1 << 0)
+#define VIRTIO_IOMMU_FAULT_F_WRITE (1 << 1)
+#define VIRTIO_IOMMU_FAULT_F_EXEC (1 << 2)
+#define VIRTIO_IOMMU_FAULT_F_ADDRESS (1 << 8)
+
+struct virtio_iommu_fault {
+ __u8 reason;
+ __u8 padding[3];
+ __le32 flags;
+ __le32 endpoint;
+ __le64 address;
+} __packed;
+
#endif
--
2.17.0
^ permalink raw reply related
* [PATCH v2 3/5] iommu/virtio: Add probe request
From: Jean-Philippe Brucker @ 2018-06-21 19:06 UTC (permalink / raw)
To: iommu, kvm, virtualization, virtio-dev, devicetree, kvmarm
Cc: mark.rutland, jayachandran.nair, lorenzo.pieralisi, tnowicki, mst,
marc.zyngier, joro, will.deacon, eric.auger, robh+dt,
robin.murphy
In-Reply-To: <20180621190655.56391-1-jean-philippe.brucker@arm.com>
When the device offers the probe feature, send a probe request for each
device managed by the IOMMU. Extract RESV_MEM information. When we
encounter a MSI doorbell region, set it up as a IOMMU_RESV_MSI region.
This will tell other subsystems that there is no need to map the MSI
doorbell in the virtio-iommu, because MSIs bypass it.
Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
---
drivers/iommu/virtio-iommu.c | 149 ++++++++++++++++++++++++++++--
include/uapi/linux/virtio_iommu.h | 37 ++++++++
2 files changed, 180 insertions(+), 6 deletions(-)
diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index ea0242d8624b..29ce9f4398ef 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -48,6 +48,7 @@ struct viommu_dev {
struct iommu_domain_geometry geometry;
u64 pgsize_bitmap;
u8 domain_bits;
+ u32 probe_size;
};
struct viommu_mapping {
@@ -69,8 +70,10 @@ struct viommu_domain {
};
struct viommu_endpoint {
+ struct device *dev;
struct viommu_dev *viommu;
struct viommu_domain *vdomain;
+ struct list_head resv_regions;
};
struct viommu_request {
@@ -121,6 +124,9 @@ static off_t viommu_get_req_offset(struct viommu_dev *viommu,
{
size_t tail_size = sizeof(struct virtio_iommu_req_tail);
+ if (req->type == VIRTIO_IOMMU_T_PROBE)
+ return len - viommu->probe_size - tail_size;
+
return len - tail_size;
}
@@ -404,6 +410,103 @@ static int viommu_replay_mappings(struct viommu_domain *vdomain)
return ret;
}
+static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
+ struct virtio_iommu_probe_resv_mem *mem,
+ size_t len)
+{
+ struct iommu_resv_region *region = NULL;
+ unsigned long prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+
+ u64 start = le64_to_cpu(mem->start);
+ u64 end = le64_to_cpu(mem->end);
+ size_t size = end - start + 1;
+
+ if (len < sizeof(*mem))
+ return -EINVAL;
+
+ switch (mem->subtype) {
+ default:
+ if (mem->subtype != VIRTIO_IOMMU_RESV_MEM_T_RESERVED &&
+ mem->subtype != VIRTIO_IOMMU_RESV_MEM_T_MSI)
+ dev_warn(vdev->dev, "unknown resv mem subtype 0x%x\n",
+ mem->subtype);
+ /* Fall-through */
+ case VIRTIO_IOMMU_RESV_MEM_T_RESERVED:
+ region = iommu_alloc_resv_region(start, size, 0,
+ IOMMU_RESV_RESERVED);
+ break;
+ case VIRTIO_IOMMU_RESV_MEM_T_MSI:
+ region = iommu_alloc_resv_region(start, size, prot,
+ IOMMU_RESV_MSI);
+ break;
+ }
+
+ list_add(&vdev->resv_regions, ®ion->list);
+ return 0;
+}
+
+static int viommu_probe_endpoint(struct viommu_dev *viommu, struct device *dev)
+{
+ int ret;
+ u16 type, len;
+ size_t cur = 0;
+ size_t probe_len;
+ struct virtio_iommu_req_probe *probe;
+ struct virtio_iommu_probe_property *prop;
+ struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+ struct viommu_endpoint *vdev = fwspec->iommu_priv;
+
+ if (!fwspec->num_ids)
+ return -EINVAL;
+
+ probe_len = sizeof(*probe) + viommu->probe_size +
+ sizeof(struct virtio_iommu_req_tail);
+ probe = kzalloc(probe_len, GFP_KERNEL);
+ if (!probe)
+ return -ENOMEM;
+
+ probe->head.type = VIRTIO_IOMMU_T_PROBE;
+ /*
+ * For now, assume that properties of an endpoint that outputs multiple
+ * IDs are consistent. Only probe the first one.
+ */
+ probe->endpoint = cpu_to_le32(fwspec->ids[0]);
+
+ ret = viommu_send_req_sync(viommu, probe, probe_len);
+ if (ret)
+ goto out_free;
+
+ prop = (void *)probe->properties;
+ type = le16_to_cpu(prop->type) & VIRTIO_IOMMU_PROBE_T_MASK;
+
+ while (type != VIRTIO_IOMMU_PROBE_T_NONE &&
+ cur < viommu->probe_size) {
+ len = le16_to_cpu(prop->length);
+
+ switch (type) {
+ case VIRTIO_IOMMU_PROBE_T_RESV_MEM:
+ ret = viommu_add_resv_mem(vdev, (void *)prop->value, len);
+ break;
+ default:
+ dev_dbg(dev, "unknown viommu prop 0x%x\n", type);
+ }
+
+ if (ret)
+ dev_err(dev, "failed to parse viommu prop 0x%x\n", type);
+
+ cur += sizeof(*prop) + len;
+ if (cur >= viommu->probe_size)
+ break;
+
+ prop = (void *)probe->properties + cur;
+ type = le16_to_cpu(prop->type) & VIRTIO_IOMMU_PROBE_T_MASK;
+ }
+
+out_free:
+ kfree(probe);
+ return ret;
+}
+
/* IOMMU API */
static struct iommu_domain *viommu_domain_alloc(unsigned type)
@@ -627,15 +730,33 @@ static void viommu_iotlb_sync(struct iommu_domain *domain)
static void viommu_get_resv_regions(struct device *dev, struct list_head *head)
{
- struct iommu_resv_region *region;
+ struct iommu_resv_region *entry, *new_entry, *msi = NULL;
+ struct viommu_endpoint *vdev = dev->iommu_fwspec->iommu_priv;
int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
- region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH, prot,
- IOMMU_RESV_SW_MSI);
- if (!region)
- return;
+ list_for_each_entry(entry, &vdev->resv_regions, list) {
+ /*
+ * If the device registered a bypass MSI windows, use it.
+ * Otherwise add a software-mapped region
+ */
+ if (entry->type == IOMMU_RESV_MSI)
+ msi = entry;
+
+ new_entry = kmemdup(entry, sizeof(*entry), GFP_KERNEL);
+ if (!new_entry)
+ return;
+ list_add_tail(&new_entry->list, head);
+ }
+
+ if (!msi) {
+ msi = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
+ prot, IOMMU_RESV_SW_MSI);
+ if (!msi)
+ return;
+
+ list_add_tail(&msi->list, head);
+ }
- list_add_tail(®ion->list, head);
iommu_dma_get_resv_regions(dev, head);
}
@@ -683,9 +804,18 @@ static int viommu_add_device(struct device *dev)
if (!vdev)
return -ENOMEM;
+ vdev->dev = dev;
vdev->viommu = viommu;
+ INIT_LIST_HEAD(&vdev->resv_regions);
fwspec->iommu_priv = vdev;
+ if (viommu->probe_size) {
+ /* Get additional information for this endpoint */
+ ret = viommu_probe_endpoint(viommu, dev);
+ if (ret)
+ return ret;
+ }
+
ret = iommu_device_link(&viommu->iommu, dev);
if (ret)
goto err_free_dev;
@@ -708,6 +838,7 @@ static int viommu_add_device(struct device *dev)
iommu_device_unlink(&viommu->iommu, dev);
err_free_dev:
+ viommu_put_resv_regions(dev, &vdev->resv_regions);
kfree(vdev);
return ret;
@@ -725,6 +856,7 @@ static void viommu_remove_device(struct device *dev)
iommu_group_remove_device(dev);
iommu_device_unlink(&vdev->viommu->iommu, dev);
+ viommu_put_resv_regions(dev, &vdev->resv_regions);
kfree(vdev);
}
@@ -821,6 +953,10 @@ static int viommu_probe(struct virtio_device *vdev)
struct virtio_iommu_config, domain_bits,
&viommu->domain_bits);
+ virtio_cread_feature(vdev, VIRTIO_IOMMU_F_PROBE,
+ struct virtio_iommu_config, probe_size,
+ &viommu->probe_size);
+
viommu->geometry = (struct iommu_domain_geometry) {
.aperture_start = input_start,
.aperture_end = input_end,
@@ -902,6 +1038,7 @@ static unsigned int features[] = {
VIRTIO_IOMMU_F_MAP_UNMAP,
VIRTIO_IOMMU_F_DOMAIN_BITS,
VIRTIO_IOMMU_F_INPUT_RANGE,
+ VIRTIO_IOMMU_F_PROBE,
};
static struct virtio_device_id id_table[] = {
diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
index f4fafa0b41a7..2fd802a860cd 100644
--- a/include/uapi/linux/virtio_iommu.h
+++ b/include/uapi/linux/virtio_iommu.h
@@ -14,6 +14,7 @@
#define VIRTIO_IOMMU_F_DOMAIN_BITS 1
#define VIRTIO_IOMMU_F_MAP_UNMAP 2
#define VIRTIO_IOMMU_F_BYPASS 3
+#define VIRTIO_IOMMU_F_PROBE 4
struct virtio_iommu_config {
/* Supported page sizes */
@@ -25,6 +26,9 @@ struct virtio_iommu_config {
} input_range;
/* Max domain ID size */
__u8 domain_bits;
+ __u8 padding[3];
+ /* Probe buffer size */
+ __u32 probe_size;
} __packed;
/* Request types */
@@ -32,6 +36,7 @@ struct virtio_iommu_config {
#define VIRTIO_IOMMU_T_DETACH 0x02
#define VIRTIO_IOMMU_T_MAP 0x03
#define VIRTIO_IOMMU_T_UNMAP 0x04
+#define VIRTIO_IOMMU_T_PROBE 0x05
/* Status types */
#define VIRTIO_IOMMU_S_OK 0x00
@@ -105,6 +110,37 @@ struct virtio_iommu_req_unmap {
struct virtio_iommu_req_tail tail;
} __packed;
+#define VIRTIO_IOMMU_RESV_MEM_T_RESERVED 0
+#define VIRTIO_IOMMU_RESV_MEM_T_MSI 1
+
+struct virtio_iommu_probe_resv_mem {
+ __u8 subtype;
+ __u8 reserved[3];
+ __le64 start;
+ __le64 end;
+} __packed;
+
+#define VIRTIO_IOMMU_PROBE_T_NONE 0
+#define VIRTIO_IOMMU_PROBE_T_RESV_MEM 1
+
+#define VIRTIO_IOMMU_PROBE_T_MASK 0xfff
+
+struct virtio_iommu_probe_property {
+ __le16 type;
+ __le16 length;
+ __u8 value[];
+} __packed;
+
+struct virtio_iommu_req_probe {
+ struct virtio_iommu_req_head head;
+ __le32 endpoint;
+ __u8 reserved[64];
+
+ __u8 properties[];
+
+ /* Tail follows the variable-length properties array (no padding) */
+} __packed;
+
union virtio_iommu_req {
struct virtio_iommu_req_head head;
@@ -112,6 +148,7 @@ union virtio_iommu_req {
struct virtio_iommu_req_detach detach;
struct virtio_iommu_req_map map;
struct virtio_iommu_req_unmap unmap;
+ struct virtio_iommu_req_probe probe;
};
#endif
--
2.17.0
^ permalink raw reply related
* [PATCH v2 2/5] iommu: Add virtio-iommu driver
From: Jean-Philippe Brucker @ 2018-06-21 19:06 UTC (permalink / raw)
To: iommu, kvm, virtualization, virtio-dev, devicetree, kvmarm
Cc: mark.rutland, jayachandran.nair, lorenzo.pieralisi, tnowicki, mst,
marc.zyngier, joro, will.deacon, eric.auger, robh+dt,
robin.murphy
In-Reply-To: <20180621190655.56391-1-jean-philippe.brucker@arm.com>
The virtio IOMMU is a para-virtualized device, allowing to send IOMMU
requests such as map/unmap over virtio-mmio transport without emulating
page tables. This implementation handles ATTACH, DETACH, MAP and UNMAP
requests.
The bulk of the code transforms calls coming from the IOMMU API into
corresponding virtio requests. Mappings are kept in an interval tree
instead of page tables.
Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
---
MAINTAINERS | 6 +
drivers/iommu/Kconfig | 11 +
drivers/iommu/Makefile | 1 +
drivers/iommu/virtio-iommu.c | 929 ++++++++++++++++++++++++++++++
include/uapi/linux/virtio_ids.h | 1 +
include/uapi/linux/virtio_iommu.h | 117 ++++
6 files changed, 1065 insertions(+)
create mode 100644 drivers/iommu/virtio-iommu.c
create mode 100644 include/uapi/linux/virtio_iommu.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 55c08968aaab..bfb09dfa8e02 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15248,6 +15248,12 @@ S: Maintained
F: drivers/virtio/virtio_input.c
F: include/uapi/linux/virtio_input.h
+VIRTIO IOMMU DRIVER
+M: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
+S: Maintained
+F: drivers/iommu/virtio-iommu.c
+F: include/uapi/linux/virtio_iommu.h
+
VIRTUAL BOX GUEST DEVICE DRIVER
M: Hans de Goede <hdegoede@redhat.com>
M: Arnd Bergmann <arnd@arndb.de>
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 958417f22020..820709383899 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -412,4 +412,15 @@ config QCOM_IOMMU
help
Support for IOMMU on certain Qualcomm SoCs.
+config VIRTIO_IOMMU
+ bool "Virtio IOMMU driver"
+ depends on VIRTIO_MMIO=y
+ select IOMMU_API
+ select INTERVAL_TREE
+ select ARM_DMA_USE_IOMMU if ARM
+ help
+ Para-virtualised IOMMU driver with virtio.
+
+ Say Y here if you intend to run this kernel as a guest.
+
endif # IOMMU_SUPPORT
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 244ad7913a81..b559c6ae81ea 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -33,3 +33,4 @@ obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o
obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o
obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
obj-$(CONFIG_QCOM_IOMMU) += qcom_iommu.o
+obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
new file mode 100644
index 000000000000..ea0242d8624b
--- /dev/null
+++ b/drivers/iommu/virtio-iommu.c
@@ -0,0 +1,929 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Virtio driver for the paravirtualized IOMMU
+ *
+ * Copyright (C) 2018 Arm Limited
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/amba/bus.h>
+#include <linux/delay.h>
+#include <linux/dma-iommu.h>
+#include <linux/freezer.h>
+#include <linux/interval_tree.h>
+#include <linux/iommu.h>
+#include <linux/module.h>
+#include <linux/of_iommu.h>
+#include <linux/of_platform.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/virtio.h>
+#include <linux/virtio_config.h>
+#include <linux/virtio_ids.h>
+#include <linux/wait.h>
+
+#include <uapi/linux/virtio_iommu.h>
+
+#define MSI_IOVA_BASE 0x8000000
+#define MSI_IOVA_LENGTH 0x100000
+
+#define VIOMMU_REQUEST_VQ 0
+#define VIOMMU_NR_VQS 1
+
+#define VIOMMU_REQUEST_TIMEOUT 10000 /* 10s */
+
+struct viommu_dev {
+ struct iommu_device iommu;
+ struct device *dev;
+ struct virtio_device *vdev;
+
+ struct ida domain_ids;
+
+ struct virtqueue *vqs[VIOMMU_NR_VQS];
+ spinlock_t request_lock;
+ struct list_head requests;
+
+ /* Device configuration */
+ struct iommu_domain_geometry geometry;
+ u64 pgsize_bitmap;
+ u8 domain_bits;
+};
+
+struct viommu_mapping {
+ phys_addr_t paddr;
+ struct interval_tree_node iova;
+ u32 flags;
+};
+
+struct viommu_domain {
+ struct iommu_domain domain;
+ struct viommu_dev *viommu;
+ struct mutex mutex;
+ unsigned int id;
+
+ spinlock_t mappings_lock;
+ struct rb_root_cached mappings;
+
+ unsigned long nr_endpoints;
+};
+
+struct viommu_endpoint {
+ struct viommu_dev *viommu;
+ struct viommu_domain *vdomain;
+};
+
+struct viommu_request {
+ struct list_head list;
+ void *writeback;
+ unsigned int write_offset;
+ unsigned int len;
+ char buf[];
+};
+
+#define to_viommu_domain(domain) \
+ container_of(domain, struct viommu_domain, domain)
+
+static int viommu_get_req_errno(void *buf, size_t len)
+{
+ struct virtio_iommu_req_tail *tail = buf + len - sizeof(*tail);
+
+ switch (tail->status) {
+ case VIRTIO_IOMMU_S_OK:
+ return 0;
+ case VIRTIO_IOMMU_S_UNSUPP:
+ return -ENOSYS;
+ case VIRTIO_IOMMU_S_INVAL:
+ return -EINVAL;
+ case VIRTIO_IOMMU_S_RANGE:
+ return -ERANGE;
+ case VIRTIO_IOMMU_S_NOENT:
+ return -ENOENT;
+ case VIRTIO_IOMMU_S_FAULT:
+ return -EFAULT;
+ case VIRTIO_IOMMU_S_IOERR:
+ case VIRTIO_IOMMU_S_DEVERR:
+ default:
+ return -EIO;
+ }
+}
+
+static void viommu_set_req_status(void *buf, size_t len, int status)
+{
+ struct virtio_iommu_req_tail *tail = buf + len - sizeof(*tail);
+
+ tail->status = status;
+}
+
+static off_t viommu_get_req_offset(struct viommu_dev *viommu,
+ struct virtio_iommu_req_head *req,
+ size_t len)
+{
+ size_t tail_size = sizeof(struct virtio_iommu_req_tail);
+
+ return len - tail_size;
+}
+
+/*
+ * __viommu_sync_req - Complete all in-flight requests
+ *
+ * Wait for all added requests to complete. When this function returns, all
+ * requests that were in-flight at the time of the call have completed.
+ */
+static int __viommu_sync_req(struct viommu_dev *viommu)
+{
+ int ret = 0;
+ unsigned int len;
+ size_t write_len;
+ struct viommu_request *req;
+ struct virtqueue *vq = viommu->vqs[VIOMMU_REQUEST_VQ];
+ ktime_t timeout = ktime_add_ms(ktime_get(), VIOMMU_REQUEST_TIMEOUT);
+
+ assert_spin_locked(&viommu->request_lock);
+
+ virtqueue_kick(vq);
+
+ while (!list_empty(&viommu->requests)) {
+ len = 0;
+ req = virtqueue_get_buf(vq, &len);
+ if (req == NULL) {
+ if (ktime_before(ktime_get(), timeout))
+ continue;
+
+ /* After timeout, remove all requests */
+ req = list_first_entry(&viommu->requests,
+ struct viommu_request, list);
+ ret = -ETIMEDOUT;
+ }
+
+ if (!len)
+ viommu_set_req_status(req->buf, req->len,
+ VIRTIO_IOMMU_S_IOERR);
+
+ write_len = req->len - req->write_offset;
+ if (req->writeback && len >= write_len)
+ memcpy(req->writeback, req->buf + req->write_offset,
+ write_len);
+
+ list_del(&req->list);
+ kfree(req);
+ }
+
+ return ret;
+}
+
+static int viommu_sync_req(struct viommu_dev *viommu)
+{
+ int ret;
+ unsigned long flags;
+
+ spin_lock_irqsave(&viommu->request_lock, flags);
+ ret = __viommu_sync_req(viommu);
+ if (ret)
+ dev_dbg(viommu->dev, "could not sync requests (%d)\n", ret);
+ spin_unlock_irqrestore(&viommu->request_lock, flags);
+
+ return ret;
+}
+
+/*
+ * __viommu_add_request - Add one request to the queue
+ * @buf: pointer to the request buffer
+ * @len: length of the request buffer
+ * @writeback: copy data back to the buffer when the request completes.
+ *
+ * Add a request to the queue. Only synchronize the queue if it's already full.
+ * Otherwise don't kick the queue nor wait for requests to complete.
+ *
+ * When @writeback is true, data written by the device, including the request
+ * status, is copied into @buf after the request completes. This is unsafe if
+ * the caller allocates @buf on stack and drops the lock between add_req() and
+ * sync_req().
+ *
+ * Return 0 if the request was successfully added to the queue.
+ */
+static int __viommu_add_req(struct viommu_dev *viommu, void *buf, size_t len,
+ bool writeback)
+{
+ int ret;
+ off_t write_offset;
+ struct viommu_request *req;
+ struct scatterlist top_sg, bottom_sg;
+ struct scatterlist *sg[2] = { &top_sg, &bottom_sg };
+ struct virtqueue *vq = viommu->vqs[VIOMMU_REQUEST_VQ];
+
+ assert_spin_locked(&viommu->request_lock);
+
+ write_offset = viommu_get_req_offset(viommu, buf, len);
+ if (!write_offset)
+ return -EINVAL;
+
+ req = kzalloc(sizeof(*req) + len, GFP_ATOMIC);
+ if (!req)
+ return -ENOMEM;
+
+ req->len = len;
+ if (writeback) {
+ req->writeback = buf + write_offset;
+ req->write_offset = write_offset;
+ }
+ memcpy(&req->buf, buf, write_offset);
+
+ sg_init_one(&top_sg, req->buf, write_offset);
+ sg_init_one(&bottom_sg, req->buf + write_offset, len - write_offset);
+
+ ret = virtqueue_add_sgs(vq, sg, 1, 1, req, GFP_ATOMIC);
+ if (ret == -ENOSPC) {
+ /* If the queue is full, sync and retry */
+ if (!__viommu_sync_req(viommu))
+ ret = virtqueue_add_sgs(vq, sg, 1, 1, req, GFP_ATOMIC);
+ }
+ if (ret)
+ goto err_free;
+
+ list_add_tail(&req->list, &viommu->requests);
+ return 0;
+
+err_free:
+ kfree(req);
+ return ret;
+}
+
+static int viommu_add_req(struct viommu_dev *viommu, void *buf, size_t len)
+{
+ int ret;
+ unsigned long flags;
+
+ spin_lock_irqsave(&viommu->request_lock, flags);
+ ret = __viommu_add_req(viommu, buf, len, false);
+ if (ret)
+ dev_dbg(viommu->dev, "could not add request: %d\n", ret);
+ spin_unlock_irqrestore(&viommu->request_lock, flags);
+
+ return ret;
+}
+
+/*
+ * Send a request and wait for it to complete. Return the request status (as an
+ * errno)
+ */
+static int viommu_send_req_sync(struct viommu_dev *viommu, void *buf,
+ size_t len)
+{
+ int ret;
+ unsigned long flags;
+
+ spin_lock_irqsave(&viommu->request_lock, flags);
+
+ ret = __viommu_add_req(viommu, buf, len, true);
+ if (ret) {
+ dev_dbg(viommu->dev, "could not add request (%d)\n", ret);
+ goto out_unlock;
+ }
+
+ ret = __viommu_sync_req(viommu);
+ if (ret) {
+ dev_dbg(viommu->dev, "could not sync requests (%d)\n", ret);
+ /* Fall-through (get the actual request status) */
+ }
+
+ ret = viommu_get_req_errno(buf, len);
+out_unlock:
+ spin_unlock_irqrestore(&viommu->request_lock, flags);
+ return ret;
+}
+
+/*
+ * viommu_add_mapping - add a mapping to the internal tree
+ *
+ * On success, return the new mapping. Otherwise return NULL.
+ */
+static struct viommu_mapping *
+viommu_add_mapping(struct viommu_domain *vdomain, unsigned long iova,
+ phys_addr_t paddr, size_t size, u32 flags)
+{
+ unsigned long irqflags;
+ struct viommu_mapping *mapping;
+
+ mapping = kzalloc(sizeof(*mapping), GFP_ATOMIC);
+ if (!mapping)
+ return NULL;
+
+ mapping->paddr = paddr;
+ mapping->iova.start = iova;
+ mapping->iova.last = iova + size - 1;
+ mapping->flags = flags;
+
+ spin_lock_irqsave(&vdomain->mappings_lock, irqflags);
+ interval_tree_insert(&mapping->iova, &vdomain->mappings);
+ spin_unlock_irqrestore(&vdomain->mappings_lock, irqflags);
+
+ return mapping;
+}
+
+/*
+ * viommu_del_mappings - remove mappings from the internal tree
+ *
+ * @vdomain: the domain
+ * @iova: start of the range
+ * @size: size of the range. A size of 0 corresponds to the entire address
+ * space.
+ *
+ * On success, returns the number of unmapped bytes (>= size)
+ */
+static size_t viommu_del_mappings(struct viommu_domain *vdomain,
+ unsigned long iova, size_t size)
+{
+ size_t unmapped = 0;
+ unsigned long flags;
+ unsigned long last = iova + size - 1;
+ struct viommu_mapping *mapping = NULL;
+ struct interval_tree_node *node, *next;
+
+ spin_lock_irqsave(&vdomain->mappings_lock, flags);
+ next = interval_tree_iter_first(&vdomain->mappings, iova, last);
+ while (next) {
+ node = next;
+ mapping = container_of(node, struct viommu_mapping, iova);
+ next = interval_tree_iter_next(node, iova, last);
+
+ /* Trying to split a mapping? */
+ if (mapping->iova.start < iova)
+ break;
+
+ /*
+ * Note that for a partial range, this will return the full
+ * mapping so we avoid sending split requests to the device.
+ */
+ unmapped += mapping->iova.last - mapping->iova.start + 1;
+
+ interval_tree_remove(node, &vdomain->mappings);
+ kfree(mapping);
+ }
+ spin_unlock_irqrestore(&vdomain->mappings_lock, flags);
+
+ return unmapped;
+}
+
+/*
+ * viommu_replay_mappings - re-send MAP requests
+ *
+ * When reattaching a domain that was previously detached from all endpoints,
+ * mappings were deleted from the device. Re-create the mappings available in
+ * the internal tree.
+ */
+static int viommu_replay_mappings(struct viommu_domain *vdomain)
+{
+ int ret;
+ unsigned long flags;
+ struct viommu_mapping *mapping;
+ struct interval_tree_node *node;
+ struct virtio_iommu_req_map map;
+
+ spin_lock_irqsave(&vdomain->mappings_lock, flags);
+ node = interval_tree_iter_first(&vdomain->mappings, 0, -1UL);
+ while (node) {
+ mapping = container_of(node, struct viommu_mapping, iova);
+ map = (struct virtio_iommu_req_map) {
+ .head.type = VIRTIO_IOMMU_T_MAP,
+ .domain = cpu_to_le32(vdomain->id),
+ .virt_start = cpu_to_le64(mapping->iova.start),
+ .virt_end = cpu_to_le64(mapping->iova.last),
+ .phys_start = cpu_to_le64(mapping->paddr),
+ .flags = cpu_to_le32(mapping->flags),
+ };
+
+ ret = viommu_send_req_sync(vdomain->viommu, &map, sizeof(map));
+ if (ret)
+ break;
+
+ node = interval_tree_iter_next(node, 0, -1UL);
+ }
+ spin_unlock_irqrestore(&vdomain->mappings_lock, flags);
+
+ return ret;
+}
+
+/* IOMMU API */
+
+static struct iommu_domain *viommu_domain_alloc(unsigned type)
+{
+ struct viommu_domain *vdomain;
+
+ if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
+ return NULL;
+
+ vdomain = kzalloc(sizeof(*vdomain), GFP_KERNEL);
+ if (!vdomain)
+ return NULL;
+
+ mutex_init(&vdomain->mutex);
+ spin_lock_init(&vdomain->mappings_lock);
+ vdomain->mappings = RB_ROOT_CACHED;
+
+ if (type == IOMMU_DOMAIN_DMA &&
+ iommu_get_dma_cookie(&vdomain->domain)) {
+ kfree(vdomain);
+ return NULL;
+ }
+
+ return &vdomain->domain;
+}
+
+static int viommu_domain_finalise(struct viommu_dev *viommu,
+ struct iommu_domain *domain)
+{
+ int ret;
+ struct viommu_domain *vdomain = to_viommu_domain(domain);
+ /* ida limits size to 31 bits. A value of 0 means "max" */
+ unsigned int max_domain = viommu->domain_bits >= 31 ? 0 :
+ 1U << viommu->domain_bits;
+
+ vdomain->viommu = viommu;
+
+ domain->pgsize_bitmap = viommu->pgsize_bitmap;
+ domain->geometry = viommu->geometry;
+
+ ret = ida_simple_get(&viommu->domain_ids, 0, max_domain, GFP_KERNEL);
+ if (ret >= 0)
+ vdomain->id = (unsigned int)ret;
+
+ return ret > 0 ? 0 : ret;
+}
+
+static void viommu_domain_free(struct iommu_domain *domain)
+{
+ struct viommu_domain *vdomain = to_viommu_domain(domain);
+
+ iommu_put_dma_cookie(domain);
+
+ /* Free all remaining mappings (size 2^64) */
+ viommu_del_mappings(vdomain, 0, 0);
+
+ if (vdomain->viommu)
+ ida_simple_remove(&vdomain->viommu->domain_ids, vdomain->id);
+
+ kfree(vdomain);
+}
+
+static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev)
+{
+ int i;
+ int ret = 0;
+ struct virtio_iommu_req_attach req;
+ struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+ struct viommu_endpoint *vdev = fwspec->iommu_priv;
+ struct viommu_domain *vdomain = to_viommu_domain(domain);
+
+ mutex_lock(&vdomain->mutex);
+ if (!vdomain->viommu) {
+ /*
+ * Initialize the domain proper now that we know which viommu
+ * owns it.
+ */
+ ret = viommu_domain_finalise(vdev->viommu, domain);
+ } else if (vdomain->viommu != vdev->viommu) {
+ dev_err(dev, "cannot attach to foreign vIOMMU\n");
+ ret = -EXDEV;
+ }
+ mutex_unlock(&vdomain->mutex);
+
+ if (ret)
+ return ret;
+
+ /*
+ * In the virtio-iommu device, when attaching the endpoint to a new
+ * domain, it is detached from the old one and, if as as a result the
+ * old domain isn't attached to any endpoint, all mappings are removed
+ * from the old domain and it is freed.
+ *
+ * In the driver the old domain still exists, and its mappings will be
+ * recreated if it gets reattached to an endpoint. Otherwise it will be
+ * freed explicitly.
+ *
+ * vdev->vdomain is protected by group->mutex
+ */
+ if (vdev->vdomain)
+ vdev->vdomain->nr_endpoints--;
+
+ req = (struct virtio_iommu_req_attach) {
+ .head.type = VIRTIO_IOMMU_T_ATTACH,
+ .domain = cpu_to_le32(vdomain->id),
+ };
+
+ for (i = 0; i < fwspec->num_ids; i++) {
+ req.endpoint = cpu_to_le32(fwspec->ids[i]);
+
+ ret = viommu_send_req_sync(vdomain->viommu, &req, sizeof(req));
+ if (ret)
+ return ret;
+ }
+
+ if (!vdomain->nr_endpoints) {
+ /*
+ * This endpoint is the first to be attached to the domain.
+ * Replay existing mappings (e.g. SW MSI).
+ */
+ ret = viommu_replay_mappings(vdomain);
+ if (ret)
+ return ret;
+ }
+
+ vdomain->nr_endpoints++;
+ vdev->vdomain = vdomain;
+
+ return 0;
+}
+
+static int viommu_map(struct iommu_domain *domain, unsigned long iova,
+ phys_addr_t paddr, size_t size, int prot)
+{
+ int ret;
+ int flags;
+ struct viommu_mapping *mapping;
+ struct virtio_iommu_req_map map;
+ struct viommu_domain *vdomain = to_viommu_domain(domain);
+
+ flags = (prot & IOMMU_READ ? VIRTIO_IOMMU_MAP_F_READ : 0) |
+ (prot & IOMMU_WRITE ? VIRTIO_IOMMU_MAP_F_WRITE : 0) |
+ (prot & IOMMU_MMIO ? VIRTIO_IOMMU_MAP_F_MMIO : 0);
+
+ mapping = viommu_add_mapping(vdomain, iova, paddr, size, flags);
+ if (!mapping)
+ return -ENOMEM;
+
+ map = (struct virtio_iommu_req_map) {
+ .head.type = VIRTIO_IOMMU_T_MAP,
+ .domain = cpu_to_le32(vdomain->id),
+ .virt_start = cpu_to_le64(iova),
+ .phys_start = cpu_to_le64(paddr),
+ .virt_end = cpu_to_le64(iova + size - 1),
+ .flags = cpu_to_le32(flags),
+ };
+
+ if (!vdomain->nr_endpoints)
+ return 0;
+
+ ret = viommu_send_req_sync(vdomain->viommu, &map, sizeof(map));
+ if (ret)
+ viommu_del_mappings(vdomain, iova, size);
+
+ return ret;
+}
+
+static size_t viommu_unmap(struct iommu_domain *domain, unsigned long iova,
+ size_t size)
+{
+ int ret = 0;
+ size_t unmapped;
+ struct virtio_iommu_req_unmap unmap;
+ struct viommu_domain *vdomain = to_viommu_domain(domain);
+
+ unmapped = viommu_del_mappings(vdomain, iova, size);
+ if (unmapped < size)
+ return 0;
+
+ /* Device already removed all mappings after detach. */
+ if (!vdomain->nr_endpoints)
+ return unmapped;
+
+ unmap = (struct virtio_iommu_req_unmap) {
+ .head.type = VIRTIO_IOMMU_T_UNMAP,
+ .domain = cpu_to_le32(vdomain->id),
+ .virt_start = cpu_to_le64(iova),
+ .virt_end = cpu_to_le64(iova + unmapped - 1),
+ };
+
+ ret = viommu_add_req(vdomain->viommu, &unmap, sizeof(unmap));
+ return ret ? 0 : unmapped;
+}
+
+static phys_addr_t viommu_iova_to_phys(struct iommu_domain *domain,
+ dma_addr_t iova)
+{
+ u64 paddr = 0;
+ unsigned long flags;
+ struct viommu_mapping *mapping;
+ struct interval_tree_node *node;
+ struct viommu_domain *vdomain = to_viommu_domain(domain);
+
+ spin_lock_irqsave(&vdomain->mappings_lock, flags);
+ node = interval_tree_iter_first(&vdomain->mappings, iova, iova);
+ if (node) {
+ mapping = container_of(node, struct viommu_mapping, iova);
+ paddr = mapping->paddr + (iova - mapping->iova.start);
+ }
+ spin_unlock_irqrestore(&vdomain->mappings_lock, flags);
+
+ return paddr;
+}
+
+static void viommu_iotlb_sync(struct iommu_domain *domain)
+{
+ struct viommu_domain *vdomain = to_viommu_domain(domain);
+
+ viommu_sync_req(vdomain->viommu);
+}
+
+static void viommu_get_resv_regions(struct device *dev, struct list_head *head)
+{
+ struct iommu_resv_region *region;
+ int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+
+ region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH, prot,
+ IOMMU_RESV_SW_MSI);
+ if (!region)
+ return;
+
+ list_add_tail(®ion->list, head);
+ iommu_dma_get_resv_regions(dev, head);
+}
+
+static void viommu_put_resv_regions(struct device *dev, struct list_head *head)
+{
+ struct iommu_resv_region *entry, *next;
+
+ list_for_each_entry_safe(entry, next, head, list)
+ kfree(entry);
+}
+
+static struct iommu_ops viommu_ops;
+static struct virtio_driver virtio_iommu_drv;
+
+static int viommu_match_node(struct device *dev, void *data)
+{
+ return dev->parent->fwnode == data;
+}
+
+static struct viommu_dev *viommu_get_by_fwnode(struct fwnode_handle *fwnode)
+{
+ struct device *dev = driver_find_device(&virtio_iommu_drv.driver, NULL,
+ fwnode, viommu_match_node);
+ put_device(dev);
+
+ return dev ? dev_to_virtio(dev)->priv : NULL;
+}
+
+static int viommu_add_device(struct device *dev)
+{
+ int ret;
+ struct iommu_group *group;
+ struct viommu_endpoint *vdev;
+ struct viommu_dev *viommu = NULL;
+ struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+
+ if (!fwspec || fwspec->ops != &viommu_ops)
+ return -ENODEV;
+
+ viommu = viommu_get_by_fwnode(fwspec->iommu_fwnode);
+ if (!viommu)
+ return -ENODEV;
+
+ vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
+ if (!vdev)
+ return -ENOMEM;
+
+ vdev->viommu = viommu;
+ fwspec->iommu_priv = vdev;
+
+ ret = iommu_device_link(&viommu->iommu, dev);
+ if (ret)
+ goto err_free_dev;
+
+ /*
+ * Last step creates a default domain and attaches to it. Everything
+ * must be ready.
+ */
+ group = iommu_group_get_for_dev(dev);
+ if (IS_ERR(group)) {
+ ret = PTR_ERR(group);
+ goto err_unlink_dev;
+ }
+
+ iommu_group_put(group);
+
+ return PTR_ERR_OR_ZERO(group);
+
+err_unlink_dev:
+ iommu_device_unlink(&viommu->iommu, dev);
+
+err_free_dev:
+ kfree(vdev);
+
+ return ret;
+}
+
+static void viommu_remove_device(struct device *dev)
+{
+ struct viommu_endpoint *vdev;
+ struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+
+ if (!fwspec || fwspec->ops != &viommu_ops)
+ return;
+
+ vdev = fwspec->iommu_priv;
+
+ iommu_group_remove_device(dev);
+ iommu_device_unlink(&vdev->viommu->iommu, dev);
+ kfree(vdev);
+}
+
+static struct iommu_group *viommu_device_group(struct device *dev)
+{
+ if (dev_is_pci(dev))
+ return pci_device_group(dev);
+ else
+ return generic_device_group(dev);
+}
+
+static int viommu_of_xlate(struct device *dev, struct of_phandle_args *args)
+{
+ return iommu_fwspec_add_ids(dev, args->args, 1);
+}
+
+static struct iommu_ops viommu_ops = {
+ .domain_alloc = viommu_domain_alloc,
+ .domain_free = viommu_domain_free,
+ .attach_dev = viommu_attach_dev,
+ .map = viommu_map,
+ .unmap = viommu_unmap,
+ .map_sg = default_iommu_map_sg,
+ .iova_to_phys = viommu_iova_to_phys,
+ .iotlb_sync = viommu_iotlb_sync,
+ .add_device = viommu_add_device,
+ .remove_device = viommu_remove_device,
+ .device_group = viommu_device_group,
+ .get_resv_regions = viommu_get_resv_regions,
+ .put_resv_regions = viommu_put_resv_regions,
+ .of_xlate = viommu_of_xlate,
+};
+
+static int viommu_init_vqs(struct viommu_dev *viommu)
+{
+ struct virtio_device *vdev = dev_to_virtio(viommu->dev);
+ const char *name = "request";
+ void *ret;
+
+ ret = virtio_find_single_vq(vdev, NULL, name);
+ if (IS_ERR(ret)) {
+ dev_err(viommu->dev, "cannot find VQ\n");
+ return PTR_ERR(ret);
+ }
+
+ viommu->vqs[VIOMMU_REQUEST_VQ] = ret;
+
+ return 0;
+}
+
+static int viommu_probe(struct virtio_device *vdev)
+{
+ struct device *parent_dev = vdev->dev.parent;
+ struct viommu_dev *viommu = NULL;
+ struct device *dev = &vdev->dev;
+ u64 input_start = 0;
+ u64 input_end = -1UL;
+ int ret;
+
+ viommu = devm_kzalloc(dev, sizeof(*viommu), GFP_KERNEL);
+ if (!viommu)
+ return -ENOMEM;
+
+ spin_lock_init(&viommu->request_lock);
+ ida_init(&viommu->domain_ids);
+ viommu->dev = dev;
+ viommu->vdev = vdev;
+ INIT_LIST_HEAD(&viommu->requests);
+
+ ret = viommu_init_vqs(viommu);
+ if (ret)
+ return ret;
+
+ virtio_cread(vdev, struct virtio_iommu_config, page_size_mask,
+ &viommu->pgsize_bitmap);
+
+ if (!viommu->pgsize_bitmap) {
+ ret = -EINVAL;
+ goto err_free_vqs;
+ }
+
+ viommu->domain_bits = 32;
+
+ /* Optional features */
+ virtio_cread_feature(vdev, VIRTIO_IOMMU_F_INPUT_RANGE,
+ struct virtio_iommu_config, input_range.start,
+ &input_start);
+
+ virtio_cread_feature(vdev, VIRTIO_IOMMU_F_INPUT_RANGE,
+ struct virtio_iommu_config, input_range.end,
+ &input_end);
+
+ virtio_cread_feature(vdev, VIRTIO_IOMMU_F_DOMAIN_BITS,
+ struct virtio_iommu_config, domain_bits,
+ &viommu->domain_bits);
+
+ viommu->geometry = (struct iommu_domain_geometry) {
+ .aperture_start = input_start,
+ .aperture_end = input_end,
+ .force_aperture = true,
+ };
+
+ viommu_ops.pgsize_bitmap = viommu->pgsize_bitmap;
+
+ virtio_device_ready(vdev);
+
+ ret = iommu_device_sysfs_add(&viommu->iommu, dev, NULL, "%s",
+ virtio_bus_name(vdev));
+ if (ret)
+ goto err_free_vqs;
+
+ iommu_device_set_ops(&viommu->iommu, &viommu_ops);
+ iommu_device_set_fwnode(&viommu->iommu, parent_dev->fwnode);
+
+ iommu_device_register(&viommu->iommu);
+
+#ifdef CONFIG_PCI
+ if (pci_bus_type.iommu_ops != &viommu_ops) {
+ pci_request_acs();
+ ret = bus_set_iommu(&pci_bus_type, &viommu_ops);
+ if (ret)
+ goto err_unregister;
+ }
+#endif
+#ifdef CONFIG_ARM_AMBA
+ if (amba_bustype.iommu_ops != &viommu_ops) {
+ ret = bus_set_iommu(&amba_bustype, &viommu_ops);
+ if (ret)
+ goto err_unregister;
+ }
+#endif
+ if (platform_bus_type.iommu_ops != &viommu_ops) {
+ ret = bus_set_iommu(&platform_bus_type, &viommu_ops);
+ if (ret)
+ goto err_unregister;
+ }
+
+ vdev->priv = viommu;
+
+ dev_info(dev, "input address: %u bits\n",
+ order_base_2(viommu->geometry.aperture_end));
+ dev_info(dev, "page mask: %#llx\n", viommu->pgsize_bitmap);
+
+ return 0;
+
+err_unregister:
+ iommu_device_sysfs_remove(&viommu->iommu);
+ iommu_device_unregister(&viommu->iommu);
+err_free_vqs:
+ vdev->config->del_vqs(vdev);
+
+ return ret;
+}
+
+static void viommu_remove(struct virtio_device *vdev)
+{
+ struct viommu_dev *viommu = vdev->priv;
+
+ iommu_device_sysfs_remove(&viommu->iommu);
+ iommu_device_unregister(&viommu->iommu);
+
+ /* Stop all virtqueues */
+ vdev->config->reset(vdev);
+ vdev->config->del_vqs(vdev);
+
+ dev_info(&vdev->dev, "device removed\n");
+}
+
+static void viommu_config_changed(struct virtio_device *vdev)
+{
+ dev_warn(&vdev->dev, "config changed\n");
+}
+
+static unsigned int features[] = {
+ VIRTIO_IOMMU_F_MAP_UNMAP,
+ VIRTIO_IOMMU_F_DOMAIN_BITS,
+ VIRTIO_IOMMU_F_INPUT_RANGE,
+};
+
+static struct virtio_device_id id_table[] = {
+ { VIRTIO_ID_IOMMU, VIRTIO_DEV_ANY_ID },
+ { 0 },
+};
+
+static struct virtio_driver virtio_iommu_drv = {
+ .driver.name = KBUILD_MODNAME,
+ .driver.owner = THIS_MODULE,
+ .id_table = id_table,
+ .feature_table = features,
+ .feature_table_size = ARRAY_SIZE(features),
+ .probe = viommu_probe,
+ .remove = viommu_remove,
+ .config_changed = viommu_config_changed,
+};
+
+module_virtio_driver(virtio_iommu_drv);
+
+IOMMU_OF_DECLARE(viommu, "virtio,mmio");
+
+MODULE_DESCRIPTION("Virtio IOMMU driver");
+MODULE_AUTHOR("Jean-Philippe Brucker <jean-philippe.brucker@arm.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
index 6d5c3b2d4f4d..cfe47c5d9a56 100644
--- a/include/uapi/linux/virtio_ids.h
+++ b/include/uapi/linux/virtio_ids.h
@@ -43,5 +43,6 @@
#define VIRTIO_ID_INPUT 18 /* virtio input */
#define VIRTIO_ID_VSOCK 19 /* virtio vsock transport */
#define VIRTIO_ID_CRYPTO 20 /* virtio crypto */
+#define VIRTIO_ID_IOMMU 23 /* virtio IOMMU */
#endif /* _LINUX_VIRTIO_IDS_H */
diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
new file mode 100644
index 000000000000..f4fafa0b41a7
--- /dev/null
+++ b/include/uapi/linux/virtio_iommu.h
@@ -0,0 +1,117 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Virtio-iommu definition v0.7
+ *
+ * Copyright (C) 2018 Arm Ltd.
+ */
+#ifndef _UAPI_LINUX_VIRTIO_IOMMU_H
+#define _UAPI_LINUX_VIRTIO_IOMMU_H
+
+#include <linux/types.h>
+
+/* Feature bits */
+#define VIRTIO_IOMMU_F_INPUT_RANGE 0
+#define VIRTIO_IOMMU_F_DOMAIN_BITS 1
+#define VIRTIO_IOMMU_F_MAP_UNMAP 2
+#define VIRTIO_IOMMU_F_BYPASS 3
+
+struct virtio_iommu_config {
+ /* Supported page sizes */
+ __u64 page_size_mask;
+ /* Supported IOVA range */
+ struct virtio_iommu_range {
+ __u64 start;
+ __u64 end;
+ } input_range;
+ /* Max domain ID size */
+ __u8 domain_bits;
+} __packed;
+
+/* Request types */
+#define VIRTIO_IOMMU_T_ATTACH 0x01
+#define VIRTIO_IOMMU_T_DETACH 0x02
+#define VIRTIO_IOMMU_T_MAP 0x03
+#define VIRTIO_IOMMU_T_UNMAP 0x04
+
+/* Status types */
+#define VIRTIO_IOMMU_S_OK 0x00
+#define VIRTIO_IOMMU_S_IOERR 0x01
+#define VIRTIO_IOMMU_S_UNSUPP 0x02
+#define VIRTIO_IOMMU_S_DEVERR 0x03
+#define VIRTIO_IOMMU_S_INVAL 0x04
+#define VIRTIO_IOMMU_S_RANGE 0x05
+#define VIRTIO_IOMMU_S_NOENT 0x06
+#define VIRTIO_IOMMU_S_FAULT 0x07
+
+struct virtio_iommu_req_head {
+ __u8 type;
+ __u8 reserved[3];
+} __packed;
+
+struct virtio_iommu_req_tail {
+ __u8 status;
+ __u8 reserved[3];
+} __packed;
+
+struct virtio_iommu_req_attach {
+ struct virtio_iommu_req_head head;
+
+ __le32 domain;
+ __le32 endpoint;
+ __le32 reserved;
+
+ struct virtio_iommu_req_tail tail;
+} __packed;
+
+struct virtio_iommu_req_detach {
+ struct virtio_iommu_req_head head;
+
+ __le32 endpoint;
+ __le32 reserved;
+
+ struct virtio_iommu_req_tail tail;
+} __packed;
+
+#define VIRTIO_IOMMU_MAP_F_READ (1 << 0)
+#define VIRTIO_IOMMU_MAP_F_WRITE (1 << 1)
+#define VIRTIO_IOMMU_MAP_F_EXEC (1 << 2)
+#define VIRTIO_IOMMU_MAP_F_MMIO (1 << 3)
+
+#define VIRTIO_IOMMU_MAP_F_MASK (VIRTIO_IOMMU_MAP_F_READ | \
+ VIRTIO_IOMMU_MAP_F_WRITE | \
+ VIRTIO_IOMMU_MAP_F_EXEC | \
+ VIRTIO_IOMMU_MAP_F_MMIO)
+
+struct virtio_iommu_req_map {
+ struct virtio_iommu_req_head head;
+
+ __le32 domain;
+ __le64 virt_start;
+ __le64 virt_end;
+ __le64 phys_start;
+ __le32 flags;
+
+ struct virtio_iommu_req_tail tail;
+} __packed;
+
+struct virtio_iommu_req_unmap {
+ struct virtio_iommu_req_head head;
+
+ __le32 domain;
+ __le64 virt_start;
+ __le64 virt_end;
+ __le32 reserved;
+
+ struct virtio_iommu_req_tail tail;
+} __packed;
+
+union virtio_iommu_req {
+ struct virtio_iommu_req_head head;
+
+ struct virtio_iommu_req_attach attach;
+ struct virtio_iommu_req_detach detach;
+ struct virtio_iommu_req_map map;
+ struct virtio_iommu_req_unmap unmap;
+};
+
+#endif
--
2.17.0
^ permalink raw reply related
* [PATCH v2 1/5] dt-bindings: virtio: Specify #iommu-cells value for a virtio-iommu
From: Jean-Philippe Brucker @ 2018-06-21 19:06 UTC (permalink / raw)
To: iommu, kvm, virtualization, virtio-dev, devicetree, kvmarm
Cc: mark.rutland, jayachandran.nair, lorenzo.pieralisi, tnowicki, mst,
marc.zyngier, joro, will.deacon, eric.auger, robh+dt,
robin.murphy
In-Reply-To: <20180621190655.56391-1-jean-philippe.brucker@arm.com>
A virtio-mmio node may represent a virtio-iommu device. This is discovered
by the virtio driver at probe time, but the DMA topology isn't
discoverable and must be described by firmware. For DT the standard IOMMU
description is used, as specified in bindings/iommu/iommu.txt and
bindings/pci/pci-iommu.txt. Like many other IOMMUs, virtio-iommu
distinguishes masters by their endpoint IDs, which requires one IOMMU cell
in the "iommus" property.
Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
---
Documentation/devicetree/bindings/virtio/mmio.txt | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/virtio/mmio.txt b/Documentation/devicetree/bindings/virtio/mmio.txt
index 5069c1b8e193..337da0e3a87f 100644
--- a/Documentation/devicetree/bindings/virtio/mmio.txt
+++ b/Documentation/devicetree/bindings/virtio/mmio.txt
@@ -8,6 +8,14 @@ Required properties:
- reg: control registers base address and size including configuration space
- interrupts: interrupt generated by the device
+Required properties for virtio-iommu:
+
+- #iommu-cells: When the node describes a virtio-iommu device, it is
+ linked to DMA masters using the "iommus" property as
+ described in devicetree/bindings/iommu/iommu.txt. For
+ virtio-iommu #iommu-cells must be 1, each cell describing
+ a single endpoint ID.
+
Example:
virtio_block@3000 {
--
2.17.0
^ permalink raw reply related
* [PATCH v2 0/5] Add virtio-iommu driver
From: Jean-Philippe Brucker @ 2018-06-21 19:06 UTC (permalink / raw)
To: iommu, kvm, virtualization, virtio-dev, devicetree, kvmarm
Cc: mark.rutland, jayachandran.nair, lorenzo.pieralisi, tnowicki, mst,
marc.zyngier, joro, will.deacon, eric.auger, robh+dt,
robin.murphy
Implement the base virtio-iommu driver, following version 0.7 of the
specification [1].
Changes since last version [2]:
* Address comments, thanks again for the review.
* As suggested, add a DT binding description in patch 1.
* Depend on VIRTIO_MMIO=y to fix a build failure¹
* Switch to v0.7 of the spec, which changes resv_mem parameters and adds
an MMIO flag. These are trivial but not backward compatible. Once
device or driver is upstream, updates to the spec will rely on feature
bits to stay compatible with this code.
* Implement the new tlb_sync interface, by splitting add_req() and
sync_req(). I noticed a small improvement on netperf stream because
the synchronous iommu_unmap() also benefits from this. Other
experiments, such as using kmem_cache for requests instead of kmalloc,
didn't show any improvement.
Driver is available on branch virtio-iommu/v0.7 [3], and the x86+ACPI
prototype is on branch virtio-iommu/devel. That x86 code hasn't changed,
it still requires the DMA ifdeffery and I lack the expertise to tidy it
up. The kvmtool example device has been cleaned up and is available on
branch virtio-iommu/v0.7 [4].
Feedback welcome!
Thanks,
Jean
[1] virtio-iommu specification v0.7
https://www.spinics.net/lists/linux-virtualization/msg34127.html
[2] virtio-iommu driver v1
https://www.spinics.net/lists/kvm/msg164322.html
[3] git://linux-arm.org/linux-jpb.git virtio-iommu/v0.7
http://www.linux-arm.org/git?p=linux-jpb.git;a=shortlog;h=refs/heads/virtio-iommu/v0.7
[4] git://linux-arm.org/kvmtool-jpb.git virtio-iommu/v0.7
---
¹ A word on the module story. Because of complex dependencies IOMMU
drivers cannot yet be .ko modules. Enabling it is outside the scope of
this series but I have a small prototype on branch virtio-iommu/
module-devel. It seems desirable since some distros currently ship the
transport code as module and are unlikely to change this on our account.
Note that this series works fine with arm64 defconfig, which selects
VIRTIO_MMIO=y.
I could use some help to clean this up. Currently my solution is to split
virtio-iommu into a module and a 3-lines built-in stub, which isn't
graceful but could have been worse.
Keeping the whole virtio-iommu driver as builtin would require accessing
any virtio utility through get_symbol. So far we only have seven of
those and could keep a list of pointer ops, but I find this solution
terrible. If virtio or virtio_mmio is a module, virtio-iommu also needs
to be one.
The minimal set of changes to make a module out of an OF-based IOMMU
driver seems to be:
* Export IOMMU symbols used by drivers
* Don't give up deferring probe in of_iommu
* Move IOMMU OF tables to .rodata
* Create a static virtio-iommu stub that declares the virtio-iommu OF
table entry. The build system doesn't pick up IOMMU_OF_DECLARE when
done from an object destined to be built as module :(
* Create a device_link between endpoint and IOMMU, to ensure that
removing the IOMMU driver also unbinds the endpoint driver. Putting
this in IOMMU core seems like a better approach since even when not
built as module, unbinding an IOMMU device via sysfs will cause
crashes.
With this, as long as virtio-mmio isn't loaded, the OF code defers probe
of endpoints that depend on virtio-iommu. Once virtio-mmio is loaded,
the probe is still deferred until virtio-iommu registers itself to the
virtio bus. Once virtio-iommu is loaded, probe of endpoints managed by
the IOMMU follows.
I'll investigate ACPI IORT when I find some time, though I don't expect
much complication and suggest we tackle one problem at a time. Since it
is a luxury that requires changes to the IOMMU core, module support is
left as a future improvement.
---
Jean-Philippe Brucker (5):
dt-bindings: virtio: Specify #iommu-cells value for a virtio-iommu
iommu: Add virtio-iommu driver
iommu/virtio: Add probe request
iommu/virtio: Add event queue
vfio: Allow type-1 IOMMU instantiation for ARM
.../devicetree/bindings/virtio/mmio.txt | 8 +
MAINTAINERS | 6 +
drivers/iommu/Kconfig | 11 +
drivers/iommu/Makefile | 1 +
drivers/iommu/virtio-iommu.c | 1164 +++++++++++++++++
drivers/vfio/Kconfig | 2 +-
include/uapi/linux/virtio_ids.h | 1 +
include/uapi/linux/virtio_iommu.h | 172 +++
8 files changed, 1364 insertions(+), 1 deletion(-)
create mode 100644 drivers/iommu/virtio-iommu.c
create mode 100644 include/uapi/linux/virtio_iommu.h
--
2.17.0
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [virtio-dev] Re: [Qemu-devel] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Michael S. Tsirkin @ 2018-06-21 18:20 UTC (permalink / raw)
To: Cornelia Huck
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, konrad.wilk,
Jakub Kicinski, Samudrala, Sridhar, qemu-devel, virtualization,
Siwei Liu, Venu Busireddy, Netdev, boris.ostrovsky, aaron.f.brown,
Joao Martins
In-Reply-To: <20180621165913.7e3f4faa.cohuck@redhat.com>
On Thu, Jun 21, 2018 at 04:59:13PM +0200, Cornelia Huck wrote:
> On Wed, 20 Jun 2018 22:48:58 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Wed, Jun 20, 2018 at 06:06:19PM +0200, Cornelia Huck wrote:
> > > In any case, I'm not sure anymore why we'd want the extra uuid.
> >
> > It's mostly so we can have e.g. multiple devices with same MAC
> > (which some people seem to want in order to then use
> > then with different containers).
> >
> > But it is also handy for when you assign a PF, since then you
> > can't set the MAC.
> >
>
> OK, so what about the following:
>
> - introduce a new feature bit, VIRTIO_NET_F_STANDBY_UUID that indicates
> that we have a new uuid field in the virtio-net config space
> - in QEMU, add a property for virtio-net that allows to specify a uuid,
> offer VIRTIO_NET_F_STANDBY_UUID if set
> - when configuring, set the property to the group UUID of the vfio-pci
> device
> - in the guest, use the uuid from the virtio-net device's config space
> if applicable; else, fall back to matching by MAC as done today
>
> That should work for all virtio transports.
True. I'm a bit unhappy that it's virtio net specific though
since down the road I expect we'll have a very similar feature
for scsi (and maybe others).
But we do not have a way to have fields that are portable
both across devices and transports, and I think it would
be a useful addition. How would this work though? Any idea?
--
MST
^ permalink raw reply
* Re: [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Michael S. Tsirkin @ 2018-06-21 18:14 UTC (permalink / raw)
To: Jason Wang
Cc: alexander.h.duyck, virtio-dev, Samudrala, Sridhar, qemu-devel,
virtualization
In-Reply-To: <ce4da2b0-88fd-c79b-0567-fe564bb34922@redhat.com>
On Wed, Jun 13, 2018 at 01:40:59PM +0800, Jason Wang wrote:
>
>
> On 2018年06月13日 12:24, Samudrala, Sridhar wrote:
> > On 6/12/2018 7:38 PM, Jason Wang wrote:
> > >
> > >
> > > On 2018年06月12日 19:54, Michael S. Tsirkin wrote:
> > > > On Wed, Jun 06, 2018 at 10:29:03AM +0800, Jason Wang wrote:
> > > > >
> > > > > On 2018年06月05日 20:33, Michael S. Tsirkin wrote:
> > > > > > I don't think this is sufficient.
> > > > > >
> > > > > > If both primary and standby devices are present, a
> > > > > > legacy guest without
> > > > > > support for the feature might see two devices with same mac and get
> > > > > > confused.
> > > > > >
> > > > > > I think that we should only make primary visible after
> > > > > > guest acked the
> > > > > > backup feature bit.
> > > > > I think we want exactly the reverse? E.g fail the
> > > > > negotiation when guest
> > > > > does not ack backup feature.
> > > > >
> > > > > Otherwise legacy guest won't even have the chance to see
> > > > > primary device in
> > > > > the guest.
> > > > That's by design.
> > >
> > > So management needs to know the capability of guest to set the
> > > backup feature. This looks a chicken or egg problem to me.
> >
> > I don't think so. If the tenant requests 'accelerated datapath feature',
> > the management
> > will set 'standby' feature bit on virtio-net interface and if the guest
> > virtio-net driver
> > supports this feature, then the tenant VM will get that capability via a
> > hot-plugged
> > primary device.
>
> Ok, I thought exactly the reverse because of the commit title is "enable
> virtio_net to act as a standby for a passthru device". But re-read the
> commit log content, I understand the case a little bit. Btw, VF is not
> necessarily faster than virtio-net, especially consider virtio-net may have
> a lot of queues.
Don't do that then, right?
> >
> >
> > >
> > > >
> > > > > > And on reset or when backup is cleared in some other way, unplug the
> > > > > > primary.
> > > > > What if guest does not support hotplug?
> > > > It shouldn't ack the backup feature then and it's a good point.
> > > > We should both document this and check kernel config has
> > > > hotplug support. Sridhar could you take a look pls?
> > > >
> > > > > > Something like the below will do the job:
> > > > > >
> > > > > > Primary device is added with a special "primary-failover" flag.
> > > > > > A virtual machine is then initialized with just a standby virtio
> > > > > > device. Primary is not yet added.
> > > > > A question is how to do the matching? Qemu knows nothing about e.g mac
> > > > > address of a pass-through device I believe?
> > > > Supply a flag to the VFIO when it's added, this way QEMU will know.
> > > >
> > > > > > Later QEMU detects that guest driver device set DRIVER_OK.
> > > > > > It then exposes the primary device to the guest, and triggers
> > > > > > a device addition event (hot-plug event) for it.
> > > > > Do you mean we won't have primary device in the initial qemu cli?
> > > > No, that's not what I mean.
> > > >
> > > > I mean management will supply a flag to VFIO and then
> > > >
> > > >
> > > > - VFIO defers exposing
> > > > primary to guest until guest acks the feature bit.
> > > > - When we see guest ack, initiate hotplug.
> > > > - On reboot, hide it again.
> > > > - On reset without reboot, request hot-unplug and on eject hide
> > > > it again.
> > >
> > > This sounds much like a kind of bonding in qemu.
> > >
> > > >
> > > > > > If QEMU detects guest driver removal, it initiates a
> > > > > > hot-unplug sequence
> > > > > > to remove the primary driver. In particular, if QEMU detects guest
> > > > > > re-initialization (e.g. by detecting guest reset) it
> > > > > > immediately removes
> > > > > > the primary device.
> > > > > I believe guest failover module should handle this gracefully?
> > > > It can't control enumeration order, if primary is enumerated before
> > > > standby then guest will load its driver and it's too late
> > > > when failover driver is loaded.
> > >
> > > Well, even if we can delay the visibility of primary until
> > > DRIVER_OK, there still be a race I think? And it looks to me it's
> > > still a bug of guest:
> > >
> > > E.g primary could be probed before failover_register() in guest.
> > > Then we will miss the enslaving of primary forever.
> >
> > That is not an issue. Even if the primary is probed before failover
> > driver, it will
> > enslave the primary via the call to failover_existing_slave_register()
> > as part of
> > failover_register() routine.
>
> Aha I get it. So the enumeration order is not an issue.
>
> Consider primary may still be seen by guest kernel even if we delay its
> visibility, I wonder whether we can control the lifecycle of primary through
> driver but not qemu. This can simplify a lot of things.
>
> Thanks
>
> >
> > >
> > > Thanks
> > >
> > > >
> > > > > Thanks
> > > > >
> > > > > > We can move some of this code to management as well,
> > > > > > architecturally it
> > > > > > does not make too much sense but it might be easier
> > > > > > implementation-wise.
> > > > > >
> > > > > > HTH
> > > > > >
> > > > > > On Mon, Jun 04, 2018 at 06:41:48PM -0700, Samudrala, Sridhar wrote:
> > > > > > > Ping on this patch now that the kernel patches are
> > > > > > > accepted into davem's net-next tree.
> > > > > > > https://patchwork.ozlabs.org/cover/920005/
> > > > > > >
> > > > > > >
> > > > > > > On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:
> > > > > > > > This feature bit can be used by hypervisor to
> > > > > > > > indicate virtio_net device to
> > > > > > > > act as a standby for another device with the same MAC address.
> > > > > > > >
> > > > > > > > I tested this with a small change to the patch
> > > > > > > > to mark the STANDBY feature 'true'
> > > > > > > > by default as i am using libvirt to start the VMs.
> > > > > > > > Is there a way to pass the newly added feature
> > > > > > > > bit 'standby' to qemu via libvirt
> > > > > > > > XML file?
> > > > > > > >
> > > > > > > > Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> > > > > > > > ---
> > > > > > > > hw/net/virtio-net.c | 2 ++
> > > > > > > > include/standard-headers/linux/virtio_net.h | 3 +++
> > > > > > > > 2 files changed, 5 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > > > > > > index 90502fca7c..38b3140670 100644
> > > > > > > > --- a/hw/net/virtio-net.c
> > > > > > > > +++ b/hw/net/virtio-net.c
> > > > > > > > @@ -2198,6 +2198,8 @@ static Property virtio_net_properties[] = {
> > > > > > > > true),
> > > > > > > > DEFINE_PROP_INT32("speed", VirtIONet,
> > > > > > > > net_conf.speed, SPEED_UNKNOWN),
> > > > > > > > DEFINE_PROP_STRING("duplex", VirtIONet,
> > > > > > > > net_conf.duplex_str),
> > > > > > > > + DEFINE_PROP_BIT64("standby", VirtIONet,
> > > > > > > > host_features, VIRTIO_NET_F_STANDBY,
> > > > > > > > + false),
> > > > > > > > DEFINE_PROP_END_OF_LIST(),
> > > > > > > > };
> > > > > > > > diff --git
> > > > > > > > a/include/standard-headers/linux/virtio_net.h
> > > > > > > > b/include/standard-headers/linux/virtio_net.h
> > > > > > > > index e9f255ea3f..01ec09684c 100644
> > > > > > > > --- a/include/standard-headers/linux/virtio_net.h
> > > > > > > > +++ b/include/standard-headers/linux/virtio_net.h
> > > > > > > > @@ -57,6 +57,9 @@
> > > > > > > > * Steering */
> > > > > > > > #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
> > > > > > > > +#define VIRTIO_NET_F_STANDBY 62 /* Act
> > > > > > > > as standby for another device
> > > > > > > > + * with the same MAC.
> > > > > > > > + */
> > > > > > > > #define VIRTIO_NET_F_SPEED_DUPLEX 63 /*
> > > > > > > > Device set linkspeed and duplex */
> > > > > > > > #ifndef VIRTIO_NET_NO_LEGACY
> > >
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [virtio-dev] Re: [Qemu-devel] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Cornelia Huck @ 2018-06-21 14:59 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, konrad.wilk,
Jakub Kicinski, Samudrala, Sridhar, qemu-devel, virtualization,
Siwei Liu, Venu Busireddy, Netdev, boris.ostrovsky, aaron.f.brown,
Joao Martins
In-Reply-To: <20180620224535-mutt-send-email-mst@kernel.org>
On Wed, 20 Jun 2018 22:48:58 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Jun 20, 2018 at 06:06:19PM +0200, Cornelia Huck wrote:
> > In any case, I'm not sure anymore why we'd want the extra uuid.
>
> It's mostly so we can have e.g. multiple devices with same MAC
> (which some people seem to want in order to then use
> then with different containers).
>
> But it is also handy for when you assign a PF, since then you
> can't set the MAC.
>
OK, so what about the following:
- introduce a new feature bit, VIRTIO_NET_F_STANDBY_UUID that indicates
that we have a new uuid field in the virtio-net config space
- in QEMU, add a property for virtio-net that allows to specify a uuid,
offer VIRTIO_NET_F_STANDBY_UUID if set
- when configuring, set the property to the group UUID of the vfio-pci
device
- in the guest, use the uuid from the virtio-net device's config space
if applicable; else, fall back to matching by MAC as done today
That should work for all virtio transports.
^ permalink raw reply
* Re: [PATCH v5 2/3] x86/asm: add _ASM_ARG* constants for argument registers to <asm/asm.h>
From: Ingo Molnar @ 2018-06-21 12:28 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Kate Stewart, andrea.parri, linux-efi, brijesh.singh, J. Kiszka,
Josh Poimboeuf, Will Deacon, jarkko.sakkinen, virtualization,
Masahiro Yamada, Manoj Gupta, hpa, boris.ostrovsky,
Thiebaud Weksteen, mawilcox, x86, akataria, Greg Hackmann, mingo,
Alistair Strachan, David Rientjes, geert, thomas.lendacky,
Arnd Bergmann, Linux Kbuild mailing list, Philippe Ombredanne,
rostedt
In-Reply-To: <CAKwvOdmE-EHJY7tgzkhyQqNT3m3oWtd6iE3sWBTz19Rijzx4Ew@mail.gmail.com>
* Nick Desaulniers <ndesaulniers@google.com> wrote:
> On Thu, Jun 14, 2018 at 5:17 PM H. Peter Anvin <hpa@zytor.com> wrote:
> >
> > On 06/14/18 13:59, Nick Desaulniers wrote:
> > > On Thu, Jun 14, 2018 at 1:48 PM H. Peter Anvin <hpa@zytor.com> wrote:
> > >>
> > >> On 06/13/18 14:05, Nick Desaulniers wrote:
> > >>> From: "H. Peter Anvin" <hpa@linux.intel.com>
> > >>>
> > >>> i386 and x86-64 uses different registers for arguments; make them
> > >>> available so we don't have to #ifdef in the actual code.
> > >>>
> > >>> Native size and specified size (q, l, w, b) versions are provided.
> > >>>
> > >>> Suggested-by: Sedat Dilek <sedat.dilek@gmail.com>
> > >>> Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
> > >>> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > >>
> > >> I still object to Suggested-by: here. Sedat did a correction, which is
> > >> a Reviewed-by:, but unless I'm completely out to sea, there was no
> > >> suggestion on Sedat's part of this; and I had certainly not seen it when
> > >> I wrote the code.
> > >
> > > I'm fine with changing it from a Suggested-by to a Reviewed-by. Can
> > > you do that when you apply the set, or should I send a v6?
> > >
> >
> > I'm not handling patch mechanics for x86 at the moment.
>
> Hi Ingo and Thomas,
> Have you had a chance to review this patch series for application?
>
> Note that Juergen (the paravirt maintainer) acked the series:
> https://bugs.llvm.org/show_bug.cgi?id=37880
> The only issue being swapping a Suggested-by for a Reviewed-by tag for
> one commit, as above.
Could you please update the series with all suggestions and tags and send a new
series?
Thanks,
Ingo
^ permalink raw reply
* Re: [PATCH] net: vhost: improve performance when enable busyloop
From: Jason Wang @ 2018-06-21 5:59 UTC (permalink / raw)
To: Tonghao Zhang; +Cc: netdev, Tonghao Zhang, virtualization
In-Reply-To: <1529501332-118823-1-git-send-email-xiangxia.m.yue@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2916 bytes --]
On 2018年06月20日 21:28, Tonghao Zhang wrote:
> This patch improves the guest receive performance from
> host. On the handle_tx side, we poll the sock receive
> queue at the same time. handle_rx do that in the same way.
>
> we set the poll-us=100 us and use the iperf3 to test
> its throughput. The iperf3 command is shown as below.
>
> iperf3 -s -D
> iperf3 -c 192.168.1.100 -i 1 -P 10 -t 10 -M 1400 --bandwidth 100000M
>
> * With the patch: 21.1 Gbits/sec
> * Without the patch: 12.7 Gbits/sec
Thanks a lot for the patch. But looks like it needs some work to avoid
e.g deadlock.
E.g in vhost_process_iotlb_msg() we call vhost_dev_lock_vqs() which did:
for (i = 0; i < d->nvqs; ++i)
mutex_lock_nested(&d->vqs[i]->mutex, i);
I believe we need to change the code to lock the vq one by one like the
attached (only compile test).
> Signed-off-by: Tonghao Zhang <zhangtonghao@didichuxing.com>
> ---
> drivers/vhost/net.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index e7cf7d2..9364ede 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -429,22 +429,43 @@ static int vhost_net_enable_vq(struct vhost_net *n,
> return vhost_poll_start(poll, sock->file);
> }
>
> +static int sk_has_rx_data(struct sock *sk);
> +
How about move sk_has_rx_data() here.
> static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
> struct vhost_virtqueue *vq,
> struct iovec iov[], unsigned int iov_size,
> unsigned int *out_num, unsigned int *in_num)
> {
> unsigned long uninitialized_var(endtime);
> + struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_RX];
> + struct vhost_virtqueue *rvq = &nvq->vq;
> + struct socket *sock = rvq->private_data;
> +
> int r = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
> out_num, in_num, NULL, NULL);
>
> if (r == vq->num && vq->busyloop_timeout) {
> + mutex_lock_nested(&rvq->mutex, 1);
> +
> + vhost_disable_notify(&net->dev, rvq);
> +
> preempt_disable();
> endtime = busy_clock() + vq->busyloop_timeout;
> while (vhost_can_busy_poll(vq->dev, endtime) &&
> + !(sock && sk_has_rx_data(sock->sk)) &&
> vhost_vq_avail_empty(vq->dev, vq))
> cpu_relax();
> preempt_enable();
> +
> + if (sock && sk_has_rx_data(sock->sk))
> + vhost_poll_queue(&rvq->poll);
> + else if (unlikely(vhost_enable_notify(&net->dev, rvq))) {
> + vhost_disable_notify(&net->dev, rvq);
> + vhost_poll_queue(&rvq->poll);
> + }
> +
> + mutex_unlock(&rvq->mutex);
Some kinds of code duplication, can we try to unify them?
Btw, net-next is closed, so you need resubmit after it was open and use
a "net-next" as the prefix of the patch.
Thanks
> +
> r = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
> out_num, in_num, NULL, NULL);
> }
[-- Attachment #2: 0001-vhost-lock-vqs-one-by-one.patch --]
[-- Type: text/x-patch, Size: 2178 bytes --]
From 383fe9d98420d92a632dc554969b4b1716017ba2 Mon Sep 17 00:00:00 2001
From: Jason Wang <jasowang@redhat.com>
Date: Thu, 21 Jun 2018 13:58:31 +0800
Subject: [PATCH] vhost: lock vqs one by one
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 23 ++++++-----------------
1 file changed, 6 insertions(+), 17 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index e5bc4bb..937252d 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -294,8 +294,11 @@ static void vhost_vq_meta_reset(struct vhost_dev *d)
{
int i;
- for (i = 0; i < d->nvqs; ++i)
+ for (i = 0; i < d->nvqs; ++i) {
+ mutex_lock(&d->vqs[i]->mutex);
__vhost_vq_meta_reset(d->vqs[i]);
+ mutex_unlock(&d->vqs[i]->mutex);
+ }
}
static void vhost_vq_reset(struct vhost_dev *dev,
@@ -855,20 +858,6 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
#define vhost_get_used(vq, x, ptr) \
vhost_get_user(vq, x, ptr, VHOST_ADDR_USED)
-static void vhost_dev_lock_vqs(struct vhost_dev *d)
-{
- int i = 0;
- for (i = 0; i < d->nvqs; ++i)
- mutex_lock_nested(&d->vqs[i]->mutex, i);
-}
-
-static void vhost_dev_unlock_vqs(struct vhost_dev *d)
-{
- int i = 0;
- for (i = 0; i < d->nvqs; ++i)
- mutex_unlock(&d->vqs[i]->mutex);
-}
-
static int vhost_new_umem_range(struct vhost_umem *umem,
u64 start, u64 size, u64 end,
u64 userspace_addr, int perm)
@@ -918,7 +907,9 @@ static void vhost_iotlb_notify_vq(struct vhost_dev *d,
if (msg->iova <= vq_msg->iova &&
msg->iova + msg->size - 1 > vq_msg->iova &&
vq_msg->type == VHOST_IOTLB_MISS) {
+ mutex_lock(&node->vq->mutex);
vhost_poll_queue(&node->vq->poll);
+ mutex_unlock(&node->vq->mutex);
list_del(&node->node);
kfree(node);
}
@@ -950,7 +941,6 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
int ret = 0;
mutex_lock(&dev->mutex);
- vhost_dev_lock_vqs(dev);
switch (msg->type) {
case VHOST_IOTLB_UPDATE:
if (!dev->iotlb) {
@@ -984,7 +974,6 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
break;
}
- vhost_dev_unlock_vqs(dev);
mutex_unlock(&dev->mutex);
return ret;
--
2.7.4
[-- Attachment #3: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* [PATCH net] vhost_net: validate sock before trying to put its fd
From: Jason Wang @ 2018-06-21 5:11 UTC (permalink / raw)
To: mst, kvm, virtualization, netdev, linux-kernel; +Cc: dan.carpenter
Sock will be NULL if we pass -1 to vhost_net_set_backend(), but when
we meet errors during ubuf allocation, the code does not check for
NULL before calling sockfd_put(), this will lead NULL
dereferencing. Fixing by checking sock pointer before.
Fixes: bab632d69ee4 ("vhost: vhost TX zero-copy support")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 986058a..b97a994 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -1208,7 +1208,8 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
if (ubufs)
vhost_net_ubuf_put_wait_and_free(ubufs);
err_ubufs:
- sockfd_put(sock);
+ if (sock)
+ sockfd_put(sock);
err_vq:
mutex_unlock(&vq->mutex);
err:
--
2.7.4
^ permalink raw reply related
* Re: [virtio-dev] Re: [Qemu-devel] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Siwei Liu @ 2018-06-20 19:59 UTC (permalink / raw)
To: Cornelia Huck
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, Michael S. Tsirkin,
Jakub Kicinski, Samudrala, Sridhar, konrad.wilk, qemu-devel,
virtualization, Venu Busireddy, Netdev, boris.ostrovsky,
aaron.f.brown, Joao Martins
In-Reply-To: <20180620163408.737af8c2.cohuck@redhat.com>
On Wed, Jun 20, 2018 at 7:34 AM, Cornelia Huck <cohuck@redhat.com> wrote:
> On Tue, 19 Jun 2018 13:09:14 -0700
> Siwei Liu <loseweigh@gmail.com> wrote:
>
>> On Tue, Jun 19, 2018 at 3:54 AM, Cornelia Huck <cohuck@redhat.com> wrote:
>> > On Fri, 15 Jun 2018 10:06:07 -0700
>> > Siwei Liu <loseweigh@gmail.com> wrote:
>> >
>> >> On Fri, Jun 15, 2018 at 4:48 AM, Cornelia Huck <cohuck@redhat.com> wrote:
>> >> > On Thu, 14 Jun 2018 18:57:11 -0700
>> >> > Siwei Liu <loseweigh@gmail.com> wrote:
>
>> >> > I'm a bit confused here. What, exactly, ties the two devices together?
>> >>
>> >> The group UUID. Since QEMU VFIO dvice does not have insight of MAC
>> >> address (which it doesn't have to), the association between VFIO
>> >> passthrough and standby must be specificed for QEMU to understand the
>> >> relationship with this model. Note, standby feature is no longer
>> >> required to be exposed under this model.
>> >
>> > Isn't that a bit limiting, though?
>> >
>> > With this model, you can probably tie a vfio-pci device and a
>> > virtio-net-pci device together. But this will fail if you have
>> > different transports: Consider tying together a vfio-pci device and a
>> > virtio-net-ccw device on s390, for example. The standby feature bit is
>> > on the virtio-net level and should not have any dependency on the
>> > transport used.
>>
>> Probably we'd limit the support for grouping to virtio-net-pci device
>> and vfio-pci device only. For virtio-net-pci, as you might see with
>> Venu's patch, we store the group UUID on the config space of
>> virtio-pci, which is only applicable to PCI transport.
>>
>> If virtio-net-ccw needs to support the same, I think similar grouping
>> interface should be defined on the VirtIO CCW transport. I think the
>> current implementation of the Linux failover driver assumes that it's
>> SR-IOV VF with same MAC address which the virtio-net-pci needs to pair
>> with, and that the PV path is on same PF without needing to update
>> network of the port-MAC association change. If we need to extend the
>> grouping mechanism to virtio-net-ccw, it has to pass such failover
>> mode to virtio driver specifically through some other option I guess.
>
> Hm, I've just spent some time reading the Linux failover code and I did
> not really find much pci-related magic in there (other than checking
> for a pci device in net_failover_slave_pre_register). We also seem to
> look for a matching device by MAC only. What magic am I missing?
The existing assumptions around SR-IOV VF and thus PCI is implicit. A
lot of simplications are built on the fact that the passthrough device
is a SR-IOV Virtual Function specifically than others: MAC addresses
for couple devices must be the same, changing MAC address is
prohibited, programming VLAN filter is challenged, the datapath of
virtio-net has to share the same physical function where VF belongs
to. There's no hankshake during datapath switching at all to support a
normal passthrough device at this point. I'd imagine some work around
that ahead, which might be a bit involved than just to support a
simplified model for VF migration.
>
> Is the look-for-uuid handling supposed to happen in the host only?
The look-for-MAC matching scheme is not ideal in many aspects. I don't
want to repeat those again, but once the group UUID is added to QEMU,
the failover driver is supposed to switch to the UUID based matching
scheme in the guest.
>
>> >> > If libvirt already has the knowledge that it should manage the two as a
>> >> > couple, why do we need the group id (or something else for other
>> >> > architectures)? (Maybe I'm simply missing something because I'm not
>> >> > that familiar with pci.)
>> >>
>> >> The idea is to have QEMU control the visibility and enumeration order
>> >> of the passthrough VFIO for the failover scenario. Hotplug can be one
>> >> way to achieve it, and perhaps there's other way around also. The
>> >> group ID is not just for QEMU to couple devices, it's also helpful to
>> >> guest too as grouping using MAC address is just not safe.
>> >
>> > Sorry about dragging mainframes into this, but this will only work for
>> > homogenous device coupling, not for heterogenous. Consider my vfio-pci
>> > + virtio-net-ccw example again: The guest cannot find out that the two
>> > belong together by checking some group ID, it has to either use the MAC
>> > or some needs-to-be-architectured property.
>> >
>> > Alternatively, we could propose that mechanism as pci-only, which means
>> > we can rely on mechanisms that won't necessarily work on non-pci
>> > transports. (FWIW, I don't see a use case for using vfio-ccw to pass
>> > through a network card anytime in the near future, due to the nature of
>> > network cards currently in use on s390.)
>>
>> Yes, let's do this just for PCI transport (homogenous) for now.
>
> But why? Using pci for passthrough to make things easier (and because
> there's not really a use case), sure. But I really don't want to
> restrict this to virtio-pci only.
Of course, technically it doesn't have to be virtio-pci only. The
group UUID can even extend it further to non-pci transport. However,
with the current focus of the driver support on SR-IOV VF and limited
use case on non-pci, I'd feel no immediate effort will be needed on
that front.
>
>> >> In the model of (b), I think it essentially turns hotplug to one of
>> >> mechanisms for QEMU to control the visibility. The libvirt can still
>> >> manage the hotplug of individual devices during live migration or in
>> >> normal situation to hot add/remove devices. Though the visibility of
>> >> the VFIO is under the controll of QEMU, and it's possible that the hot
>> >> add/remove request does not involve actual hot plug activity in guest
>> >> at all.
>> >
>> > That depends on how you model visibility, I guess. You'll probably want
>> > to stop traffic flowing through one or the other of the cards; would
>> > link down or similar be enough for the virtio device?
>>
>> I'm not sure if it is a good idea. The guest user will see two devices
>> with same MAC but one of them is down. Do you expect user to use it or
>> not? And since the guest is going to be migrated, we need to unplug a
>> broken VF from guest before migrating, why do we bother plugging in
>> this useless VF at the first place?
>
> I was thinking about using hotunplugging only over migration and doing
> the link up only after feature negotiation has finished, but that is
> probably too complicated. Let's stick to hotplug for simplicity's sake.
OK. Thanks for the discussion, it's really useful.
Regards,
-Siwei
^ permalink raw reply
* Re: [virtio-dev] Re: [Qemu-devel] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Michael S. Tsirkin @ 2018-06-20 19:48 UTC (permalink / raw)
To: Cornelia Huck
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, konrad.wilk,
Jakub Kicinski, Samudrala, Sridhar, qemu-devel, virtualization,
Siwei Liu, Venu Busireddy, Netdev, boris.ostrovsky, aaron.f.brown,
Joao Martins
In-Reply-To: <20180620180619.6b4ee52d.cohuck@redhat.com>
On Wed, Jun 20, 2018 at 06:06:19PM +0200, Cornelia Huck wrote:
> In any case, I'm not sure anymore why we'd want the extra uuid.
It's mostly so we can have e.g. multiple devices with same MAC
(which some people seem to want in order to then use
then with different containers).
But it is also handy for when you assign a PF, since then you
can't set the MAC.
--
MST
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox