Re: [Linux] [ARM] Granting memory obtained from the DMA API

Re: [Linux] [ARM] Granting memory obtained from the DMA API

[Julien Grall]

On 19/08/2020 12:04, Simon Leiner wrote:
> Hi everyone,

Hi Simon,

> 
> I'm working on a virtio driver for the Linux kernel that supports the
> dynamic connection of devices via the Xenbus (as part of a research
> project at the Karlsruhe Institute of Technology).

There is a lot of interest to get Virtio working on Xen at the moment. 
Is this going to be a new transport layer for Virtio?

> My question concerns the Xenbus client API in the Linux kernel. As this
> is my first time posting on the Xen mailing lists, I'm not entirely
> sure if this is the right place for this question. If not, feel free to
> point me to the right place :-)

Xen-devel is probably most suitable for this discussion, so I moved the 
discussion there. I have also CCed a couple of Linux maintainers that 
should be able to provide feedbacks on the approaches.

> 
> Part of virtio is having shared memory. So naturally, I'm using Xen's
> grant system for that. Part of the Xenbus client API is the function
> xenbus_grant_ring which, by its documentation grants access to a block
> of memory starting at vaddr to another domain. I tried using this in my
> driver which created the grants and returned without any error, but
> after mounting the grants on another domain, it turns out that some
> other location in memory was actually granted instead of the one behind
> the original vaddr.
> 
> So I found the problem: The vaddr that I was using xenbus_grant_ring
> with was obtained by dma_alloc_coherent (whereas the other split
> drivers included in the mainline kernel use Xen IO rings allocated by
> the "regular" mechanisms such as __get_free_page, alloc_page etc.).
> But xenbus_grant_ring uses virt_to_gfn to get the GFN for the vaddr
> which on ARM(64) must not be used for DMA addresses. So I could fix the
> problem by providing a modified version of xenbus_grant_ring as part of
> my driver which takes a dma_addr_t instead of a void* for the start
> address, gets the PFN via dma_to_phys, converts it to a GFN and then
> delegates to gnttab_grant_foreign_access, just like xenbus_grant_ring.
> I can confirm that this works on Linux 5.4.0.
> 
> My question to you is: How can this be fixed "the right way"?
> Is there anything that can be done to prevent others from debugging
> the same problem (which for me, took some hours...)?
> 
> I can see multiple approaches:
> 1. Have xenbus_grant_ring "just work" even with DMA addresses on ARM
>     This would certainly be the nicest solution, but I don't see how
>     it could be implemented. I don't know how to check whether some
>     address actually is a DMA address and even if there was a way to
>     know, dma_to_phys still requires a pointer to the device struct
>     which was used for allocation.
> 2. Provide another version which takes a dma_addr_t instead of void*
>     This can be easily done, but things get complicated when the device
>     for which the DMA memory was allocated is not the xenbus_device
>     which is passed anyway. So, it would be necessary to include an
>     additional argument pointing the actual device struct which was used
>     for allocation.
> 3. Just use gnttab_grant_foreign_access which works with GFNs anyway
>     Which is essentially what I'm doing currently, as in my driver I
>     know from which the device the DMA addresses were allocated.
>     If this is the preferred solution to this problem, I propose adding
>     a warning to the documentation of xenbus_grant_ring that forbids
>     using this for vaddrs obtained from the DMA API as it will not work
>     (at least on ARM).
> 
> What do you think?
> 
> Greetings from Germany,
> Simon

Best regards,

-- 
Julien Grall

Re: [Linux] [ARM] Granting memory obtained from the DMA API

[Simon Leiner]

Hi Julien,

On 20.08.20 13:17, Julien Grall wrote:
> There is a lot of interest to get Virtio working on Xen at the moment.
> Is this going to be a new transport layer for Virtio?


It is designed that way, yes. The current implementation (based on
virtio_mmio.c) has a few limitations:
 - Only the host side is implemented for Linux (We are currently only
using bare metal domains for the device side - so the device
implementation is based on OpenAMP[1])

- It lacks some features, e.g. there is currently no device
configuration space

- It is tested only very narrowly (only for my use case which is RPMsg
via the rpmsg_char kernel driver)

As this was really just a byproduct of my main research topic, I'm
currently not in touch with the virtio standards committee. But I'm
happy to contribute my work if there is interest :-)

> Xen-devel is probably most suitable for this discussion, so I moved the
> discussion there. I have also CCed a couple of Linux maintainers that
> should be able to provide feedbacks on the approaches.

Thanks!

Greetings,
Simon


[1]: https://www.openampproject.org

Re: [Linux] [ARM] Granting memory obtained from the DMA API

[Stefano Stabellini]

On Thu, 20 Aug 2020, Julien Grall wrote:
> > Part of virtio is having shared memory. So naturally, I'm using Xen's
> > grant system for that. Part of the Xenbus client API is the function
> > xenbus_grant_ring which, by its documentation grants access to a block
> > of memory starting at vaddr to another domain. I tried using this in my
> > driver which created the grants and returned without any error, but
> > after mounting the grants on another domain, it turns out that some
> > other location in memory was actually granted instead of the one behind
> > the original vaddr.
> > 
> > So I found the problem: The vaddr that I was using xenbus_grant_ring
> > with was obtained by dma_alloc_coherent (whereas the other split
> > drivers included in the mainline kernel use Xen IO rings allocated by
> > the "regular" mechanisms such as __get_free_page, alloc_page etc.).
> > But xenbus_grant_ring uses virt_to_gfn to get the GFN for the vaddr
> > which on ARM(64) must not be used for DMA addresses. So I could fix the
> > problem by providing a modified version of xenbus_grant_ring as part of
> > my driver which takes a dma_addr_t instead of a void* for the start
> > address, gets the PFN via dma_to_phys, converts it to a GFN and then
> > delegates to gnttab_grant_foreign_access, just like xenbus_grant_ring.
> > I can confirm that this works on Linux 5.4.0.
>
> > My question to you is: How can this be fixed "the right way"?
> > Is there anything that can be done to prevent others from debugging
> > the same problem (which for me, took some hours...)?
> > 
> > I can see multiple approaches:
> > 1. Have xenbus_grant_ring "just work" even with DMA addresses on ARM
> >     This would certainly be the nicest solution, but I don't see how
> >     it could be implemented. I don't know how to check whether some
> >     address actually is a DMA address and even if there was a way to
> >     know, dma_to_phys still requires a pointer to the device struct
> >     which was used for allocation.
> > 2. Provide another version which takes a dma_addr_t instead of void*
> >     This can be easily done, but things get complicated when the device
> >     for which the DMA memory was allocated is not the xenbus_device
> >     which is passed anyway. So, it would be necessary to include an
> >     additional argument pointing the actual device struct which was used
> >     for allocation.
> > 3. Just use gnttab_grant_foreign_access which works with GFNs anyway
> >     Which is essentially what I'm doing currently, as in my driver I
> >     know from which the device the DMA addresses were allocated.
> >     If this is the preferred solution to this problem, I propose adding
> >     a warning to the documentation of xenbus_grant_ring that forbids
> >     using this for vaddrs obtained from the DMA API as it will not work
> >     (at least on ARM).
> > 
> > What do you think?

Thank for the well-written analysis of the problem. The following should
work to translate the virtual address properly in xenbus_grant_ring:

	if (is_vmalloc_addr(vaddr))
		page = vmalloc_to_page(vaddr);
	else
		page = virt_to_page(vaddr);

Please give it a try and let me know. Otherwise, if it cannot be made to
work, option 3 with a proper warning is also fine.

Re: [Linux] [ARM] Granting memory obtained from the DMA API

[Simon Leiner]

On 20.08.20 20:35, Stefano Stabellini wrote:
> Thank for the well-written analysis of the problem. The following 
should
> work to translate the virtual address properly in xenbus_grant_ring:
> 
> 	if (is_vmalloc_addr(vaddr))
> 		page = vmalloc_to_page(vaddr);
> 	else
> 		page = virt_to_page(vaddr);

Great idea, thanks! I modified it lightly (see below) and it did indeed 
work! I'm wondering though whether the check for vmalloc'd addresses 
should be included directly in the ARM implementation of virt_to_gfn. 
As far as I see, this should not break anything, but might impose a 
small performance overhead in cases where it is known for sure that we 
are dealing with directly mapped memory. What do you think?

diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index e17ca8156171..d7a97f946f2f 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -344,6 +344,21 @@ static void xenbus_switch_fatal(struct xenbus_device *dev, int depth, int err,
        __xenbus_switch_state(dev, XenbusStateClosing, 1);
 }
 
+/**
+ * vaddr_to_gfn
+ * @vaddr: any virtual address
+ * 
+ * Returns the guest frame number (GFN) corresponding to vaddr.
+ */
+static inline unsigned long vaddr_to_gfn(void *vaddr)
+{
+   if (is_vmalloc_addr(vaddr)) {
+       return pfn_to_gfn(vmalloc_to_pfn(vaddr));
+   } else {
+       return virt_to_gfn(vaddr);
+   }
+}
+
 /**
  * xenbus_grant_ring
  * @dev: xenbus device
@@ -364,7 +379,7 @@ int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
 
    for (i = 0; i < nr_pages; i++) {
        err = gnttab_grant_foreign_access(dev->otherend_id,
-                         virt_to_gfn(vaddr), 0);
+                         vaddr_to_gfn(vaddr), 0);
        if (err < 0) {
            xenbus_dev_fatal(dev, err,
                     "granting access to ring page");

Re: [Linux] [ARM] Granting memory obtained from the DMA API

[Stefano Stabellini]

On Fri, 21 Aug 2020, Simon Leiner wrote:
> On 20.08.20 20:35, Stefano Stabellini wrote:
> > Thank for the well-written analysis of the problem. The following 
> should
> > work to translate the virtual address properly in xenbus_grant_ring:
> > 
> > 	if (is_vmalloc_addr(vaddr))
> > 		page = vmalloc_to_page(vaddr);
> > 	else
> > 		page = virt_to_page(vaddr);
> 
> Great idea, thanks! I modified it lightly (see below) and it did indeed 
> work! I'm wondering though whether the check for vmalloc'd addresses 
> should be included directly in the ARM implementation of virt_to_gfn. 
> As far as I see, this should not break anything, but might impose a 
> small performance overhead in cases where it is known for sure that we 
> are dealing with directly mapped memory. What do you think?

Thanks for testing!

We could ask the relevant maintainers for feedback, but I think it is
probably intended that virt_to_gfn doesn't work on vmalloc addresses.
That's because vmalloc addresses are not typically supposed to be used
like that.



> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
> index e17ca8156171..d7a97f946f2f 100644
> --- a/drivers/xen/xenbus/xenbus_client.c
> +++ b/drivers/xen/xenbus/xenbus_client.c
> @@ -344,6 +344,21 @@ static void xenbus_switch_fatal(struct xenbus_device *dev, int depth, int err,
>         __xenbus_switch_state(dev, XenbusStateClosing, 1);
>  }
>  
> +/**
> + * vaddr_to_gfn
> + * @vaddr: any virtual address
> + * 
> + * Returns the guest frame number (GFN) corresponding to vaddr.
> + */
> +static inline unsigned long vaddr_to_gfn(void *vaddr)
> +{
> +   if (is_vmalloc_addr(vaddr)) {
> +       return pfn_to_gfn(vmalloc_to_pfn(vaddr));
> +   } else {
> +       return virt_to_gfn(vaddr);
> +   }
> +}
> +

For the same reason as above, I would rather have the check inside
xenbus_grant_ring, rather than above in a generic function:

- if this is a special case the check should be inside xenbus_grant_ring
- if this is not a special case, then the fix should be in virt_to_gfn
  as you mentioned

either way, I wouldn't introduce this function here

Juergen, do you agree with this?


>  /**
>   * xenbus_grant_ring
>   * @dev: xenbus device
> @@ -364,7 +379,7 @@ int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
>  
>     for (i = 0; i < nr_pages; i++) {
>         err = gnttab_grant_foreign_access(dev->otherend_id,
> -                         virt_to_gfn(vaddr), 0);
> +                         vaddr_to_gfn(vaddr), 0);
>         if (err < 0) {
>             xenbus_dev_fatal(dev, err,
>                      "granting access to ring page");

Re: [Linux] [ARM] Granting memory obtained from the DMA API

[Jürgen Groß]

On 24.08.20 22:02, Stefano Stabellini wrote:
> On Fri, 21 Aug 2020, Simon Leiner wrote:
>> On 20.08.20 20:35, Stefano Stabellini wrote:
>>> Thank for the well-written analysis of the problem. The following
>> should
>>> work to translate the virtual address properly in xenbus_grant_ring:
>>>
>>> 	if (is_vmalloc_addr(vaddr))
>>> 		page = vmalloc_to_page(vaddr);
>>> 	else
>>> 		page = virt_to_page(vaddr);
>>
>> Great idea, thanks! I modified it lightly (see below) and it did indeed
>> work! I'm wondering though whether the check for vmalloc'd addresses
>> should be included directly in the ARM implementation of virt_to_gfn.
>> As far as I see, this should not break anything, but might impose a
>> small performance overhead in cases where it is known for sure that we
>> are dealing with directly mapped memory. What do you think?
> 
> Thanks for testing!
> 
> We could ask the relevant maintainers for feedback, but I think it is
> probably intended that virt_to_gfn doesn't work on vmalloc addresses.
> That's because vmalloc addresses are not typically supposed to be used
> like that.
> 
> 
> 
>> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
>> index e17ca8156171..d7a97f946f2f 100644
>> --- a/drivers/xen/xenbus/xenbus_client.c
>> +++ b/drivers/xen/xenbus/xenbus_client.c
>> @@ -344,6 +344,21 @@ static void xenbus_switch_fatal(struct xenbus_device *dev, int depth, int err,
>>          __xenbus_switch_state(dev, XenbusStateClosing, 1);
>>   }
>>   
>> +/**
>> + * vaddr_to_gfn
>> + * @vaddr: any virtual address
>> + *
>> + * Returns the guest frame number (GFN) corresponding to vaddr.
>> + */
>> +static inline unsigned long vaddr_to_gfn(void *vaddr)
>> +{
>> +   if (is_vmalloc_addr(vaddr)) {
>> +       return pfn_to_gfn(vmalloc_to_pfn(vaddr));
>> +   } else {
>> +       return virt_to_gfn(vaddr);
>> +   }
>> +}
>> +
> 
> For the same reason as above, I would rather have the check inside
> xenbus_grant_ring, rather than above in a generic function:
> 
> - if this is a special case the check should be inside xenbus_grant_ring
> - if this is not a special case, then the fix should be in virt_to_gfn
>    as you mentioned
> 
> either way, I wouldn't introduce this function here
> 
> Juergen, do you agree with this?

Basically, yes. Lets do it in xenbus_grant_ring() plus adding a

WARN_ON_ONCE(is_vmalloc_addr(vaddr), ...)

in virt_to_gfn() for being able to catch other special cases.


Juergen

Virtio Xen (WAS: Re: [Linux] [ARM] Granting memory obtained from the DMA API)

[Julien Grall]

On 20/08/2020 12:57, Simon Leiner wrote:
> Hi Julien,

Hi Simon,

> 
> On 20.08.20 13:17, Julien Grall wrote:
>> There is a lot of interest to get Virtio working on Xen at the moment.
>> Is this going to be a new transport layer for Virtio?
> 
> 
> It is designed that way, yes. The current implementation (based on
> virtio_mmio.c) has a few limitations:
>   - Only the host side is implemented for Linux (We are currently only
> using bare metal domains for the device side - so the device
> implementation is based on OpenAMP[1])
> 
> - It lacks some features, e.g. there is currently no device
> configuration space
> 
> - It is tested only very narrowly (only for my use case which is RPMsg
> via the rpmsg_char kernel driver)
> 
> As this was really just a byproduct of my main research topic, I'm
> currently not in touch with the virtio standards committee. But I'm
> happy to contribute my work if there is interest :-)

I have CCed a few folks that are interested in virtio. May I ask why did 
you create a new transport rather than using the existing one?

So far, there is an RFC to implement virtio-mmio for Xen on Arm (see 
[2]). But the idea of a Xen specific transport is discussed on the 
mailing list time to time. It would be more secure than existing 
transport, but I am worried about the adoption of the transport.

A new transport requires to modify all the OSes so they can work on Xen. 
This is fine for open-source OS, but it may be more difficult to get 
proprietary OS to invest in the new transport.

Do see any use of this transport outside of Xen?

Cheers,

[2] 
https://lore.kernel.org/xen-devel/1596478888-23030-1-git-send-email-olekstysh@gmail.com/

-- 
Julien Grall

Re: Virtio Xen (WAS: Re: [Linux] [ARM] Granting memory obtained from the DMA API)

[Simon Leiner]

Hi Julien,

On 25.08.20 15:02, Julien Grall wrote:
> May I ask why did you create a new transport rather than using the
> existing one?

We wanted a mechanism for dynamically creating virtio devices at 
runtime. I looked at virtio-mmio briefly and it seemed to me that a lot 
of things would have to be known in advance (how many devices are 
there? How much memory do they need? Where does the memory range for 
virtio-mmio start on the device domain?). So after reading a bit about 
Xen and how the classic split drivers work, I figured building a split 
driver for virtio was the way to go. The basic principle is really 
simple:

 - Using grants to share memory for the virtqueues
 - Using event channels as a queue notification mechanism
 - All state handling and other information exchange (like number of 
   queues, grant refs, event channel numbers etc.) is done through the 
   Xenbus.

On the Linux side, this is implemented as a kernel module. No patches 
to the kernel itself (apart from the ones I sent in earlier) or to Xen 
itself are required.

> So far, there is an RFC to implement virtio-mmio for Xen on Arm

I did not see that before. Also, I'm not familiar with the ioreq 
mechanism. But from skimming the patch, it seems like it achieves the 
same thing (dynamic creation of virtio devices at runtime). Is that 
right?

> But the idea of a Xen specific transport is discussed on the mailing
> list time to time. It would be more secure than existing transport,
> but I am worried about the adoption of the transport. 

What are the security issues with the existing transport mechanisms?
I'm quite new to the Xen community, so I have no idea about adoption 
rates.

> A new transport requires to modify all the OSes so they can work on 
> Xen.

Just to be clear: They would need to be modified in order to support 
that mechanism, but it changes nothing about the interface between 
hypervisor and guest.

However, supporting the same use case with an already existing 
transport mechanism is more elegant than building another transport 
mechanism specifically for that case IMO. The only technical difference 
between my implementation and the virtio-mmio approach in actually 
running the virtio device is that I'm using event channels for queue 
notification while virtio-mmio uses some bytes in memory for that. I do 
not have any measurements indicating whether or not this makes a 
difference.

> Do see any use of this transport outside of Xen? 

This mechanism relies on the Xenbus, so no.

Greetings,
Simon

Re: Virtio Xen (WAS: Re: [Linux] [ARM] Granting memory obtained from the DMA API)

[Julien Grall]

On 29/08/2020 11:38, Simon Leiner wrote:
> Hi Julien,

Hi Simon,

Apologies for the late answer.

> On 25.08.20 15:02, Julien Grall wrote:
>> May I ask why did you create a new transport rather than using the
>> existing one?
> 
> We wanted a mechanism for dynamically creating virtio devices at
> runtime. I looked at virtio-mmio briefly and it seemed to me that a lot
> of things would have to be known in advance (how many devices are
> there? How much memory do they need? Where does the memory range for
> virtio-mmio start on the device domain?). So after reading a bit about
> Xen and how the classic split drivers work, I figured building a split
> driver for virtio was the way to go. The basic principle is really
> simple:
> 
>   - Using grants to share memory for the virtqueues
>   - Using event channels as a queue notification mechanism
>   - All state handling and other information exchange (like number of
>     queues, grant refs, event channel numbers etc.) is done through the
>     Xenbus.
> 
> On the Linux side, this is implemented as a kernel module. No patches
> to the kernel itself (apart from the ones I sent in earlier) or to Xen
> itself are required.
> 
>> So far, there is an RFC to implement virtio-mmio for Xen on Arm
> 
> I did not see that before. Also, I'm not familiar with the ioreq
> mechanism. But from skimming the patch, it seems like it achieves the
> same thing (dynamic creation of virtio devices at runtime). Is that
> right?

I am not aware of any mechanism with virtio-mmio to notify a new device 
after the OS booted. But virtio-pci should allow you to do as as this is 
just a PCI device.

However, you will still need to size the PCI hostbridge I/O region 
correctly when the domain creation. Not sure if this would be an issue 
in your use case.

> 
>> But the idea of a Xen specific transport is discussed on the mailing
>> list time to time. It would be more secure than existing transport,
>> but I am worried about the adoption of the transport.
> 
> What are the security issues with the existing transport mechanisms?
In the Xen PV model, a backend cannot access the frontend memory unless 
the latter did explictly grant.

In the default virtio-{mmio, pci} model, a backend must have full access 
to the frontend memory. This can be an issue if you don't trust your 
backend or it can get compromised.

> I'm quite new to the Xen community, so I have no idea about adoption
> rates.
> 
>> A new transport requires to modify all the OSes so they can work on
>> Xen.
> 
> Just to be clear: They would need to be modified in order to support
> that mechanism, but it changes nothing about the interface between
> hypervisor and guest.

Right, this is the first bit I am more concerned about. IMHO, the main 
goal of virtio is to allow moving from one hypervisor to another without 
(or possibly limited) changing the guest OS code.

Adding a new transport open source OS for a new transport is fairly 
easy, but it may be harder to justify for proprietary OS if it only 
benefits Xen.

That said, if we can convince other hypervisor vendors to adopt it then 
most of my concern are moot :).

> 
> However, supporting the same use case with an already existing
> transport mechanism is more elegant than building another transport
> mechanism specifically for that case IMO. The only technical difference
> between my implementation and the virtio-mmio approach in actually
> running the virtio device is that I'm using event channels for queue
> notification while virtio-mmio uses some bytes in memory for that. I do
> not have any measurements indicating whether or not this makes a
> difference.

The virtio-mmio notification is potentially going to be expensive on Xen 
because the guest because a vCPU will be blocked until the I/O has been 
handled by the IOREQ server.

The notification would look like:
     1) Frontend write in notification address
     2) Trap in Xen
     3) Pause the vCPU and forward the I/O to the IOREQ server (e.g. 
your virtio backend)
     4) Schedule the domain where the IOREQ server resides
     5) Wait for the I/O completion
     6) Unpause the vCPU
     7) Return to guest

We may be able to optimize as there is no need to wait for the I/O to 
complete when we notify.

Cheers,

-- 
Julien Grall

RE: Virtio Xen (WAS: Re: [Linux] [ARM] Granting memory obtained from the DMA API)

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Julien Grall
> Sent: 23 September 2020 10:04
> To: Simon Leiner <simon@leiner.me>
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>; Bertrand Marquis
> <Bertrand.Marquis@arm.com>; Oleksandr Tyshchenko <olekstysh@gmail.com>
> Subject: Re: Virtio Xen (WAS: Re: [Linux] [ARM] Granting memory obtained from the DMA API)
> 
> On 29/08/2020 11:38, Simon Leiner wrote:
> > Hi Julien,
> 
> Hi Simon,
> 
> Apologies for the late answer.
> 
> > On 25.08.20 15:02, Julien Grall wrote:
> >> May I ask why did you create a new transport rather than using the
> >> existing one?
> >
> > We wanted a mechanism for dynamically creating virtio devices at
> > runtime. I looked at virtio-mmio briefly and it seemed to me that a lot
> > of things would have to be known in advance (how many devices are
> > there? How much memory do they need? Where does the memory range for
> > virtio-mmio start on the device domain?). So after reading a bit about
> > Xen and how the classic split drivers work, I figured building a split
> > driver for virtio was the way to go. The basic principle is really
> > simple:
> >
> >   - Using grants to share memory for the virtqueues
> >   - Using event channels as a queue notification mechanism
> >   - All state handling and other information exchange (like number of
> >     queues, grant refs, event channel numbers etc.) is done through the
> >     Xenbus.
> >
> > On the Linux side, this is implemented as a kernel module. No patches
> > to the kernel itself (apart from the ones I sent in earlier) or to Xen
> > itself are required.
> >
> >> So far, there is an RFC to implement virtio-mmio for Xen on Arm
> >
> > I did not see that before. Also, I'm not familiar with the ioreq
> > mechanism. But from skimming the patch, it seems like it achieves the
> > same thing (dynamic creation of virtio devices at runtime). Is that
> > right?
> 
> I am not aware of any mechanism with virtio-mmio to notify a new device
> after the OS booted. But virtio-pci should allow you to do as as this is
> just a PCI device.
> 
> However, you will still need to size the PCI hostbridge I/O region
> correctly when the domain creation. Not sure if this would be an issue
> in your use case.
> 
> >
> >> But the idea of a Xen specific transport is discussed on the mailing
> >> list time to time. It would be more secure than existing transport,
> >> but I am worried about the adoption of the transport.
> >
> > What are the security issues with the existing transport mechanisms?
> In the Xen PV model, a backend cannot access the frontend memory unless
> the latter did explictly grant.
> 
> In the default virtio-{mmio, pci} model, a backend must have full access
> to the frontend memory. This can be an issue if you don't trust your
> backend or it can get compromised.

Is 'full access' required? The virtio device implementation would essentially be performing DMA so and vIOMMU implementation could restrict memory access on a per-PCI-device basis.

> 
> > I'm quite new to the Xen community, so I have no idea about adoption
> > rates.
> >
> >> A new transport requires to modify all the OSes so they can work on
> >> Xen.
> >
> > Just to be clear: They would need to be modified in order to support
> > that mechanism, but it changes nothing about the interface between
> > hypervisor and guest.
> 
> Right, this is the first bit I am more concerned about. IMHO, the main
> goal of virtio is to allow moving from one hypervisor to another without
> (or possibly limited) changing the guest OS code.
> 
> Adding a new transport open source OS for a new transport is fairly
> easy, but it may be harder to justify for proprietary OS if it only
> benefits Xen.
> 
> That said, if we can convince other hypervisor vendors to adopt it then
> most of my concern are moot :).
> 
> >
> > However, supporting the same use case with an already existing
> > transport mechanism is more elegant than building another transport
> > mechanism specifically for that case IMO. The only technical difference
> > between my implementation and the virtio-mmio approach in actually
> > running the virtio device is that I'm using event channels for queue
> > notification while virtio-mmio uses some bytes in memory for that. I do
> > not have any measurements indicating whether or not this makes a
> > difference.
> 
> The virtio-mmio notification is potentially going to be expensive on Xen
> because the guest because a vCPU will be blocked until the I/O has been
> handled by the IOREQ server.
> 
> The notification would look like:
>      1) Frontend write in notification address
>      2) Trap in Xen
>      3) Pause the vCPU and forward the I/O to the IOREQ server (e.g.
> your virtio backend)
>      4) Schedule the domain where the IOREQ server resides
>      5) Wait for the I/O completion
>      6) Unpause the vCPU
>      7) Return to guest
> 
> We may be able to optimize as there is no need to wait for the I/O to
> complete when we notify.

Perhaps take a look at the 'bufioreq' ring in the implementation?

  Paul

> 
> Cheers,
> 
> --
> Julien Grall

Re: Virtio Xen (WAS: Re: [Linux] [ARM] Granting memory obtained from the DMA API)

[Julien Grall]

On 23/09/2020 11:09, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Julien Grall
>> Sent: 23 September 2020 10:04
>> To: Simon Leiner <simon@leiner.me>
>> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>; Bertrand Marquis
>> <Bertrand.Marquis@arm.com>; Oleksandr Tyshchenko <olekstysh@gmail.com>
>> Subject: Re: Virtio Xen (WAS: Re: [Linux] [ARM] Granting memory obtained from the DMA API)
>>
>> On 29/08/2020 11:38, Simon Leiner wrote:
>>> Hi Julien,
>>
>> Hi Simon,
>>
>> Apologies for the late answer.
>>
>>> On 25.08.20 15:02, Julien Grall wrote:
>>>> May I ask why did you create a new transport rather than using the
>>>> existing one?
>>>
>>> We wanted a mechanism for dynamically creating virtio devices at
>>> runtime. I looked at virtio-mmio briefly and it seemed to me that a lot
>>> of things would have to be known in advance (how many devices are
>>> there? How much memory do they need? Where does the memory range for
>>> virtio-mmio start on the device domain?). So after reading a bit about
>>> Xen and how the classic split drivers work, I figured building a split
>>> driver for virtio was the way to go. The basic principle is really
>>> simple:
>>>
>>>    - Using grants to share memory for the virtqueues
>>>    - Using event channels as a queue notification mechanism
>>>    - All state handling and other information exchange (like number of
>>>      queues, grant refs, event channel numbers etc.) is done through the
>>>      Xenbus.
>>>
>>> On the Linux side, this is implemented as a kernel module. No patches
>>> to the kernel itself (apart from the ones I sent in earlier) or to Xen
>>> itself are required.
>>>
>>>> So far, there is an RFC to implement virtio-mmio for Xen on Arm
>>>
>>> I did not see that before. Also, I'm not familiar with the ioreq
>>> mechanism. But from skimming the patch, it seems like it achieves the
>>> same thing (dynamic creation of virtio devices at runtime). Is that
>>> right?
>>
>> I am not aware of any mechanism with virtio-mmio to notify a new device
>> after the OS booted. But virtio-pci should allow you to do as as this is
>> just a PCI device.
>>
>> However, you will still need to size the PCI hostbridge I/O region
>> correctly when the domain creation. Not sure if this would be an issue
>> in your use case.
>>
>>>
>>>> But the idea of a Xen specific transport is discussed on the mailing
>>>> list time to time. It would be more secure than existing transport,
>>>> but I am worried about the adoption of the transport.
>>>
>>> What are the security issues with the existing transport mechanisms?
>> In the Xen PV model, a backend cannot access the frontend memory unless
>> the latter did explictly grant.
>>
>> In the default virtio-{mmio, pci} model, a backend must have full access
>> to the frontend memory. This can be an issue if you don't trust your
>> backend or it can get compromised.
> 
> Is 'full access' required? The virtio device implementation would essentially be performing DMA so and vIOMMU implementation could restrict memory access on a per-PCI-device basis.

The 'full access' is not necessary, however this is the *default* model 
for now.

I know there is discussion about vIOMMU but I am not aware of any effort 
toward implementing it.

> 
>>
>>> I'm quite new to the Xen community, so I have no idea about adoption
>>> rates.
>>>
>>>> A new transport requires to modify all the OSes so they can work on
>>>> Xen.
>>>
>>> Just to be clear: They would need to be modified in order to support
>>> that mechanism, but it changes nothing about the interface between
>>> hypervisor and guest.
>>
>> Right, this is the first bit I am more concerned about. IMHO, the main
>> goal of virtio is to allow moving from one hypervisor to another without
>> (or possibly limited) changing the guest OS code.
>>
>> Adding a new transport open source OS for a new transport is fairly
>> easy, but it may be harder to justify for proprietary OS if it only
>> benefits Xen.
>>
>> That said, if we can convince other hypervisor vendors to adopt it then
>> most of my concern are moot :).
>>
>>>
>>> However, supporting the same use case with an already existing
>>> transport mechanism is more elegant than building another transport
>>> mechanism specifically for that case IMO. The only technical difference
>>> between my implementation and the virtio-mmio approach in actually
>>> running the virtio device is that I'm using event channels for queue
>>> notification while virtio-mmio uses some bytes in memory for that. I do
>>> not have any measurements indicating whether or not this makes a
>>> difference.
>>
>> The virtio-mmio notification is potentially going to be expensive on Xen
>> because the guest because a vCPU will be blocked until the I/O has been
>> handled by the IOREQ server.
>>
>> The notification would look like:
>>       1) Frontend write in notification address
>>       2) Trap in Xen
>>       3) Pause the vCPU and forward the I/O to the IOREQ server (e.g.
>> your virtio backend)
>>       4) Schedule the domain where the IOREQ server resides
>>       5) Wait for the I/O completion
>>       6) Unpause the vCPU
>>       7) Return to guest
>>
>> We may be able to optimize as there is no need to wait for the I/O to
>> complete when we notify.
> 
> Perhaps take a look at the 'bufioreq' ring in the implementation?

Yes, this is what I have in mind.

Cheers,

-- 
Julien Grall