* Re: [RFC] virtio-mem: paravirtualized memory
From: Michael S. Tsirkin @ 2017-06-16 20:19 UTC (permalink / raw)
To: David Hildenbrand
Cc: Andrea Arcangeli, linux-mm@kvack.org, qemu-devel@nongnu.org, KVM,
virtualization@lists.linux-foundation.org
In-Reply-To: <4cdf547c-079b-6b44-484f-e1132e960364@redhat.com>
On Fri, Jun 16, 2017 at 05:59:07PM +0200, David Hildenbrand wrote:
> On 16.06.2017 17:04, Michael S. Tsirkin wrote:
> > On Fri, Jun 16, 2017 at 04:20:02PM +0200, David Hildenbrand wrote:
> >> Hi,
> >>
> >> this is an idea that is based on Andrea Arcangeli's original idea to
> >> host enforce guest access to memory given up using virtio-balloon using
> >> userfaultfd in the hypervisor. While looking into the details, I
> >> realized that host-enforcing virtio-balloon would result in way too many
> >> problems (mainly backwards compatibility) and would also have some
> >> conceptual restrictions that I want to avoid. So I developed the idea of
> >> virtio-mem - "paravirtualized memory".
> >
> > Thanks! I went over this quickly, will read some more in the
> > coming days. I would like to ask for some clarifications
> > on one part meanwhile:
>
> Thanks for looking into it that fast! :)
>
> In general, what this section is all about: Why to not simply host
> enforce virtio-balloon.
> >
> >> Q: Why not reuse virtio-balloon?
> >>
> >> A: virtio-balloon is for cooperative memory management. It has a fixed
> >> page size
> >
> > We are fixing that with VIRTIO_BALLOON_F_PAGE_CHUNKS btw.
> > I would appreciate you looking into that patchset.
>
> Will do, thanks. Problem is that there is no "enforcement" on the page
> size. VIRTIO_BALLOON_F_PAGE_CHUNKS simply allows to send bigger chunks.
> Nobody hinders the guest (especially legacy virtio-balloon drivers) from
> sending 4k pages.
>
> So this doesn't really fix the issue (we have here), it just allows to
> speed up transfer. Which is a good thing, but does not help for
> enforcement at all. So, yes support for page sizes > 4k, but no way to
> enforce it.
>
> >
> >> and will deflate in certain situations.
> >
> > What does this refer to?
>
> A Linux guest will deflate the balloon (all or some pages) in the
> following scenarios:
> a) page migration
It inflates it first, doesn't it?
> b) unload virtio-balloon kernel module
> c) hibernate/suspension
> d) (DEFLATE_ON_OOM)
You need to set a flag in the balloon to allow this, right?
> A Linux guest will touch memory without deflating:
> a) During a kexec() dump
> d) On reboots (regular, after kexec(), system_reset)
> >
> >> Any change we
> >> introduce will break backwards compatibility.
> >
> > Why does this have to be the case
> If we suddenly enforce the existing virtio-balloon, we will break legacy
> guests.
Can't we do it with a feature flag?
> Simple example:
> Guest with inflated virtio-balloon reboots. Touches inflated memory.
> Gets killed at some random point.
>
> Of course, another discussion would be "can't we move virtio-mem
> functionality into virtio-balloon instead of changing virtio-balloon".
> With the current concept this is also not possible (one region per
> device vs. one virtio-balloon device). And I think while similar, these
> are two different concepts.
>
> >
> >> virtio-balloon was not
> >> designed to give guarantees. Nobody can hinder the guest from
> >> deflating/reusing inflated memory.
> >
> > Reusing without deflate is forbidden with TELL_HOST, right?
>
> TELL_HOST just means "please inform me". There is no way to NACK a
> request. It is not a permission to do so, just a "friendly
> notification". And this is exactly not what we want when host enforcing
> memory access.
>
>
> >
> >> In addition, it might make perfect
> >> sense to have both, virtio-balloon and virtio-mem at the same time,
> >> especially looking at the DEFLATE_ON_OOM or STATS features of
> >> virtio-balloon. While virtio-mem is all about guarantees, virtio-
> >> balloon is about cooperation.
> >
> > Thanks, and I intend to look more into this next week.
> >
>
> I know that it is tempting to force this concept into virtio-balloon. I
> spent quite some time thinking about this (and possible other techniques
> like implicit memory deflation on reboots) and decided not to do it. We
> just end up trying to hack around all possible things that could go
> wrong, while still not being able to handle all requirements properly.
I agree there's a large # of requirements here not addressed by the balloon.
One other thing that would be helpful here is pointing out the
similarities between virtio-mem and the balloon. I'll ponder it
over the weekend.
The biggest worry for me is inability to support DMA into this memory.
Is this hard to fix?
Thanks!
> --
>
> Thanks,
>
> David
^ permalink raw reply
* Re: [RFC] virtio-mem: paravirtualized memory
From: David Hildenbrand @ 2017-06-16 15:59 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Andrea Arcangeli, linux-mm@kvack.org, qemu-devel@nongnu.org, KVM,
virtualization@lists.linux-foundation.org
In-Reply-To: <20170616175748-mutt-send-email-mst@kernel.org>
On 16.06.2017 17:04, Michael S. Tsirkin wrote:
> On Fri, Jun 16, 2017 at 04:20:02PM +0200, David Hildenbrand wrote:
>> Hi,
>>
>> this is an idea that is based on Andrea Arcangeli's original idea to
>> host enforce guest access to memory given up using virtio-balloon using
>> userfaultfd in the hypervisor. While looking into the details, I
>> realized that host-enforcing virtio-balloon would result in way too many
>> problems (mainly backwards compatibility) and would also have some
>> conceptual restrictions that I want to avoid. So I developed the idea of
>> virtio-mem - "paravirtualized memory".
>
> Thanks! I went over this quickly, will read some more in the
> coming days. I would like to ask for some clarifications
> on one part meanwhile:
Thanks for looking into it that fast! :)
In general, what this section is all about: Why to not simply host
enforce virtio-balloon.
>
>> Q: Why not reuse virtio-balloon?
>>
>> A: virtio-balloon is for cooperative memory management. It has a fixed
>> page size
>
> We are fixing that with VIRTIO_BALLOON_F_PAGE_CHUNKS btw.
> I would appreciate you looking into that patchset.
Will do, thanks. Problem is that there is no "enforcement" on the page
size. VIRTIO_BALLOON_F_PAGE_CHUNKS simply allows to send bigger chunks.
Nobody hinders the guest (especially legacy virtio-balloon drivers) from
sending 4k pages.
So this doesn't really fix the issue (we have here), it just allows to
speed up transfer. Which is a good thing, but does not help for
enforcement at all. So, yes support for page sizes > 4k, but no way to
enforce it.
>
>> and will deflate in certain situations.
>
> What does this refer to?
A Linux guest will deflate the balloon (all or some pages) in the
following scenarios:
a) page migration
b) unload virtio-balloon kernel module
c) hibernate/suspension
d) (DEFLATE_ON_OOM)
A Linux guest will touch memory without deflating:
a) During a kexec() dump
d) On reboots (regular, after kexec(), system_reset)
>
>> Any change we
>> introduce will break backwards compatibility.
>
> Why does this have to be the case
If we suddenly enforce the existing virtio-balloon, we will break legacy
guests.
Simple example:
Guest with inflated virtio-balloon reboots. Touches inflated memory.
Gets killed at some random point.
Of course, another discussion would be "can't we move virtio-mem
functionality into virtio-balloon instead of changing virtio-balloon".
With the current concept this is also not possible (one region per
device vs. one virtio-balloon device). And I think while similar, these
are two different concepts.
>
>> virtio-balloon was not
>> designed to give guarantees. Nobody can hinder the guest from
>> deflating/reusing inflated memory.
>
> Reusing without deflate is forbidden with TELL_HOST, right?
TELL_HOST just means "please inform me". There is no way to NACK a
request. It is not a permission to do so, just a "friendly
notification". And this is exactly not what we want when host enforcing
memory access.
>
>> In addition, it might make perfect
>> sense to have both, virtio-balloon and virtio-mem at the same time,
>> especially looking at the DEFLATE_ON_OOM or STATS features of
>> virtio-balloon. While virtio-mem is all about guarantees, virtio-
>> balloon is about cooperation.
>
> Thanks, and I intend to look more into this next week.
>
I know that it is tempting to force this concept into virtio-balloon. I
spent quite some time thinking about this (and possible other techniques
like implicit memory deflation on reboots) and decided not to do it. We
just end up trying to hack around all possible things that could go
wrong, while still not being able to handle all requirements properly.
--
Thanks,
David
^ permalink raw reply
* Re: [RFC] virtio-mem: paravirtualized memory
From: Michael S. Tsirkin @ 2017-06-16 15:04 UTC (permalink / raw)
To: David Hildenbrand
Cc: Andrea Arcangeli, linux-mm@kvack.org, qemu-devel@nongnu.org, KVM,
virtualization@lists.linux-foundation.org
In-Reply-To: <547865a9-d6c2-7140-47e2-5af01e7d761d@redhat.com>
On Fri, Jun 16, 2017 at 04:20:02PM +0200, David Hildenbrand wrote:
> Hi,
>
> this is an idea that is based on Andrea Arcangeli's original idea to
> host enforce guest access to memory given up using virtio-balloon using
> userfaultfd in the hypervisor. While looking into the details, I
> realized that host-enforcing virtio-balloon would result in way too many
> problems (mainly backwards compatibility) and would also have some
> conceptual restrictions that I want to avoid. So I developed the idea of
> virtio-mem - "paravirtualized memory".
Thanks! I went over this quickly, will read some more in the
coming days. I would like to ask for some clarifications
on one part meanwhile:
> Q: Why not reuse virtio-balloon?
>
> A: virtio-balloon is for cooperative memory management. It has a fixed
> page size
We are fixing that with VIRTIO_BALLOON_F_PAGE_CHUNKS btw.
I would appreciate you looking into that patchset.
> and will deflate in certain situations.
What does this refer to?
> Any change we
> introduce will break backwards compatibility.
Why does this have to be the case?
> virtio-balloon was not
> designed to give guarantees. Nobody can hinder the guest from
> deflating/reusing inflated memory.
Reusing without deflate is forbidden with TELL_HOST, right?
> In addition, it might make perfect
> sense to have both, virtio-balloon and virtio-mem at the same time,
> especially looking at the DEFLATE_ON_OOM or STATS features of
> virtio-balloon. While virtio-mem is all about guarantees, virtio-
> balloon is about cooperation.
Thanks, and I intend to look more into this next week.
--
MST
^ permalink raw reply
* [RFC] virtio-mem: paravirtualized memory
From: David Hildenbrand @ 2017-06-16 14:20 UTC (permalink / raw)
To: KVM, virtualization@lists.linux-foundation.org,
qemu-devel@nongnu.org, linux-mm@kvack.org
Cc: Andrea Arcangeli, Michael S. Tsirkin
Hi,
this is an idea that is based on Andrea Arcangeli's original idea to
host enforce guest access to memory given up using virtio-balloon using
userfaultfd in the hypervisor. While looking into the details, I
realized that host-enforcing virtio-balloon would result in way too many
problems (mainly backwards compatibility) and would also have some
conceptual restrictions that I want to avoid. So I developed the idea of
virtio-mem - "paravirtualized memory".
The basic idea is to add memory to the guest via a paravirtualized
mechanism (so the guest can hotplug it) and remove memory via a
mechanism similar to a balloon. This avoids having to online memory as
"online-movable" in the guest and allows more fain grained memory
hot(un)plug. In addition, migrating QEMU guests after adding/removing
memory gets a lot easier.
Actually, this has a lot in common with the XEN balloon or the Hyper-V
balloon (namely: paravirtualized hotplug and ballooning), but is very
different when going into the details.
Getting this all implemented properly will take quite some effort,
that's why I want to get some early feedback regarding the general
concept. If you have some alternative ideas, or ideas how to modify this
concept, I'll be happy to discuss. Just please make sure to have a look
at the requirements first.
-----------------------------------------------------------------------
0. Outline:
-----------------------------------------------------------------------
- I. General concept
- II. Use cases
- III. Identified requirements
- IV. Possible modifications
- V. Prototype
- VI. Problems to solve / things to sort out / missing in prototype
- VII. Questions
- VIII. Q&A
------------------------------------------------------------------------
I. General concept
------------------------------------------------------------------------
We expose memory regions to the guest via a paravirtualize interface. So
instead of e.g. a DIMM on x86, such memory is not anounced via ACPI.
Unmodified guests (without a virtio-mem driver) won't be able to see/use
this memory. The virtio-mem guest driver is needed to detect and manage
these memory areas. What makes this memory special is that it can grow
while the guest is running ("plug memory") and might shrink on a reboot
(to compensate "unplugged" memory - see next paragraph). Each virtio-mem
device manages exactly one such memory area. By having multiple ones
assigned to different NUMA nodes, we can modify memory on a NUMA basis.
Of course, we cannot shrink these memory areas while the guest is
running. To be able to unplug memory, we do something like a balloon
does, however limited to this very memory area that belongs to the
virtio-mem device. The guest will hand back small chunks of memory. If
we want to add memory to the guest, we first "replug" memory that has
previously been given up by the guest, before we grow our memory area.
On a reboot, we want to avoid any memory holes in our memory, therefore
we resize our memory area (shrink it) to compensate memory that has been
unplugged. This highly simplifies hotplugging memory in the guest (
hotplugging memory with random memory holes is basically impossible).
We have to make sure that all memory chunks the guest hands back on
unplug requests will not consume memory in the host. We do this by
write-protecting that memory chunk in the host and then dropping the
backing pages. The guest can read this memory (reading from the ZERO
page) but no longer write to it. For now, this will only work on
anonymous memory. We will use userfaultfd WP (write-protect mode) to
avoid creating too many VMAs. Huge pages will require more effort (no
explicit ZERO page).
As we unplug memory on a fine grained basis (and e.g. not on
a complete DIMM basis), there is no need to online virtio-mem memory
as online-movable. Also, memory unplug support for Windows might be
supported that way. You can find more details in the Q/A section below.
The important points here are:
- After a reboot, every memory the guest sees can be accessed and used.
(in contrast to e.g. the XEN balloon, see Q/A fore more details)
- Rebooting into an unmodified guest will not result into random
crashed. The guest will simply not be able to use all memory without a
virtio-mem driver.
- Adding/Removing memory will not require modifying the QEMU command
line on the migration target. Migration simply works (re-sizing memory
areas is already part of the migration protocol!). Essentially, this
makes adding/removing memory to/from a guest way simpler and
independent of the underlying architecture. If the guest OS can online
new memory, we can add more memory this way.
- Unplugged memory can be read. This allows e.g. kexec() without nasty
modifications. Especially relevant for Windows' kexec() variant.
- It will play nicely with other things mapped into the address space,
e.g. also other DIMMs or NVDIMM. virtio-mem will only work on its own
memory region (in contrast e.g. to virtio-balloon). Especially it will
not give up ("allocate") memory on other DIMMs, hindering them to get
unplugged the ACPI way.
- We can add/remove memory without running into KVM memory slot or other
(e.g. ACPI slot) restrictions. The granularity in which we can add
memory is only limited by the granularity the guest can add memory
(e.g. Windows 2MB, Linux on x86 128MB for now).
- By not having to online memory as online-movable we don't run into any
memory restrictions in the guest. E.g. page tables can only be created
on !movable memory. So while there might be plenty of online-movable
memory left, allocation of page tables might fail. See Q/A for more
details.
- The admin will not have to set memory offline in the guest first in
order to unplug it. virtio-mem will handle this internally and not
require interaction with an admin or a guest-agent.
Important restrictions of this concept:
- Guests without a virtio-mem guest driver can't see that memory.
- We will always require some boot memory that cannot get unplugged.
Also, virtio-mem memory (as all other hotplugged memory) cannot become
DMA memory under Linux. So the boot memory also defines the amount of
DMA memory.
- Hibernation/Sleep+Restore while virtio-mem is active is not supported.
On a reboot/fresh start, the size of the virtio-mem memory area might
change and a running/loaded guest can't deal with that.
- Unplug support for hugetlbfs/shmem will take quite some time to
support. The larger the used page size, the harder for the guest to
give up memory. We can still use DIMM based hotplug for that.
- Huge huge pages are problematic, as the guest would have to give up
e.g. 1GB chunks. This is not expected to be supported. We can still
use DIMM based hotplug for setups that require that.
- For any memory we unplug using this mechanism, for now we will still
have struct pages allocated in the guest. This means, that roughly
1.6% of unplugged memory will still be allocated in the guest, being
unusable.
------------------------------------------------------------------------
II. Use cases
------------------------------------------------------------------------
Of course, we want to deny any access to unplugged memory. In contrast
to virtio-balloon or other similar ideas (free page hinting), this is
not about cooperative memory management, but about guarantees. The idea
is, that both concepts can coexist.
So one use case is of course cloud providers. Customers can add
or remove memory to/from a VM without having to care about how to
online memory or in which amount to add memory in the first place in
order to remove it again. In cloud environments, we care about
guarantees. E.g. for virtio-balloon a malicious guest can simply reuse
any deflated memory, and the hypervisor can't even tell if the guest is
malicious (e.g. a harmless guest reboot might look like a malicious
guest). For virtio-mem, we guarantee that the guest can't reuse any
memory that it previously gave up.
But also for ordinary VMs (!cloud), this avoids having to online memory
in the guest as online-movable and therefore not running into allocation
problems if there are e.g. many processes needing many page tables on
!movable memory. Also here, we don't have to know how much memory we
want to remove some-when in the future before we add memory. (e.g. if we
add a 128GB DIMM, we can only remove that 128GB DIMM - if we are lucky).
We might be able to support memory unplug for Windows (as for now,
ACPI unplug is not supported), more details have to be clarified.
As we can grow these memory areas quite easily, another use case might
be guests that tell us they need more memory. Thinking about VMs to
protect containers, there seems to be the general problem that we don't
know how much memory the container will actually need. We could
implement a mechanism (in virtio-mem or guest driver), by which the
guest can request more memory. If the hypervisor agrees, it can simply
give the guest more memory. As this is all handled within QEMU,
migration is not a problem. Adding more memory will not result in new
DIMM devices.
------------------------------------------------------------------------
III. Identified requirements
------------------------------------------------------------------------
I considered the following requirements.
NUMA aware:
We want to be able to add/remove memory to/from NUMA nodes.
Different page-size support:
We want to be able to support different page sizes, e.g. because of
huge pages in the hypervisor or because host and guest have different
page sizes (powerpc 64k vs 4k).
Guarantees:
There has to be no way the guest can reuse unplugged memory without
host consent. Still, we could implement a mechanism for the guest to
request more memory. The hypervisor then has to decide how it wants to
handle that request.
Architecture independence:
We want this to work independently of other technologies bound to
specific architectures, like ACPI.
Avoid online-movable:
We don't want to have to online memory in the guest as online-movable
just to be able to unplug (at least parts of) it again.
Migration support:
Be able to migrate without too much hassle. Especially, to handle it
completely within QEMU (not having to add new devices to the target
command line).
Windows support:
We definitely want to support Windows guests in the long run.
Coexistence with other hotplug mechanisms:
Allow to hotplug DIMMs / NVDIMMs, therefore to share the "hotplug"
address space part with other devices.
Backwards compatibility:
Don't break if rebooting into an unmodified guest after having
unplugged some memory. All memory a freshly booted guest sees must not
contain memory holes that will crash it if it tries to access it.
------------------------------------------------------------------------
IV. Possible modifications
------------------------------------------------------------------------
Adding a guest->host request mechanism would make sense to e.g. be able
to request further memory from the hypervisor directly from the guest.
Adding memory will be much easier than removing memory. We can split
this up and first introduce "adding memory" and later add "removing
memory". Removing memory will require userfaultfd WP in the hypervisor
and a special fancy allocator in the guest. So this will take some time.
Adding a mechanism to trade in memory blocks might make sense to allow
some sort of memory compaction. However I expect this to be highly
complicated and basically not feasible.
Being able to unplug memory "any" memory instead of only memory
belonging to the virtio-mem device sounds tempting (and simplifies
certain parts), however it has a couple of side effects I want to avoid.
You can read more about that in the Q/A below.
------------------------------------------------------------------------
V. Prototype
------------------------------------------------------------------------
To identify potential problems I developed a very basic prototype. It
is incomplete, full of hacks and most probably broken in various ways.
I used it only in the given setup, only on x86 and only with an initrd.
It uses a fixed page size of 256k for now, has a very ugly allocator
hack in the guest, the virtio protocol really needs some tuning and
an async job interface towards the user is missing. Instead of using
userfaultfd WP, I am using simply mprotect() in this prototype. Basic
migration works (not involving userfaultfd).
Please, don't even try to review it (that's why I will also not attach
any patches to this mail :) ), just use this as an inspiration what this
could look like. You can find the latest hack at:
QEMU: https://github.com/davidhildenbrand/qemu/tree/virtio-mem
Kernel: https://github.com/davidhildenbrand/linux/tree/virtio-mem
Use the kernel in the guest and make sure to compile the virtio-mem
driver into the kernel (CONFIG_VIRTIO_MEM=y). A host kernel patch is
contained to allow atomic resize of KVM memory regions, however it is
pretty much untested.
1. Starting a guest with virtio-mem memory:
We will create a guest with 2 NUMA nodes and 4GB of "boot + DMA"
memory. This memory is visible also to guests without virtio-mem.
Also, we will add 4GB to NUMA node 0 and 3GB to NUMA node 1 using
virtio-mem. We allow both virtio-mem devices to grow up to 8GB. The
last 4 lines are the important part.
--> qemu/x86_64-softmmu/qemu-system-x86_64 \
--enable-kvm
-m 4G,maxmem=20G \
-smp sockets=2,cores=2 \
-numa node,nodeid=0,cpus=0-1 -numa node,nodeid=1,cpus=2-3 \
-machine pc \
-kernel linux/arch/x86_64/boot/bzImage \
-nodefaults \
-chardev stdio,id=serial \
-device isa-serial,chardev=serial \
-append "console=ttyS0 rd.shell rd.luks=0 rd.lvm=0" \
-initrd /boot/initramfs-4.10.8-200.fc25.x86_64.img \
-chardev socket,id=monitor,path=/var/tmp/monitor,server,nowait \
-mon chardev=monitor,mode=readline \
-object memory-backend-ram,id=mem0,size=4G,max-size=8G \
-device virtio-mem-pci,id=reg0,memdev=mem0,node=0 \
-object memory-backend-ram,id=mem1,size=3G,max-size=8G \
-device virtio-mem-pci,id=reg1,memdev=mem1,node=1
2. Listing current memory assignment:
--> (qemu) info memory-devices
Memory device [virtio-mem]: "reg0"
addr: 0x140000000
node: 0
size: 4294967296
max-size: 8589934592
memdev: /objects/mem0
Memory device [virtio-mem]: "reg1"
addr: 0x340000000
node: 1
size: 3221225472
max-size: 8589934592
memdev: /objects/mem1
--> (qemu) info numa
2 nodes
node 0 cpus: 0 1
node 0 size: 6144 MB
node 1 cpus: 2 3
node 1 size: 5120 MB
3. Resize a virtio-mem device: Unplugging memory.
Setting reg0 to 2G (remove 2G from NUMA node 0)
--> (qemu) virtio-mem reg0 2048
virtio-mem reg0 2048
--> (qemu) info numa
info numa
2 nodes
node 0 cpus: 0 1
node 0 size: 4096 MB
node 1 cpus: 2 3
node 1 size: 5120 MB
4. Resize a virtio-mem device: Plugging memory
Setting reg0 to 8G (adding 6G to NUMA node 0) will replug 2G and plug
4G, automatically re-sizing the memory area. You might experience
random crashes at this point if the host kernel missed a KVM patch
(as the memory slot is not re-sized in an atomic fashion).
--> (qemu) virtio-mem reg0 8192
virtio-mem reg0 8192
--> (qemu) info numa
info numa
2 nodes
node 0 cpus: 0 1
node 0 size: 10240 MB
node 1 cpus: 2 3
node 1 size: 5120 MB
5. Resize a virtio-mem device: Try to unplug all memory.
Setting reg0 to 0G (removing 8G from NUMA node 0) will not work. The
guest will not be able to unplug all memory. In my example, 164M
cannot be unplugged (out of memory).
--> (qemu) virtio-mem reg0 0
virtio-mem reg0 0
--> (qemu) info numa
info numa
2 nodes
node 0 cpus: 0 1
node 0 size: 2212 MB
node 1 cpus: 2 3
node 1 size: 5120 MB
--> (qemu) info virtio-mem reg0
info virtio-mem reg0
Status: ready
Request status: vm-oom
Page size: 2097152 bytes
--> (qemu) info memory-devices
Memory device [virtio-mem]: "reg0"
addr: 0x140000000
node: 0
size: 171966464
max-size: 8589934592
memdev: /objects/mem0
Memory device [virtio-mem]: "reg1"
addr: 0x340000000
node: 1
size: 3221225472
max-size: 8589934592
memdev: /objects/mem1
At any point, we can migrate our guest without having to care about
modifying the QEMU command line on the target side. Simply start the
target e.g. with an additional '-incoming "exec: cat IMAGE"' and you're
done.
------------------------------------------------------------------------
VI. Problems to solve / things to sort out / missing in prototype
------------------------------------------------------------------------
General:
- We need an async job API to send the unplug/replug/plug requests to
the guest and query the state. [medium/hard]
- Handle various alignment problems. [medium]
- We need a virtio spec
Relevant for plug:
- Resize QEMU memory regions while the guest is running (esp. grow).
While I implemented a demo solution for KVM memory slots, something
similar would be needed for vhost. Re-sizing of memory slots has to be
an atomic operation. [medium]
- NUMA: Most probably the NUMA node should not be part of the virtio-mem
device, this should rather be indicated via e.g. ACPI. [medium]
- x86: Add the complete possible memory to the a820 map as reserved.
[medium]
- x86/powerpc/...: Indicate to which NUMA node the memory belongs using
ACPI. [medium]
- x86/powerpc/...: Share address space with ordinary DIMMS/NVDIMMs, for
now this is blocked for simplicity. [medium/hard]
- If the bitmaps become too big, migrate them like memory. [medium]
Relevant for unplug:
- Allocate memory in Linux from a specific memory range. Windows has a
nice interface for that (at least it looks nice when reading the API).
This could be done using fake NUMA nodes or a new ZONE. My prototype
just uses a very ugly hack. [very hard]
- Use userfaultfd WP (write-protect) insted of mprotect. Especially,
have multiple userfaultfd user in QEMU at a time (postcopy).
[medium/hard]
Stuff for the future:
- Huge pages are problematic (no ZERO page support). This might not be
trivial to support. [hard/very hard]
- Try to free struct pages, to avoid the 1.6% overhead [very very hard]
------------------------------------------------------------------------
VII. Questions
------------------------------------------------------------------------
To get unplug working properly, it will require quite some effort,
that's why I want to get some basic feedback before continuing working
on a RFC implementation + RFC virtio spec.
a) Did I miss anything important? Are there any ultimate blockers that I
ignored? Any concepts that are broken?
b) Are there any alternatives? Any modifications that could make life
easier while still taking care of the requirements?
c) Are there other use cases we should care about and focus on?
d) Am I missing any requirements? What else could be important for
!cloud and cloud?
e) Are there any possible solutions to the allocator problem (allocating
memory from a specific memory area)? Please speak up!
f) Anything unclear?
e) Any feelings about this? Yay or nay?
As you reached this point: Thanks for having a look!!! Highly appreciated!
------------------------------------------------------------------------
VIII. Q&A
------------------------------------------------------------------------
---
Q: What's the problem with ordinary memory hot(un)plug?
A: 1. We can only unplug in the granularity we plugged. So we have to
know in advance, how much memory we want to remove later on. If we
plug a 2G dimm, we can only unplug a 2G dimm.
2. We might run out of memory slots. Although very unlikely, this
would strike if we try to always plug small modules in order to be
able to unplug again (e.g. loads of 128MB modules).
3. Any locked page in the guest can hinder us from unplugging a dimm.
Even if memory was onlined as online_movable, a single locked page
can hinder us from unplugging that memory dimm.
4. Memory has to be onlined as online_movable. If we don't put that
memory into the movable zone, any non-movable kernel allocation
could end up on it, turning the complete dimm unpluggable. As
certain allocations cannot go into the movable zone (e.g. page
tables), the ratio between online_movable/online memory depends on
the workload in the guest. Ratios of 50% -70% are usually fine.
But it could happen, that there is plenty of memory available,
but kernel allocations fail. (source: Andrea Arcangeli)
5. Unplugging might require several attempts. It takes some time to
migrate all memory from the dimm. At that point, it is then not
really obvious why it failed, and whether it could ever succeed.
6. Windows does support memory hotplug but not memory hotunplug. So
this could be a way to support it also for Windows.
---
Q: Will this work with Windows?
A: Most probably not in the current form. Memory has to be at least
added to the a820 map and ACPI (NUMA). Hyper-V ballon is also able to
hotadd memory using a paravirtualized interface, so there are very
good chances that this will work. But we won't know for sure until we
also start prototyping.
---
Q: How does this compare to virtio-balloo?
A: In contrast to virtio-balloon, virtio-mem
1. Supports multiple page sizes, even different ones for different
virtio-mem devices in a guest.
2. Is NUMA aware.
3. Is able to add more memory.
4. Doesn't work on all memory, but only on the managed one.
5. Has guarantees. There is now way for the guest to reclaim memory.
---
Q: How does this compare to XEN balloon?
A: XEN balloon also has a way to hotplug new memory. However, on a
reboot, the guest will "see" more memory than it actually has.
Compared to XEN balloon, virtio-mem:
1. Supports multiple page sizes.
2. Is NUMA aware.
3. The guest can survive a reboot into a system without the guest
driver. If the XEN guest driver doesn't come up, the guest will
get killed once it touches too much memory.
4. Reboots don't require any hacks.
5. The guest knows which memory is special. And it remains special
during a reboot. Hotplugged memory not suddenly becomes base
memory. The balloon mechanism will only work on a specific memory
area.
---
Q: How does this compare to Hyper-V balloon?
A: Based on the code from the Linux Hyper-V balloon driver, I can say
that Hyper-V also has a way to hotplug new memory. However, memory
will remain plugged on a reboot. Therefore, the guest will see more
memory than the hypervisor actually wants to assign to it.
Virtio-mem in contrast:
1. Supports multiple page sizes.
2. Is NUMA aware.
3. I have no idea what happens under Hyper-v when
a) rebooting into a guest without a fitting guest driver
b) kexec() touches all memory
c) the guest misbehaves
4. The guest knows which memory is special. And it remains special
during a reboot. Hotpplugged memory not suddenly becomes base
memory. The balloon mechanism will only work on a specific memory
area.
In general, it looks like the hypervisor has to deal with malicious
guests trying to access more memory than desired by providing enough
swap space.
---
Q: How is virtio-mem NUMA aware?
A: Each virtio-mem device belongs exactly to one NUMA node (if NUMA is
enabled). As we can resize these regions separately, we can control
from/to which node to remove/add memory.
---
Q: Why do we need support for multiple page sizes?
A: If huge pages are used in the host, we can only guarantee that they
are not accessible by the guest anymore, if the guest gives up memory
in this granularity. We prepare for that. Also, powerpc can have 64k
pages in the host but 4k pages in the guest. So the guest must only
give up 64k chunks. In addition, unplugging 4k pages might be bad
when it comes to fragmentation. My prototype currently uses 256k. We
can make this configurable - and it can vary for each virtio-mem
device.
---
Q: What are the limitations with paravirtualized memory hotplug?
A: The same as for DIMM based hotplug, but we don't run out of any
memory/ACPI slots. E.g. on x86 Linux, only 128MB chunks can be
hotplugged, on x86 Windows it's 2MB. In addition, of course we
have to take care of maximum address limits in the guest. The idea
is to communicate these limits to the hypervisor via virtio-mem,
to give hints when trying to add/remove memory.
---
Q: Why not simply unplug *any* memory like virtio-balloon does?
A: This could be done and a previous prototype did it like that.
However, there are some points to consider here.
1. If we combine this with ordinary memory hotplug (DIMM), we most
likely won't be able to unplug DIMMs anymore as virtio-mem memory
gets "allocated" on these.
2. All guests using virtio-mem cannot use huge pages as backing
storage at all (as virtio-mem only supports anonymous pages).
3. We need to track unplugged memory for the complete address space,
so we need a global state in QEMU. Bitmaps get bigger. We will not
be abe to dynamically grow the bitmaps for a virtio-mem device.
4. Resolving/checking memory to be unplugged gets significantly
harder. How should the guest know which memory it can unplug for a
specific virtio-mem device? E.g. if NUMA is active, only that NUMA
node to which a virtio-mem device belongs can be used.
5. We will need userfaultfd handler for the complete address space,
not just for the virtio-mem managed memory.
Especially, if somebody hotplugs a DIMM, we dynamically will have
to enable the userfaultfd handler.
6. What shall we do if somebody hotplugs a DIMM with huge pages? How
should we tell the guest, that this memory cannot be used for
unplugging?
In summary: This concept is way cleaner, but also harder to
implement.
---
Q: Why not reuse virtio-balloon?
A: virtio-balloon is for cooperative memory management. It has a fixed
page size and will deflate in certain situations. Any change we
introduce will break backwards compatibility. virtio-balloon was not
designed to give guarantees. Nobody can hinder the guest from
deflating/reusing inflated memory. In addition, it might make perfect
sense to have both, virtio-balloon and virtio-mem at the same time,
especially looking at the DEFLATE_ON_OOM or STATS features of
virtio-balloon. While virtio-mem is all about guarantees, virtio-
balloon is about cooperation.
---
Q: Why not reuse acpi hotplug?
A: We can easily run out of slots, migration in QEMU will just be
horrible and we don't want to bind virtio* to architecture specific
technologies.
E.g. thinking about s390x - no ACPI. Also, mixing an ACPI driver with
a virtio-driver sounds very weird. If the virtio-driver performs the
hotplug itself, we might later perform some extra tricks: e.g.
actually unplug certain regions to give up some struct pages.
We want to manage the way memory is added/removed completely in QEMU.
We cannot simply add new device from within QEMU and expect that
migration in QEMU will work.
---
Q: Why do we need resizable memory regions?
A: Migration in QEMU is special. Any device we have on our source VM has
to already be around on our target VM. So simply creating random
devides internally in QEMU is not going to work. The concept of
resizable memory regions in QEMU already exists and is part of the
migration protocol. Before memory is migrated, the memory is resized.
So in essence, this makes migration support _a lot_ easier.
In addition, we won't run in any slot number restriction when
automatically managing how to add memory in QEMU.
---
Q: Why do we have to resize memory regions on a reboot?
A: We have to compensate all memory that has been unplugged for that
area by shrinking it, so that a fresh guest can use all memory when
initializing the virtio-mem device.
---
Q: Why do we need userfaultfd?
A: mprotect() will create a lot of VMAs in the kernel. This will degrade
performance and might even fail at one point. userfaultfd avoids this
by not creating a new VMA for every protected range. userfaultfd WP
is currently still under development and suffers from false positives
that make it currently impossible to properly integrate this into the
prototype.
---
Q: Why do we have to allow reading unplugged memory?
A: E.g. if the guest crashes and want's to write a memory dump, it will
blindly access all memory. While we could find ways to fixup kexec,
Windows dumps might be more problematic. Allowing the guest to read
all memory (resulting in reading all 0's) safes us from a lot of
trouble.
The downside is, that page tables full of zero pages might be
created. (we might be able to find ways to optimize this)
---
Q: Will this work with postcopy live-migration?
A: Not in the current form. And it doesn't really make sense to spend
time on it as long as we don't use userfaultfd. Combining both
handlers will be interesting. It can be done with some effort on the
QEMU side.
---
Q: What's the problem with shmem/hugetlbfs?
A: We currently rely on the ZERO page to be mapped when the guest tries
to read unplugged memory. For shmem/hugetlbfs, there is no ZERO page,
so read access would result in memory getting populated. We could
either introduce an explicit ZERO page, or manage it using one dummy
ZERO page (using regular usefaultfd, allow only one such page to be
mapped at a time). For now, only anonymous memory.
---
Q: Ripping out random page ranges, won't this fragment our guest memory?
A: Yes, but depending on the virtio-mem page size, this might be more or
less problematic. The smaller the virtio-mem page size, the more we
fragment and make small allocations fail. The bigger the virtio-mem
page size, the higher the chance that we can't unplug any more
memory.
---
Q: Why can't we use memory compaction like virtio-balloon?
A: If the virtio-mem page size > PAGE_SIZE, we can't do ordinary
page migration, migration would have to be done in blocks. We could
later add an guest->host virtqueue, via which the guest can
"exchange" memory ranges. However, also mm has to support this kind
of migration. So it is not completely out of scope, but will require
quite some work.
---
Q: Do we really need yet another paravirtualized interface for this?
A: You tell me :)
---
Thanks,
David
^ permalink raw reply
* Re: [virtio-dev] [RFC PATCH linux] iommu: Add virtio-iommu driver
From: Jean-Philippe Brucker @ 2017-06-16 11:36 UTC (permalink / raw)
To: Bharat Bhushan, iommu@lists.linux-foundation.org,
kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
virtio-dev@lists.oasis-open.org
Cc: cdall@linaro.org, lorenzo.pieralisi@arm.com, mst@redhat.com,
marc.zyngier@arm.com, joro@8bytes.org, will.deacon@arm.com,
robin.murphy@arm.com
In-Reply-To: <AM5PR0401MB25450471C7161CC6386FC8CE9AC10@AM5PR0401MB2545.eurprd04.prod.outlook.com>
On 16/06/17 09:48, Bharat Bhushan wrote:
> Hi Jean
>> +static int viommu_map(struct iommu_domain *domain, unsigned long iova,
>> + phys_addr_t paddr, size_t size, int prot) {
>> + int ret;
>> + struct viommu_domain *vdomain = to_viommu_domain(domain);
>> + struct virtio_iommu_req_map req = {
>> + .head.type = VIRTIO_IOMMU_T_MAP,
>> + .address_space = cpu_to_le32(vdomain->id),
>> + .virt_addr = cpu_to_le64(iova),
>> + .phys_addr = cpu_to_le64(paddr),
>> + .size = cpu_to_le64(size),
>> + };
>> +
>> + pr_debug("map %llu 0x%lx -> 0x%llx (%zu)\n", vdomain->id, iova,
>> + paddr, size);
>
> A query, when I am tracing above prints I see same physical address is mapped with two different virtual address, do you know why kernel does this?
That really depends which driver is calling into viommu. iommu_map is
called from the DMA API, which can be used by any device drivers. Within
an address space, multiple IOVAs pointing to the same PA isn't forbidden.
For example, looking at MAP requests for a virtio-net device, I get the
following trace:
ioas[1] map 0xfffffff3000 -> 0x8faa0000 (4096)
ioas[1] map 0xfffffff2000 -> 0x8faa0000 (4096)
ioas[1] map 0xfffffff1000 -> 0x8faa0000 (4096)
ioas[1] map 0xfffffff0000 -> 0x8faa0000 (4096)
ioas[1] map 0xffffffef000 -> 0x8faa0000 (4096)
ioas[1] map 0xffffffee000 -> 0x8faa0000 (4096)
ioas[1] map 0xffffffed000 -> 0x8faa0000 (4096)
ioas[1] map 0xffffffec000 -> 0x8faa0000 (4096)
ioas[1] map 0xffffffeb000 -> 0x8faa0000 (4096)
ioas[1] map 0xffffffea000 -> 0x8faa0000 (4096)
ioas[1] map 0xffffffe8000 -> 0x8faa0000 (8192)
...
During initialization, the virtio-net driver primes the rx queue with
receive buffers, that the host will then fill with network packets. It
calls virtqueue_add_inbuf_ctx to create descriptors on the rx virtqueue
for each buffer. Each buffer is 0x180 bytes here, so one 4k page can
contain around 10 (actually 11, with the last one crossing page boundary).
I guess the call trace goes like this:
virtnet_open
try_fill_recv
add_recvbuf_mergeable
virtqueue_add_inbuf_ctx
vring_map_one_sg
dma_map_page
__iommu_dma_map
But the IOMMU cannot map fragments of pages, since the granule is 0x1000.
Therefore when virtqueue_add_inbuf_ctx maps the buffer, __iommu_dma_map
aligns address and size on full pages. Someone motivated could probably
optimize this by caching mapped pages and reusing IOVAs, but currently
that's how it goes.
Thanks,
Jean
^ permalink raw reply
* RE: [virtio-dev] [RFC PATCH linux] iommu: Add virtio-iommu driver
From: Bharat Bhushan @ 2017-06-16 8:48 UTC (permalink / raw)
To: Jean-Philippe Brucker, iommu@lists.linux-foundation.org,
kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
virtio-dev@lists.oasis-open.org
Cc: cdall@linaro.org, lorenzo.pieralisi@arm.com, mst@redhat.com,
marc.zyngier@arm.com, joro@8bytes.org, will.deacon@arm.com,
robin.murphy@arm.com
In-Reply-To: <20170407192314.26720-1-jean-philippe.brucker@arm.com>
Hi Jean
> -----Original Message-----
> From: virtio-dev@lists.oasis-open.org [mailto:virtio-dev@lists.oasis-
> open.org] On Behalf Of Jean-Philippe Brucker
> Sent: Saturday, April 08, 2017 12:53 AM
> To: iommu@lists.linux-foundation.org; kvm@vger.kernel.org;
> virtualization@lists.linux-foundation.org; virtio-dev@lists.oasis-open.org
> Cc: cdall@linaro.org; will.deacon@arm.com; robin.murphy@arm.com;
> lorenzo.pieralisi@arm.com; joro@8bytes.org; mst@redhat.com;
> jasowang@redhat.com; alex.williamson@redhat.com;
> marc.zyngier@arm.com
> Subject: [virtio-dev] [RFC PATCH linux] iommu: Add virtio-iommu driver
>
> The virtio IOMMU is a para-virtualized device, allowing to send IOMMU
> requests such as map/unmap over virtio-mmio transport. This driver should
> illustrate the initial proposal for virtio-iommu, that you hopefully received
> with it. It handle attach, detach, map and unmap requests.
>
> The bulk of the code is to create requests and send them through virtio.
> Implementing the IOMMU API is fairly straightforward since the virtio-iommu
> MAP/UNMAP interface is almost identical. I threw in a custom
> map_sg() function which takes up some space, but is optional. The core
> function would send a sequence of map requests, waiting for a reply
> between each mapping. This optimization avoids yielding to the host after
> each map, and instead prepares a batch of requests in the virtio ring and
> kicks the host once.
>
> It must be applied on top of the probe deferral work for IOMMU, currently
> under discussion. This allows to dissociate early driver detection and device
> probing: device-tree or ACPI is parsed early to find which devices are
> translated by the IOMMU, but the IOMMU itself cannot be probed until the
> core virtio module is loaded.
>
> Enabling DEBUG makes it extremely verbose at the moment, but it should be
> calmer in next versions.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> ---
> drivers/iommu/Kconfig | 11 +
> drivers/iommu/Makefile | 1 +
> drivers/iommu/virtio-iommu.c | 980
> ++++++++++++++++++++++++++++++++++++++
> include/uapi/linux/Kbuild | 1 +
> include/uapi/linux/virtio_ids.h | 1 +
> include/uapi/linux/virtio_iommu.h | 142 ++++++
> 6 files changed, 1136 insertions(+)
> create mode 100644 drivers/iommu/virtio-iommu.c create mode 100644
> include/uapi/linux/virtio_iommu.h
>
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig index
> 37e204f3d9be..8cd56ee9a93a 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -359,4 +359,15 @@ config MTK_IOMMU_V1
>
> if unsure, say N here.
>
> +config VIRTIO_IOMMU
> + tristate "Virtio IOMMU driver"
> + depends on VIRTIO_MMIO
> + 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
> 195f7b997d8e..1199d8475802 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -27,3 +27,4 @@ obj-$(CONFIG_TEGRA_IOMMU_SMMU) += tegra-
> smmu.o
> 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_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..1cf4f57b7817
> --- /dev/null
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -0,0 +1,980 @@
> +/*
> + * Virtio driver for the paravirtualized IOMMU
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307,
> USA.
> + *
> + * Copyright (C) 2017 ARM Limited
> + *
> + * Author: Jean-Philippe Brucker <jean-philippe.brucker@arm.com> */
> +
> +#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/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>
> +
> +struct viommu_dev {
> + struct iommu_device iommu;
> + struct device *dev;
> + struct virtio_device *vdev;
> +
> + struct virtqueue *vq;
> + struct list_head pending_requests;
> + /* Serialize anything touching the vq and the request list */
> + spinlock_t vq_lock;
> +
> + struct list_head list;
> +
> + /* Device configuration */
> + u64 pgsize_bitmap;
> + u64 aperture_start;
> + u64 aperture_end;
> +};
> +
> +struct viommu_mapping {
> + phys_addr_t paddr;
> + struct interval_tree_node iova;
> +};
> +
> +struct viommu_domain {
> + struct iommu_domain domain;
> + struct viommu_dev *viommu;
> + struct mutex mutex;
> + u64 id;
> +
> + spinlock_t mappings_lock;
> + struct rb_root mappings;
> +
> + /* Number of devices attached to this domain */
> + unsigned long attached;
> +};
> +
> +struct viommu_endpoint {
> + struct viommu_dev *viommu;
> + struct viommu_domain *vdomain;
> +};
> +
> +struct viommu_request {
> + struct scatterlist head;
> + struct scatterlist tail;
> +
> + int written;
> + struct list_head list;
> +};
> +
> +/* TODO: use an IDA */
> +static atomic64_t viommu_domain_ids_gen;
> +
> +#define to_viommu_domain(domain) container_of(domain, struct
> +viommu_domain, domain)
> +
> +/* Virtio transport */
> +
> +static int viommu_status_to_errno(u8 status) {
> + switch (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 int viommu_get_req_size(struct virtio_iommu_req_head *req, size_t
> *head,
> + size_t *tail)
> +{
> + size_t size;
> + union virtio_iommu_req r;
> +
> + *tail = sizeof(struct virtio_iommu_req_tail);
> +
> + switch (req->type) {
> + case VIRTIO_IOMMU_T_ATTACH:
> + size = sizeof(r.attach);
> + break;
> + case VIRTIO_IOMMU_T_DETACH:
> + size = sizeof(r.detach);
> + break;
> + case VIRTIO_IOMMU_T_MAP:
> + size = sizeof(r.map);
> + break;
> + case VIRTIO_IOMMU_T_UNMAP:
> + size = sizeof(r.unmap);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + *head = size - *tail;
> + return 0;
> +}
> +
> +static int viommu_receive_resp(struct viommu_dev *viommu, int
> +nr_expected) {
> +
> + unsigned int len;
> + int nr_received = 0;
> + struct viommu_request *req, *pending, *next;
> +
> + pending = list_first_entry_or_null(&viommu->pending_requests,
> + struct viommu_request, list);
> + if (WARN_ON(!pending))
> + return 0;
> +
> + while ((req = virtqueue_get_buf(viommu->vq, &len)) != NULL) {
> + if (req != pending) {
> + dev_warn(viommu->dev, "discarding stale
> request\n");
> + continue;
> + }
> +
> + pending->written = len;
> +
> + if (++nr_received == nr_expected) {
> + list_del(&pending->list);
> + /*
> + * In an ideal world, we'd wake up the waiter for this
> + * group of requests here. But everything is painfully
> + * synchronous, so waiter is the caller.
> + */
> + break;
> + }
> +
> + next = list_next_entry(pending, list);
> + list_del(&pending->list);
> +
> + if (WARN_ON(list_empty(&viommu->pending_requests)))
> + return 0;
> +
> + pending = next;
> + }
> +
> + return nr_received;
> +}
> +
> +/* Must be called with vq_lock held */
> +static int _viommu_send_reqs_sync(struct viommu_dev *viommu,
> + struct viommu_request *req, int nr,
> + int *nr_sent)
> +{
> + int i, ret;
> + ktime_t timeout;
> + int nr_received = 0;
> + struct scatterlist *sg[2];
> + /*
> + * FIXME: as it stands, 1s timeout per request. This is a voluntary
> + * exaggeration because I have no idea how real our ktime is. Are we
> + * using a RTC? Are we aware of steal time? I don't know much about
> + * this, need to do some digging.
> + */
> + unsigned long timeout_ms = 1000;
> +
> + *nr_sent = 0;
> +
> + for (i = 0; i < nr; i++, req++) {
> + /*
> + * The backend will allocate one indirect descriptor for each
> + * request, which allows to double the ring consumption, but
> + * might be slower.
> + */
> + req->written = 0;
> +
> + sg[0] = &req->head;
> + sg[1] = &req->tail;
> +
> + ret = virtqueue_add_sgs(viommu->vq, sg, 1, 1, req,
> + GFP_ATOMIC);
> + if (ret)
> + break;
> +
> + list_add_tail(&req->list, &viommu->pending_requests);
> + }
> +
> + if (i && !virtqueue_kick(viommu->vq))
> + return -EPIPE;
> +
> + /*
> + * Absolutely no wiggle room here. We're not allowed to sleep as
> callers
> + * might be holding spinlocks, so we have to poll like savages until
> + * something appears. Hopefully the host already handled the
> request
> + * during the above kick and returned it to us.
> + *
> + * A nice improvement would be for the caller to tell us if we can
> sleep
> + * whilst mapping, but this has to go through the IOMMU/DMA API.
> + */
> + timeout = ktime_add_ms(ktime_get(), timeout_ms * i);
> + while (nr_received < i && ktime_before(ktime_get(), timeout)) {
> + nr_received += viommu_receive_resp(viommu, i -
> nr_received);
> + if (nr_received < i) {
> + /*
> + * FIXME: what's a good way to yield to host? A
> second
> + * virtqueue_kick won't have any effect since we
> haven't
> + * added any descriptor.
> + */
> + udelay(10);
> + }
> + }
> + dev_dbg(viommu->dev, "request took %lld us\n",
> + ktime_us_delta(ktime_get(), ktime_sub_ms(timeout,
> timeout_ms * i)));
> +
> + if (nr_received != i)
> + ret = -ETIMEDOUT;
> +
> + if (ret == -ENOSPC && nr_received)
> + /*
> + * We've freed some space since virtio told us that the ring is
> + * full, tell the caller to come back later (after releasing the
> + * lock first, to be fair to other threads)
> + */
> + ret = -EAGAIN;
> +
> + *nr_sent = nr_received;
> +
> + return ret;
> +}
> +
> +/**
> + * viommu_send_reqs_sync - add a batch of requests, kick the host and
> wait for
> + * them to return
> + *
> + * @req: array of requests
> + * @nr: size of the array
> + * @nr_sent: contains the number of requests actually sent after this
> function
> + * returns
> + *
> + * Return 0 on success, or an error if we failed to send some of the
> requests.
> + */
> +static int viommu_send_reqs_sync(struct viommu_dev *viommu,
> + struct viommu_request *req, int nr,
> + int *nr_sent)
> +{
> + int ret;
> + int sent = 0;
> + unsigned long flags;
> +
> + *nr_sent = 0;
> + do {
> + spin_lock_irqsave(&viommu->vq_lock, flags);
> + ret = _viommu_send_reqs_sync(viommu, req, nr, &sent);
> + spin_unlock_irqrestore(&viommu->vq_lock, flags);
> +
> + *nr_sent += sent;
> + req += sent;
> + nr -= sent;
> + } while (ret == -EAGAIN);
> +
> + return ret;
> +}
> +
> +/**
> + * viommu_send_req_sync - send one request and wait for reply
> + *
> + * @head_ptr: pointer to a virtio_iommu_req_* structure
> + *
> + * Returns 0 if the request was successful, or an error number
> +otherwise. No
> + * distinction is done between transport and request errors.
> + */
> +static int viommu_send_req_sync(struct viommu_dev *viommu, void
> +*head_ptr) {
> + int ret;
> + int nr_sent;
> + struct viommu_request req;
> + size_t head_size, tail_size;
> + struct virtio_iommu_req_tail *tail;
> + struct virtio_iommu_req_head *head = head_ptr;
> +
> + ret = viommu_get_req_size(head, &head_size, &tail_size);
> + if (ret)
> + return ret;
> +
> + dev_dbg(viommu->dev, "Sending request 0x%x, %zu bytes\n",
> head->type,
> + head_size + tail_size);
> +
> + tail = head_ptr + head_size;
> +
> + sg_init_one(&req.head, head, head_size);
> + sg_init_one(&req.tail, tail, tail_size);
> +
> + ret = viommu_send_reqs_sync(viommu, &req, 1, &nr_sent);
> + if (ret || !req.written || nr_sent != 1) {
> + dev_err(viommu->dev, "failed to send command\n");
> + return -EIO;
> + }
> +
> + ret = -viommu_status_to_errno(tail->status);
> +
> + if (ret)
> + dev_dbg(viommu->dev, " completed with %d\n", ret);
> +
> + return ret;
> +}
> +
> +static int viommu_tlb_map(struct viommu_domain *vdomain, unsigned
> long iova,
> + phys_addr_t paddr, size_t size)
> +{
> + unsigned long flags;
> + struct viommu_mapping *mapping;
> +
> + mapping = kzalloc(sizeof(*mapping), GFP_ATOMIC);
> + if (!mapping)
> + return -ENOMEM;
> +
> + mapping->paddr = paddr;
> + mapping->iova.start = iova;
> + mapping->iova.last = iova + size - 1;
> +
> + spin_lock_irqsave(&vdomain->mappings_lock, flags);
> + interval_tree_insert(&mapping->iova, &vdomain->mappings);
> + spin_unlock_irqrestore(&vdomain->mappings_lock, flags);
> +
> + return 0;
> +}
> +
> +static size_t viommu_tlb_unmap(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);
> +
> + /*
> + * 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;
> +}
> +
> +/* IOMMU API */
> +
> +static bool viommu_capable(enum iommu_cap cap) {
> + return false; /* :( */
> +}
> +
> +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(struct viommu_domain), GFP_KERNEL);
> + if (!vdomain)
> + return NULL;
> +
> + vdomain->id =
> atomic64_inc_return_relaxed(&viommu_domain_ids_gen);
> +
> + mutex_init(&vdomain->mutex);
> + spin_lock_init(&vdomain->mappings_lock);
> + vdomain->mappings = RB_ROOT;
> +
> + pr_debug("alloc domain of type %d -> %llu\n", type, vdomain->id);
> +
> + if (type == IOMMU_DOMAIN_DMA &&
> + iommu_get_dma_cookie(&vdomain->domain)) {
> + kfree(vdomain);
> + return NULL;
> + }
> +
> + return &vdomain->domain;
> +}
> +
> +static void viommu_domain_free(struct iommu_domain *domain) {
> + struct viommu_domain *vdomain = to_viommu_domain(domain);
> +
> + pr_debug("free domain %llu\n", vdomain->id);
> +
> + iommu_put_dma_cookie(domain);
> +
> + /* Free all remaining mappings (size 2^64) */
> + viommu_tlb_unmap(vdomain, 0, 0);
> +
> + kfree(vdomain);
> +}
> +
> +static int viommu_attach_dev(struct iommu_domain *domain, struct
> device
> +*dev) {
> + int i;
> + int ret = 0;
> + struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> + struct viommu_endpoint *vdev = fwspec->iommu_priv;
> + struct viommu_domain *vdomain = to_viommu_domain(domain);
> + struct virtio_iommu_req_attach req = {
> + .head.type = VIRTIO_IOMMU_T_ATTACH,
> + .address_space = cpu_to_le32(vdomain->id),
> + };
> +
> + mutex_lock(&vdomain->mutex);
> + if (!vdomain->viommu) {
> + struct viommu_dev *viommu = vdev->viommu;
> +
> + vdomain->viommu = viommu;
> +
> + domain->pgsize_bitmap = viommu-
> >pgsize_bitmap;
> + domain->geometry.aperture_start = viommu-
> >aperture_start;
> + domain->geometry.aperture_end = viommu-
> >aperture_end;
> + domain->geometry.force_aperture = true;
> +
> + } 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;
> +
> + /*
> + * When attaching the device to a new domain, it will be detached
> from
> + * the old one and, if as as a result the old domain isn't attached to
> + * any device, all mappings are removed from the old domain and it is
> + * freed. (Note that we can't use get_domain_for_dev here, it
> returns
> + * the default domain during initial attach.)
> + *
> + * Take note of the device disappearing, so we can ignore unmap
> request
> + * on stale domains (that is, between this detach and the upcoming
> + * free.)
> + *
> + * vdev->vdomain is protected by group->mutex
> + */
> + if (vdev->vdomain) {
> + dev_dbg(dev, "detach from domain %llu\n", vdev-
> >vdomain->id);
> + vdev->vdomain->attached--;
> + }
> +
> + dev_dbg(dev, "attach to domain %llu\n", vdomain->id);
> +
> + for (i = 0; i < fwspec->num_ids; i++) {
> + req.device = cpu_to_le32(fwspec->ids[i]);
> +
> + ret = viommu_send_req_sync(vdomain->viommu, &req);
> + if (ret)
> + break;
> + }
> +
> + vdomain->attached++;
> + vdev->vdomain = vdomain;
> +
> + return ret;
> +}
> +
> +static int viommu_map(struct iommu_domain *domain, unsigned long iova,
> + phys_addr_t paddr, size_t size, int prot) {
> + int ret;
> + struct viommu_domain *vdomain = to_viommu_domain(domain);
> + struct virtio_iommu_req_map req = {
> + .head.type = VIRTIO_IOMMU_T_MAP,
> + .address_space = cpu_to_le32(vdomain->id),
> + .virt_addr = cpu_to_le64(iova),
> + .phys_addr = cpu_to_le64(paddr),
> + .size = cpu_to_le64(size),
> + };
> +
> + pr_debug("map %llu 0x%lx -> 0x%llx (%zu)\n", vdomain->id, iova,
> + paddr, size);
A query, when I am tracing above prints I see same physical address is mapped with two different virtual address, do you know why kernel does this?
Thanks
-Bharat
> +
> + if (!vdomain->attached)
> + return -ENODEV;
> +
> + if (prot & IOMMU_READ)
> + req.flags |= cpu_to_le32(VIRTIO_IOMMU_MAP_F_READ);
> +
> + if (prot & IOMMU_WRITE)
> + req.flags |= cpu_to_le32(VIRTIO_IOMMU_MAP_F_WRITE);
> +
> + ret = viommu_tlb_map(vdomain, iova, paddr, size);
> + if (ret)
> + return ret;
> +
> + ret = viommu_send_req_sync(vdomain->viommu, &req);
> + if (ret)
> + viommu_tlb_unmap(vdomain, iova, size);
> +
> + return ret;
> +}
> +
> +static size_t viommu_unmap(struct iommu_domain *domain, unsigned
> long iova,
> + size_t size)
> +{
> + int ret;
> + size_t unmapped;
> + struct viommu_domain *vdomain = to_viommu_domain(domain);
> + struct virtio_iommu_req_unmap req = {
> + .head.type = VIRTIO_IOMMU_T_UNMAP,
> + .address_space = cpu_to_le32(vdomain->id),
> + .virt_addr = cpu_to_le64(iova),
> + };
> +
> + pr_debug("unmap %llu 0x%lx (%zu)\n", vdomain->id, iova, size);
> +
> + /* Callers may unmap after detach, but device already took care of it.
> */
> + if (!vdomain->attached)
> + return size;
> +
> + unmapped = viommu_tlb_unmap(vdomain, iova, size);
> + if (unmapped < size)
> + return 0;
> +
> + req.size = cpu_to_le64(unmapped);
> +
> + ret = viommu_send_req_sync(vdomain->viommu, &req);
> + if (ret)
> + return 0;
> +
> + return unmapped;
> +}
> +
> +static size_t viommu_map_sg(struct iommu_domain *domain, unsigned
> long iova,
> + struct scatterlist *sg, unsigned int nents, int prot) {
> + int i, ret;
> + int nr_sent;
> + size_t mapped;
> + size_t min_pagesz;
> + size_t total_size;
> + struct scatterlist *s;
> + unsigned int flags = 0;
> + unsigned long cur_iova;
> + unsigned long mapped_iova;
> + size_t head_size, tail_size;
> + struct viommu_request reqs[nents];
> + struct virtio_iommu_req_map map_reqs[nents];
> + struct viommu_domain *vdomain = to_viommu_domain(domain);
> +
> + if (!vdomain->attached)
> + return 0;
> +
> + pr_debug("map_sg %llu %u 0x%lx\n", vdomain->id, nents, iova);
> +
> + if (prot & IOMMU_READ)
> + flags |= VIRTIO_IOMMU_MAP_F_READ;
> +
> + if (prot & IOMMU_WRITE)
> + flags |= VIRTIO_IOMMU_MAP_F_WRITE;
> +
> + min_pagesz = 1 << __ffs(domain->pgsize_bitmap);
> + tail_size = sizeof(struct virtio_iommu_req_tail);
> + head_size = sizeof(*map_reqs) - tail_size;
> +
> + cur_iova = iova;
> +
> + for_each_sg(sg, s, nents, i) {
> + size_t size = s->length;
> + phys_addr_t paddr = sg_phys(s);
> + void *tail = (void *)&map_reqs[i] + head_size;
> +
> + if (!IS_ALIGNED(paddr | size, min_pagesz)) {
> + ret = -EFAULT;
> + break;
> + }
> +
> + /* TODO: merge physically-contiguous mappings if any */
> + map_reqs[i] = (struct virtio_iommu_req_map) {
> + .head.type = VIRTIO_IOMMU_T_MAP,
> + .address_space = cpu_to_le32(vdomain->id),
> + .flags = cpu_to_le32(flags),
> + .virt_addr = cpu_to_le64(cur_iova),
> + .phys_addr = cpu_to_le64(paddr),
> + .size = cpu_to_le64(size),
> + };
> +
> + ret = viommu_tlb_map(vdomain, cur_iova, paddr, size);
> + if (ret)
> + break;
> +
> + sg_init_one(&reqs[i].head, &map_reqs[i], head_size);
> + sg_init_one(&reqs[i].tail, tail, tail_size);
> +
> + cur_iova += size;
> + }
> +
> + total_size = cur_iova - iova;
> +
> + if (ret) {
> + viommu_tlb_unmap(vdomain, iova, total_size);
> + return 0;
> + }
> +
> + ret = viommu_send_reqs_sync(vdomain->viommu, reqs, i,
> &nr_sent);
> +
> + if (nr_sent != nents)
> + goto err_rollback;
> +
> + for (i = 0; i < nents; i++) {
> + if (!reqs[i].written || map_reqs[i].tail.status)
> + goto err_rollback;
> + }
> +
> + return total_size;
> +
> +err_rollback:
> + /*
> + * Any request in the range might have failed. Unmap what was
> + * successful.
> + */
> + cur_iova = iova;
> + mapped_iova = iova;
> + mapped = 0;
> + for_each_sg(sg, s, nents, i) {
> + size_t size = s->length;
> +
> + cur_iova += size;
> +
> + if (!reqs[i].written || map_reqs[i].tail.status) {
> + if (mapped)
> + viommu_unmap(domain, mapped_iova,
> mapped);
> +
> + mapped_iova = cur_iova;
> + mapped = 0;
> + } else {
> + mapped += size;
> + }
> + }
> +
> + viommu_tlb_unmap(vdomain, iova, total_size);
> +
> + return 0;
> +}
> +
> +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);
> +
> + pr_debug("iova_to_phys %llu 0x%llx->0x%llx\n", vdomain->id, iova,
> + paddr);
> +
> + return paddr;
> +}
> +
> +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) {
> + 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;
> +
> + /*
> + * Last step creates a default domain and attaches to it. Everything
> + * must be ready.
> + */
> + group = iommu_group_get_for_dev(dev);
> +
> + return PTR_ERR_OR_ZERO(group);
> +}
> +
> +static void viommu_remove_device(struct device *dev) {
> + kfree(dev->iommu_fwspec->iommu_priv);
> +}
> +
> +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) {
> + u32 *id = args->args;
> +
> + dev_dbg(dev, "of_xlate 0x%x\n", *id);
> + return iommu_fwspec_add_ids(dev, args->args, 1); }
> +
> +/*
> + * (Maybe) temporary hack for device pass-through into guest userspace.
> +On ARM
> + * with an ITS, VFIO will look for a region where to map the doorbell,
> +even
> + * though the virtual doorbell is never written to by the device, and
> +instead
> + * the host injects interrupts directly. TODO: sort this out in VFIO.
> + */
> +#define MSI_IOVA_BASE 0x8000000
> +#define MSI_IOVA_LENGTH 0x100000
> +
> +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_MSI);
> + if (!region)
> + return;
> +
> + list_add_tail(®ion->list, 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 = {
> + .capable = viommu_capable,
> + .domain_alloc = viommu_domain_alloc,
> + .domain_free = viommu_domain_free,
> + .attach_dev = viommu_attach_dev,
> + .map = viommu_map,
> + .unmap = viommu_unmap,
> + .map_sg = viommu_map_sg,
> + .iova_to_phys = viommu_iova_to_phys,
> + .add_device = viommu_add_device,
> + .remove_device = viommu_remove_device,
> + .device_group = viommu_device_group,
> + .of_xlate = viommu_of_xlate,
> + .get_resv_regions = viommu_get_resv_regions,
> + .put_resv_regions = viommu_put_resv_regions,
> +};
> +
> +static int viommu_init_vq(struct viommu_dev *viommu) {
> + struct virtio_device *vdev = dev_to_virtio(viommu->dev);
> + vq_callback_t *callback = NULL;
> + const char *name = "request";
> + int ret;
> +
> + ret = vdev->config->find_vqs(vdev, 1, &viommu->vq, &callback,
> + &name, NULL);
> + if (ret)
> + dev_err(viommu->dev, "cannot find VQ\n");
> +
> + return ret;
> +}
> +
> +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;
> + int ret;
> +
> + viommu = kzalloc(sizeof(*viommu), GFP_KERNEL);
> + if (!viommu)
> + return -ENOMEM;
> +
> + spin_lock_init(&viommu->vq_lock);
> + INIT_LIST_HEAD(&viommu->pending_requests);
> + viommu->dev = dev;
> + viommu->vdev = vdev;
> +
> + ret = viommu_init_vq(viommu);
> + if (ret)
> + goto err_free_viommu;
> +
> + virtio_cread(vdev, struct virtio_iommu_config, page_sizes,
> + &viommu->pgsize_bitmap);
> +
> + viommu->aperture_end = -1UL;
> +
> + virtio_cread_feature(vdev, VIRTIO_IOMMU_F_INPUT_RANGE,
> + struct virtio_iommu_config, input_range.start,
> + &viommu->aperture_start);
> +
> + virtio_cread_feature(vdev, VIRTIO_IOMMU_F_INPUT_RANGE,
> + struct virtio_iommu_config, input_range.end,
> + &viommu->aperture_end);
> +
> + if (!viommu->pgsize_bitmap) {
> + ret = -EINVAL;
> + goto err_free_viommu;
> + }
> +
> + viommu_ops.pgsize_bitmap = viommu->pgsize_bitmap;
> +
> + /*
> + * Not strictly necessary, virtio would enable it later. This allows to
> + * start using the request queue early.
> + */
> + virtio_device_ready(vdev);
> +
> + ret = iommu_device_sysfs_add(&viommu->iommu, dev, NULL, "%s",
> + virtio_bus_name(vdev));
> + if (ret)
> + goto err_free_viommu;
> +
> + 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(viommu->dev, "probe successful\n");
> +
> + return 0;
> +
> +err_unregister:
> + iommu_device_unregister(&viommu->iommu);
> +
> +err_free_viommu:
> + kfree(viommu);
> +
> + return ret;
> +}
> +
> +static void viommu_remove(struct virtio_device *vdev) {
> + struct viommu_dev *viommu = vdev->priv;
> +
> + iommu_device_unregister(&viommu->iommu);
> + kfree(viommu);
> +
> + 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_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", NULL);
> +
> +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/Kbuild b/include/uapi/linux/Kbuild index
> 1f25c86374ad..c0cb0f173258 100644
> --- a/include/uapi/linux/Kbuild
> +++ b/include/uapi/linux/Kbuild
> @@ -467,6 +467,7 @@ header-y += virtio_console.h header-y +=
> virtio_gpu.h header-y += virtio_ids.h header-y += virtio_input.h
> +header-y += virtio_iommu.h
> header-y += virtio_mmio.h
> header-y += virtio_net.h
> header-y += virtio_pci.h
> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> index 6d5c3b2d4f4d..934ed3d3cd3f 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 61216 /* virtio IOMMU (temporary) */
>
> #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..ec74c9a727d4
> --- /dev/null
> +++ b/include/uapi/linux/virtio_iommu.h
> @@ -0,0 +1,142 @@
> +/*
> + * Copyright (C) 2017 ARM Ltd.
> + *
> + * This header is BSD licensed so anyone can use the definitions
> + * to implement compatible drivers/servers:
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of ARM Ltd. nor the names of its contributors
> + * may be used to endorse or promote products derived from this
> software
> + * without specific prior written permission.
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> CONTRIBUTORS
> + * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT
> NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
> FITNESS
> + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL IBM
> OR
> + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS
> OF
> + * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
> CAUSED AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
> + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> OUT
> + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +#ifndef _UAPI_LINUX_VIRTIO_IOMMU_H
> +#define _UAPI_LINUX_VIRTIO_IOMMU_H
> +
> +/* Feature bits */
> +#define VIRTIO_IOMMU_F_INPUT_RANGE 0
> +#define VIRTIO_IOMMU_F_IOASID_BITS 1
> +#define VIRTIO_IOMMU_F_MAP_UNMAP 2
> +#define VIRTIO_IOMMU_F_BYPASS 3
> +
> +__packed
> +struct virtio_iommu_config {
> + /* Supported page sizes */
> + __u64 page_sizes;
> + struct virtio_iommu_range {
> + __u64 start;
> + __u64 end;
> + } input_range;
> + __u8 ioasid_bits;
> +};
> +
> +/* 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
> +
> +__packed
> +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 address_space;
> + __le32 device;
> + __le32 reserved;
> +
> + struct virtio_iommu_req_tail tail;
> +};
> +
> +__packed
> +struct virtio_iommu_req_detach {
> + struct virtio_iommu_req_head head;
> +
> + __le32 device;
> + __le32 reserved;
> +
> + struct virtio_iommu_req_tail tail;
> +};
> +
> +#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_MASK
> (VIRTIO_IOMMU_MAP_F_READ | \
> +
> VIRTIO_IOMMU_MAP_F_WRITE | \
> +
> VIRTIO_IOMMU_MAP_F_EXEC)
> +
> +__packed
> +struct virtio_iommu_req_map {
> + struct virtio_iommu_req_head head;
> +
> + __le32 address_space;
> + __le32 flags;
> + __le64 virt_addr;
> + __le64 phys_addr;
> + __le64 size;
> +
> + struct virtio_iommu_req_tail tail;
> +};
> +
> +__packed
> +struct virtio_iommu_req_unmap {
> + struct virtio_iommu_req_head head;
> +
> + __le32 address_space;
> + __le32 flags;
> + __le64 virt_addr;
> + __le64 size;
> +
> + struct virtio_iommu_req_tail tail;
> +};
> +
> +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.12.1
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH v11 3/6] virtio-balloon: VIRTIO_BALLOON_F_PAGE_CHUNKS
From: Michael S. Tsirkin @ 2017-06-16 3:19 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, kvm, qemu-devel, amit.shah,
liliang.opensource, dave.hansen, linux-kernel, Matthew Wilcox,
virtualization, linux-mm, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <594240E9.2070705@intel.com>
On Thu, Jun 15, 2017 at 04:10:17PM +0800, Wei Wang wrote:
> On 06/14/2017 01:56 AM, Michael S. Tsirkin wrote:
> > On Fri, Jun 09, 2017 at 06:41:38PM +0800, Wei Wang wrote:
> > > Add a new feature, VIRTIO_BALLOON_F_PAGE_CHUNKS, which enables
> > > the transfer of the ballooned (i.e. inflated/deflated) pages in
> > > chunks to the host.
> > so now these chunks are just s/g list entry.
> > So let's rename this VIRTIO_BALLOON_F_SG with a comment:
> > * Use standard virtio s/g instead of PFN lists *
>
> Actually, it's not using the standard s/g list in the implementation,
> because:
> using the standard s/g will need kmalloc() the indirect table on
> demand (i.e. when virtqueue_add() converts s/g to indirect table);
>
> The implementation directly pre-allocates an indirect desc table,
> and uses a entry (i.e. vring_desc) to describe a chunk. This
> avoids the overhead of kmalloc() the indirect table.
It's a separate API but the host/guest interface is standard.
>
> > > +/*
> > > + * Callulates how many pfns can a page_bmap record. A bit corresponds to a
> > > + * page of PAGE_SIZE.
> > > + */
> > > +#define VIRTIO_BALLOON_PFNS_PER_PAGE_BMAP \
> > > + (VIRTIO_BALLOON_PAGE_BMAP_SIZE * BITS_PER_BYTE)
> > > +
> > > +/* The number of page_bmap to allocate by default. */
> > > +#define VIRTIO_BALLOON_PAGE_BMAP_DEFAULT_NUM 1
> > It's not by default, it's at probe time, right?
> It is the number of page bitmap being kept throughout the whole
> lifecycle of the driver. The page bmap will be temporarily extended
> due to insufficiency during a ballooning process, but when that
> ballooning finishes, the extended part will be freed.
> > > +/* The maximum number of page_bmap that can be allocated. */
> > Not really, this is the size of the array we use to keep them.
>
> This is the max number of the page bmap that can be
> extended temporarily.
That's just a confusing way to say the same.
> > > +#define VIRTIO_BALLOON_PAGE_BMAP_MAX_NUM 32
> > > +
> > So you still have a home-grown bitmap. I'd like to know why
> > isn't xbitmap suggested for this purpose by Matthew Wilcox
> > appropriate. Please add a comment explaining the requirements
> > from the data structure.
>
> I didn't find his xbitmap being upstreamed, did you?
It's from dax tree - Matthew?
> > > +/*
> > > + * QEMU virtio implementation requires the desc table size less than
> > > + * VIRTQUEUE_MAX_SIZE, so minus 1 here.
> > I think it doesn't, the issue is probably that you add a header
> > as a separate s/g. In any case see below.
> >
> > > + */
> > > +#define VIRTIO_BALLOON_MAX_PAGE_CHUNKS (VIRTQUEUE_MAX_SIZE - 1)
> > This is wrong, virtio spec says s/g size should not exceed VQ size.
> > If you want to support huge VQ sizes, you can add a fallback to
> > smaller sizes until it fits in 1 page.
>
> Probably no need for huge VQ size, 1024 queue size should be
> enough. And we can have 1024 descriptors in the indirect
> table, so the above size doesn't exceed the vq size, right?
You need to look at vq size, you shouldn't assume it's > 1024.
>
> > +static unsigned int extend_page_bmap_size(struct virtio_balloon *vb,
> > + unsigned long pfn_num)
> > what's this API doing? Pls add comments. this seems to assume
> > it will only be called once.
> OK, I will add some comments here. This is the function to extend
> the number of page bitmap when the original 1 page bmap is
> not sufficient during a ballooning process. As mentioned above,
> at the end of this ballooning process, the extended part will be freed.
>
> > it would be better to avoid making
> > this assumption, just look at what has been allocated
> > and extend it.
> Actually it's not an assumption. The rule here is that we always keep
> "1" page bmap. "1" is defined by the
> VIRTIO_BALLOON_PAGE_BMAP_DEFAULT_NUM. So when freeing, it also
> references VIRTIO_BALLOON_PAGE_BMAP_DEFAULT_NUM (not assuming
> any number)
When allocating, why don't you check what's allocated already?
why assume VIRTIO_BALLOON_PAGE_BMAP_DEFAULT_NUM was allocated?
Then calling extend_page_bmap_size many times would be idempotent.
> > +}
> > +
> > +/* Add a chunk to the buffer. */
> > +static void add_one_chunk(struct virtio_balloon *vb, struct virtqueue *vq,
> > + u64 base_addr, u32 size)
> > +{
> > + unsigned int *num = &vb->balloon_page_chunk.chunk_num;
> > + struct vring_desc *desc = &vb->balloon_page_chunk.desc_table[*num];
> > +
> > + desc->addr = cpu_to_virtio64(vb->vdev, base_addr);
> > + desc->len = cpu_to_virtio32(vb->vdev, size);
> > + *num += 1;
> > + if (*num == VIRTIO_BALLOON_MAX_PAGE_CHUNKS)
> > + send_page_chunks(vb, vq);
> > +}
> > +
> > Poking at virtio internals like this is not nice. Pls move to virtio
> > code. Also, pages must be read descriptors as host might modify them.
> >
> > This also lacks viommu support but this is not mandatory as
> > that is borken atm anyway. I'll send a patch to at least fail cleanly.
> OK, thanks.
>
> > > +static void convert_bmap_to_chunks(struct virtio_balloon *vb,
> > > + struct virtqueue *vq,
> > > + unsigned long *bmap,
> > > + unsigned long pfn_start,
> > > + unsigned long size)
> > > +{
> > > + unsigned long next_one, next_zero, pos = 0;
> > > + u64 chunk_base_addr;
> > > + u32 chunk_size;
> > > +
> > > + while (pos < size) {
> > > + next_one = find_next_bit(bmap, size, pos);
> > > + /*
> > > + * No "1" bit found, which means that there is no pfn
> > > + * recorded in the rest of this bmap.
> > > + */
> > > + if (next_one == size)
> > > + break;
> > > + next_zero = find_next_zero_bit(bmap, size, next_one + 1);
> > > + /*
> > > + * A bit in page_bmap corresponds to a page of PAGE_SIZE.
> > > + * Convert it to be pages of 4KB balloon page size when
> > > + * adding it to a chunk.
> > This looks wrong. add_one_chunk assumes size in bytes. So should be just
> > PAGE_SIZE.
>
> It's intended to be "chunk size", which is the number of pfns. The benefit
> is
> that the 32-bit desc->len won't be overflow, as you mentioned below.
>
You can safely assume PAGE_SIZE >= 4K. Just pass # of pages.
> >
> > > + */
> > > + chunk_size = (next_zero - next_one) *
> > > + VIRTIO_BALLOON_PAGES_PER_PAGE;
> > How do you know this won't overflow a 32 bit integer? Needs a comment.
>
> If it stores size in bytes, it has the possibility to overflow.
> If storing number of pfns, the 32-bit value can support 2^32*4KB=8TB
> memory, unlikely to overflow.
Do you put in len in the descriptor in 4K chunks then? That
needs some thought. Also, processors support up to 256TB now,
I don't think we can just assume it won't overflow anymore.
All in all I'd prefer we just split everything up to 2G chunks.
We can discuss extending len to more bits or specifying alignment
in the descriptor separately down the road.
> > +
> > +static int balloon_page_chunk_init(struct virtio_balloon *vb)
> > +{
> > + int i;
> > +
> > + vb->balloon_page_chunk.desc_table = alloc_indirect(vb->vdev,
> > + VIRTIO_BALLOON_MAX_PAGE_CHUNKS,
> > + GFP_KERNEL);
> > This one's problematic, you aren't supposed to use APIs when device
> > is not inited yet. Seems to work by luck here. I suggest moving
> > this to probe, that's where we do a bunch of inits.
> > And then you can move private init back to allocate too.
>
> This is just to allocate an indirect desc table. If allocation fails, we
> need to clear
> the related feature bit in ->validate(), right?
Failing probe on OOM is ok too.
--
MST
^ permalink raw reply
* Re: [PATCH] virtio_balloon: disable VIOMMU support
From: Jason Wang @ 2017-06-15 9:15 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel; +Cc: stable, virtualization
In-Reply-To: <1497376825-15502-1-git-send-email-mst@redhat.com>
On 2017年06月14日 02:00, Michael S. Tsirkin wrote:
> virtio balloon bypasses the DMA API entirely so does not support the
> VIOMMU right now. It's not clear we need that support, for now let's
> just make sure we don't pretend to support it.
>
> Cc: stable@vger.kernel.org
> Cc: Wei Wang <wei.w.wang@intel.com>
> Fixes: 1a937693993f ("virtio: new feature to detect IOMMU device quirk")
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/virtio/virtio_balloon.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 408c174..22caf80 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -663,6 +663,12 @@ static int virtballoon_restore(struct virtio_device *vdev)
> }
> #endif
>
> +static int virtballoon_validate(struct virtio_device *vdev)
> +{
> + __virtio_clear_bit(vdev, VIRTIO_F_IOMMU_PLATFORM);
> + return 0;
> +}
> +
> static unsigned int features[] = {
> VIRTIO_BALLOON_F_MUST_TELL_HOST,
> VIRTIO_BALLOON_F_STATS_VQ,
> @@ -675,6 +681,7 @@ static struct virtio_driver virtio_balloon_driver = {
> .driver.name = KBUILD_MODNAME,
> .driver.owner = THIS_MODULE,
> .id_table = id_table,
> + .validate = virtballoon_validate,
> .probe = virtballoon_probe,
> .remove = virtballoon_remove,
> .config_changed = virtballoon_changed,
Acked-by: Jason Wang <jasowang@redhat.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH v11 3/6] virtio-balloon: VIRTIO_BALLOON_F_PAGE_CHUNKS
From: Wei Wang @ 2017-06-15 8:10 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aarcange, virtio-dev, kvm, qemu-devel, amit.shah,
liliang.opensource, dave.hansen, linux-kernel, Matthew Wilcox,
virtualization, linux-mm, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <20170613200049-mutt-send-email-mst@kernel.org>
On 06/14/2017 01:56 AM, Michael S. Tsirkin wrote:
> On Fri, Jun 09, 2017 at 06:41:38PM +0800, Wei Wang wrote:
>> Add a new feature, VIRTIO_BALLOON_F_PAGE_CHUNKS, which enables
>> the transfer of the ballooned (i.e. inflated/deflated) pages in
>> chunks to the host.
> so now these chunks are just s/g list entry.
> So let's rename this VIRTIO_BALLOON_F_SG with a comment:
> * Use standard virtio s/g instead of PFN lists *
Actually, it's not using the standard s/g list in the implementation,
because:
using the standard s/g will need kmalloc() the indirect table on
demand (i.e. when virtqueue_add() converts s/g to indirect table);
The implementation directly pre-allocates an indirect desc table,
and uses a entry (i.e. vring_desc) to describe a chunk. This
avoids the overhead of kmalloc() the indirect table.
>> +/*
>> + * Callulates how many pfns can a page_bmap record. A bit corresponds to a
>> + * page of PAGE_SIZE.
>> + */
>> +#define VIRTIO_BALLOON_PFNS_PER_PAGE_BMAP \
>> + (VIRTIO_BALLOON_PAGE_BMAP_SIZE * BITS_PER_BYTE)
>> +
>> +/* The number of page_bmap to allocate by default. */
>> +#define VIRTIO_BALLOON_PAGE_BMAP_DEFAULT_NUM 1
> It's not by default, it's at probe time, right?
It is the number of page bitmap being kept throughout the whole
lifecycle of the driver. The page bmap will be temporarily extended
due to insufficiency during a ballooning process, but when that
ballooning finishes, the extended part will be freed.
>> +/* The maximum number of page_bmap that can be allocated. */
> Not really, this is the size of the array we use to keep them.
This is the max number of the page bmap that can be
extended temporarily.
>> +#define VIRTIO_BALLOON_PAGE_BMAP_MAX_NUM 32
>> +
> So you still have a home-grown bitmap. I'd like to know why
> isn't xbitmap suggested for this purpose by Matthew Wilcox
> appropriate. Please add a comment explaining the requirements
> from the data structure.
I didn't find his xbitmap being upstreamed, did you?
>> +/*
>> + * QEMU virtio implementation requires the desc table size less than
>> + * VIRTQUEUE_MAX_SIZE, so minus 1 here.
> I think it doesn't, the issue is probably that you add a header
> as a separate s/g. In any case see below.
>
>> + */
>> +#define VIRTIO_BALLOON_MAX_PAGE_CHUNKS (VIRTQUEUE_MAX_SIZE - 1)
> This is wrong, virtio spec says s/g size should not exceed VQ size.
> If you want to support huge VQ sizes, you can add a fallback to
> smaller sizes until it fits in 1 page.
Probably no need for huge VQ size, 1024 queue size should be
enough. And we can have 1024 descriptors in the indirect
table, so the above size doesn't exceed the vq size, right?
> +static unsigned int extend_page_bmap_size(struct virtio_balloon *vb,
> + unsigned long pfn_num)
> what's this API doing? Pls add comments. this seems to assume
> it will only be called once.
OK, I will add some comments here. This is the function to extend
the number of page bitmap when the original 1 page bmap is
not sufficient during a ballooning process. As mentioned above,
at the end of this ballooning process, the extended part will be freed.
> it would be better to avoid making
> this assumption, just look at what has been allocated
> and extend it.
Actually it's not an assumption. The rule here is that we always keep
"1" page bmap. "1" is defined by the
VIRTIO_BALLOON_PAGE_BMAP_DEFAULT_NUM. So when freeing, it also
references VIRTIO_BALLOON_PAGE_BMAP_DEFAULT_NUM (not assuming
any number)
> +}
> +
> +/* Add a chunk to the buffer. */
> +static void add_one_chunk(struct virtio_balloon *vb, struct virtqueue *vq,
> + u64 base_addr, u32 size)
> +{
> + unsigned int *num = &vb->balloon_page_chunk.chunk_num;
> + struct vring_desc *desc = &vb->balloon_page_chunk.desc_table[*num];
> +
> + desc->addr = cpu_to_virtio64(vb->vdev, base_addr);
> + desc->len = cpu_to_virtio32(vb->vdev, size);
> + *num += 1;
> + if (*num == VIRTIO_BALLOON_MAX_PAGE_CHUNKS)
> + send_page_chunks(vb, vq);
> +}
> +
> Poking at virtio internals like this is not nice. Pls move to virtio
> code. Also, pages must be read descriptors as host might modify them.
>
> This also lacks viommu support but this is not mandatory as
> that is borken atm anyway. I'll send a patch to at least fail cleanly.
OK, thanks.
>> +static void convert_bmap_to_chunks(struct virtio_balloon *vb,
>> + struct virtqueue *vq,
>> + unsigned long *bmap,
>> + unsigned long pfn_start,
>> + unsigned long size)
>> +{
>> + unsigned long next_one, next_zero, pos = 0;
>> + u64 chunk_base_addr;
>> + u32 chunk_size;
>> +
>> + while (pos < size) {
>> + next_one = find_next_bit(bmap, size, pos);
>> + /*
>> + * No "1" bit found, which means that there is no pfn
>> + * recorded in the rest of this bmap.
>> + */
>> + if (next_one == size)
>> + break;
>> + next_zero = find_next_zero_bit(bmap, size, next_one + 1);
>> + /*
>> + * A bit in page_bmap corresponds to a page of PAGE_SIZE.
>> + * Convert it to be pages of 4KB balloon page size when
>> + * adding it to a chunk.
> This looks wrong. add_one_chunk assumes size in bytes. So should be just
> PAGE_SIZE.
It's intended to be "chunk size", which is the number of pfns. The
benefit is
that the 32-bit desc->len won't be overflow, as you mentioned below.
>
>> + */
>> + chunk_size = (next_zero - next_one) *
>> + VIRTIO_BALLOON_PAGES_PER_PAGE;
> How do you know this won't overflow a 32 bit integer? Needs a comment.
If it stores size in bytes, it has the possibility to overflow.
If storing number of pfns, the 32-bit value can support 2^32*4KB=8TB
memory, unlikely to overflow.
> +
> +static int balloon_page_chunk_init(struct virtio_balloon *vb)
> +{
> + int i;
> +
> + vb->balloon_page_chunk.desc_table = alloc_indirect(vb->vdev,
> + VIRTIO_BALLOON_MAX_PAGE_CHUNKS,
> + GFP_KERNEL);
> This one's problematic, you aren't supposed to use APIs when device
> is not inited yet. Seems to work by luck here. I suggest moving
> this to probe, that's where we do a bunch of inits.
> And then you can move private init back to allocate too.
This is just to allocate an indirect desc table. If allocation fails, we
need to clear
the related feature bit in ->validate(), right?
^ permalink raw reply
* Call for papers \x1f--- ICITS'18 --- Ecuador
From: Marle @ 2017-06-14 14:36 UTC (permalink / raw)
To: virtualization
[-- Attachment #1: Type: text/plain, Size: 5191 bytes --]
---------------------------------------------------------------------------------------------------
ICITS'18 - The 2018 International Conference on Information Technology & Systems
Peninsula de Santa Elena, Ecuador, 10 - 12 January 2018
http://www.icits.me/
-------------------------------------------------------------------------------------------------
SCOPE
ICITS'18 - The 2018 International Conference on Information Technology & Systems (http://www.icits.me/), to be held at Península de Santa Elena, Ecuador, 10 - 12 January 2018, is an international forum for researchers and practitioners to present and discuss the most recent innovations, trends, results, experiences and concerns in the several perspectives of Information Technology & Systems.
We are pleased to invite you to submit your papers to ICITS'18. They can be written in English, Spanish or Portuguese. All submissions will be reviewed on the basis of relevance, originality, importance and clarity.
TOPICS
Submitted papers should be related with one or more of the main themes proposed for the Conference:
A) Information and Knowledge Management (IKM);
B) Organizational Models and Information Systems (OMIS);
C) Software and Systems Modeling (SSM);
D) Software Systems, Architectures, Applications and Tools (SSAAT);
E) Multimedia Systems and Applications (MSA);
F) Computer Networks, Mobility and Pervasive Systems (CNMPS);
G) Intelligent and Decision Support Systems (IDSS);
H) Big Data Analytics and Applications (BDAA);
I) Human-Computer Interaction (HCI);
J) Ethics, Computers and Security (ECS)
K) Health Informatics (HIS);
L) Information Technologies in Education (ITE);
SUBMISSION AND DECISION
Submitted papers (until 10-page limit) must comply with the format of Advances in Intelligent Systems and Computing series (see Instructions for Authors at Springer Website or download a DOC example) be written in English, Spanish or Portuguese, must not have been published before, not be under review for any other conference or publication and not include any information leading to the authors’ identification. Therefore, the authors’ names, affiliations and bibliographic references should not be included in the version for evaluation by the Scientific Committee. This information should only be included in the camera-ready version, saved in Word or Latex format and also in PDF format. These files must be accompanied by the Consent to Publication form filled out, in a ZIP file, and uploaded at the conference management system.
All papers will be subjected to a “double-blind review” by at least two members of the Scientific Committee.
Based on Scientific Committee evaluation, a paper can be rejected or accepted by the Conference Chairs. In the later case, it can be accepted as paper or poster.
The authors of papers accepted as posters must build and print a poster to be exhibited during the Conference. This poster must follow an A1 or A2 vertical format. The Conference can includes Work Sessions where these posters are presented and orally discussed, with a 7 minute limit per poster.
The authors of accepted papers will have 15 minutes to present their work in a Conference Work Session; approximately 5 minutes of discussion will follow each presentation.
PUBLICATION AND INDEXING
To ensure that an accepted paper is published, at least one of the authors must be fully registered by the 20th of October 2017, and the paper must comply with the suggested layout and page-limit (until 10 pages). Additionally, all recommended changes must be addressed by the authors before they submit the camera-ready version.
No more than one paper per registration will be published. An extra fee must be paid for publication of additional papers, with a maximum of one additional paper per registration. One registration permits only the participation of one author in the conference.
Papers written in English and accepted and registered will be published in Proceedings by Springer, in a book of the Advances in Intelligent Systems and Computing series, will be submitted for indexation by ISI, EI-Compendex, SCOPUS and DBLP, among others, and will be available in the SpringerLink Digital Library.
Papers written in Spanish or Portuguese and accepted and registered will be published in Proceedings by AISTI and will be submitted for evaluation and possible indexation by ISI, EI-Compendex and SCOPUS.
The authors of the best selected papers will be invited to extend them for publication in international journals indexed by ISI, SCOPUS, EI-Compendex and DBLP, among others.
IMPORTANT DATES
Paper Submission: September 6, 2017
Notification of Acceptance: October 13, 2017
Payment of Registration, to ensure the inclusion of an accepted paper in the conference proceedings: October 20, 2017.
Camera-ready Submission: October 20, 2017
---------
ICITS'18 website: http://www.icits.me/
---------
----------------------------------------------
---
This email has been checked for viruses by AVG.
http://www.avg.com
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v11 3/6] virtio-balloon: VIRTIO_BALLOON_F_PAGE_CHUNKS
From: Michael S. Tsirkin @ 2017-06-13 18:55 UTC (permalink / raw)
To: Dave Hansen
Cc: aarcange, virtio-dev, kvm, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, Matthew Wilcox, virtualization,
linux-mm, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <fb67359c-3d19-f67f-ec47-3cf868b8d9e8@intel.com>
On Tue, Jun 13, 2017 at 10:59:07AM -0700, Dave Hansen wrote:
> On 06/13/2017 10:56 AM, Michael S. Tsirkin wrote:
> >> +/* The size of one page_bmap used to record inflated/deflated pages. */
> >> +#define VIRTIO_BALLOON_PAGE_BMAP_SIZE (8 * PAGE_SIZE)
> > At this size, you probably want alloc_pages to avoid kmalloc
> > overhead.
>
> For slub, at least, kmalloc() just calls alloc_pages() basically
> directly. There's virtually no overhead.
>
>
OK then.
--
MST
^ permalink raw reply
* [PATCH] virtio_balloon: disable VIOMMU support
From: Michael S. Tsirkin @ 2017-06-13 18:00 UTC (permalink / raw)
To: linux-kernel; +Cc: stable, virtualization
virtio balloon bypasses the DMA API entirely so does not support the
VIOMMU right now. It's not clear we need that support, for now let's
just make sure we don't pretend to support it.
Cc: stable@vger.kernel.org
Cc: Wei Wang <wei.w.wang@intel.com>
Fixes: 1a937693993f ("virtio: new feature to detect IOMMU device quirk")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/virtio/virtio_balloon.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 408c174..22caf80 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -663,6 +663,12 @@ static int virtballoon_restore(struct virtio_device *vdev)
}
#endif
+static int virtballoon_validate(struct virtio_device *vdev)
+{
+ __virtio_clear_bit(vdev, VIRTIO_F_IOMMU_PLATFORM);
+ return 0;
+}
+
static unsigned int features[] = {
VIRTIO_BALLOON_F_MUST_TELL_HOST,
VIRTIO_BALLOON_F_STATS_VQ,
@@ -675,6 +681,7 @@ static struct virtio_driver virtio_balloon_driver = {
.driver.name = KBUILD_MODNAME,
.driver.owner = THIS_MODULE,
.id_table = id_table,
+ .validate = virtballoon_validate,
.probe = virtballoon_probe,
.remove = virtballoon_remove,
.config_changed = virtballoon_changed,
--
MST
^ permalink raw reply related
* Re: [PATCH v11 3/6] virtio-balloon: VIRTIO_BALLOON_F_PAGE_CHUNKS
From: Dave Hansen @ 2017-06-13 17:59 UTC (permalink / raw)
To: Michael S. Tsirkin, Wei Wang
Cc: aarcange, virtio-dev, kvm, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, Matthew Wilcox, virtualization,
linux-mm, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <20170613200049-mutt-send-email-mst@kernel.org>
On 06/13/2017 10:56 AM, Michael S. Tsirkin wrote:
>> +/* The size of one page_bmap used to record inflated/deflated pages. */
>> +#define VIRTIO_BALLOON_PAGE_BMAP_SIZE (8 * PAGE_SIZE)
> At this size, you probably want alloc_pages to avoid kmalloc
> overhead.
For slub, at least, kmalloc() just calls alloc_pages() basically
directly. There's virtually no overhead.
^ permalink raw reply
* Re: [PATCH v11 3/6] virtio-balloon: VIRTIO_BALLOON_F_PAGE_CHUNKS
From: Michael S. Tsirkin @ 2017-06-13 17:56 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, kvm, qemu-devel, amit.shah,
liliang.opensource, dave.hansen, linux-kernel, Matthew Wilcox,
virtualization, linux-mm, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <1497004901-30593-4-git-send-email-wei.w.wang@intel.com>
On Fri, Jun 09, 2017 at 06:41:38PM +0800, Wei Wang wrote:
> Add a new feature, VIRTIO_BALLOON_F_PAGE_CHUNKS, which enables
> the transfer of the ballooned (i.e. inflated/deflated) pages in
> chunks to the host.
so now these chunks are just s/g list entry.
So let's rename this VIRTIO_BALLOON_F_SG with a comment:
* Use standard virtio s/g instead of PFN lists *
> The implementation of the previous virtio-balloon is not very
> efficient, because the ballooned pages are transferred to the
> host one by one. Here is the breakdown of the time in percentage
> spent on each step of the balloon inflating process (inflating
> 7GB of an 8GB idle guest).
>
> 1) allocating pages (6.5%)
> 2) sending PFNs to host (68.3%)
> 3) address translation (6.1%)
> 4) madvise (19%)
>
> It takes about 4126ms for the inflating process to complete.
> The above profiling shows that the bottlenecks are stage 2)
> and stage 4).
>
> This patch optimizes step 2) by transferring pages to the host in
> chunks. A chunk consists of guest physically continuous pages.
> When the pages are packed into a chunk, they are converted into
> balloon page size (4KB) pages. A chunk is offered to the host
> via a base address (i.e. the start guest physical address of those
> physically continuous pages) and the size (i.e. the total number
> of the 4KB balloon size pages). A chunk is described via a
> vring_desc struct in the implementation.
>
> By doing so, step 4) can also be optimized by doing address
> translation and madvise() in chunks rather than page by page.
>
> With this new feature, the above ballooning process takes ~590ms
> resulting in an improvement of ~85%.
>
> TODO: optimize stage 1) by allocating/freeing a chunk of pages
> instead of a single page each time.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/virtio/virtio_balloon.c | 418 +++++++++++++++++++++++++++++++++---
> drivers/virtio/virtio_ring.c | 120 ++++++++++-
> include/linux/virtio.h | 7 +
> include/uapi/linux/virtio_balloon.h | 1 +
> include/uapi/linux/virtio_ring.h | 3 +
> 5 files changed, 517 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index ecb64e9..0cf945c 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -51,6 +51,36 @@ MODULE_PARM_DESC(oom_pages, "pages to free on OOM");
> static struct vfsmount *balloon_mnt;
> #endif
>
> +/* The size of one page_bmap used to record inflated/deflated pages. */
> +#define VIRTIO_BALLOON_PAGE_BMAP_SIZE (8 * PAGE_SIZE)
At this size, you probably want alloc_pages to avoid kmalloc
overhead.
> +/*
> + * Callulates how many pfns can a page_bmap record. A bit corresponds to a
> + * page of PAGE_SIZE.
> + */
> +#define VIRTIO_BALLOON_PFNS_PER_PAGE_BMAP \
> + (VIRTIO_BALLOON_PAGE_BMAP_SIZE * BITS_PER_BYTE)
> +
> +/* The number of page_bmap to allocate by default. */
> +#define VIRTIO_BALLOON_PAGE_BMAP_DEFAULT_NUM 1
It's not by default, it's at probe time, right?
> +/* The maximum number of page_bmap that can be allocated. */
Not really, this is the size of the array we use to keep them.
> +#define VIRTIO_BALLOON_PAGE_BMAP_MAX_NUM 32
> +
So you still have a home-grown bitmap. I'd like to know why
isn't xbitmap suggested for this purpose by Matthew Wilcox
appropriate. Please add a comment explaining the requirements
from the data structure.
> +/*
> + * QEMU virtio implementation requires the desc table size less than
> + * VIRTQUEUE_MAX_SIZE, so minus 1 here.
I think it doesn't, the issue is probably that you add a header
as a separate s/g. In any case see below.
> + */
> +#define VIRTIO_BALLOON_MAX_PAGE_CHUNKS (VIRTQUEUE_MAX_SIZE - 1)
This is wrong, virtio spec says s/g size should not exceed VQ size.
If you want to support huge VQ sizes, you can add a fallback to
smaller sizes until it fits in 1 page.
> +
> +/* The struct to manage ballooned pages in chunks */
> +struct virtio_balloon_page_chunk {
> + /* Indirect desc table to hold chunks of balloon pages */
> + struct vring_desc *desc_table;
> + /* Number of added chunks of balloon pages */
> + unsigned int chunk_num;
> + /* Bitmap used to record ballooned pages. */
> + unsigned long *page_bmap[VIRTIO_BALLOON_PAGE_BMAP_MAX_NUM];
> +};
> +
> struct virtio_balloon {
> struct virtio_device *vdev;
> struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
> @@ -79,6 +109,8 @@ struct virtio_balloon {
> /* Synchronize access/update to this struct virtio_balloon elements */
> struct mutex balloon_lock;
>
> + struct virtio_balloon_page_chunk balloon_page_chunk;
> +
> /* The array of pfns we tell the Host about. */
> unsigned int num_pfns;
> __virtio32 pfns[VIRTIO_BALLOON_ARRAY_PFNS_MAX];
> @@ -111,6 +143,133 @@ static void balloon_ack(struct virtqueue *vq)
> wake_up(&vb->acked);
> }
>
> +/* Update pfn_max and pfn_min according to the pfn of page */
> +static inline void update_pfn_range(struct virtio_balloon *vb,
> + struct page *page,
> + unsigned long *pfn_min,
> + unsigned long *pfn_max)
> +{
> + unsigned long pfn = page_to_pfn(page);
> +
> + *pfn_min = min(pfn, *pfn_min);
> + *pfn_max = max(pfn, *pfn_max);
> +}
> +
> +static unsigned int extend_page_bmap_size(struct virtio_balloon *vb,
> + unsigned long pfn_num)
what's this API doing? Pls add comments. this seems to assume
it will only be called once. it would be better to avoid making
this assumption, just look at what has been allocated
and extend it.
> +{
> + unsigned int i, bmap_num, allocated_bmap_num;
> + unsigned long bmap_len;
> +
> + allocated_bmap_num = VIRTIO_BALLOON_PAGE_BMAP_DEFAULT_NUM;
how come? Pls init vars where they are declared.
> + bmap_len = ALIGN(pfn_num, BITS_PER_LONG) / BITS_PER_BYTE;
> + bmap_len = roundup(bmap_len, VIRTIO_BALLOON_PAGE_BMAP_SIZE);
> + /*
> + * VIRTIO_BALLOON_PAGE_BMAP_SIZE is the size of one page_bmap, so
> + * divide it to calculate how many page_bmap that we need.
> + */
> + bmap_num = (unsigned int)(bmap_len / VIRTIO_BALLOON_PAGE_BMAP_SIZE);
> + /* The number of page_bmap to allocate should not exceed the max */
> + bmap_num = min_t(unsigned int, VIRTIO_BALLOON_PAGE_BMAP_MAX_NUM,
> + bmap_num);
two comments above don't really help just drop them.
> +
> + for (i = VIRTIO_BALLOON_PAGE_BMAP_DEFAULT_NUM; i < bmap_num; i++) {
> + vb->balloon_page_chunk.page_bmap[i] =
> + kmalloc(VIRTIO_BALLOON_PAGE_BMAP_SIZE, GFP_KERNEL);
> + if (vb->balloon_page_chunk.page_bmap[i])
> + allocated_bmap_num++;
> + else
> + break;
> + }
> +
> + return allocated_bmap_num;
> +}
> +
> +static void free_extended_page_bmap(struct virtio_balloon *vb,
> + unsigned int page_bmap_num)
> +{
> + unsigned int i;
> +
> + for (i = VIRTIO_BALLOON_PAGE_BMAP_DEFAULT_NUM; i < page_bmap_num;
> + i++) {
> + kfree(vb->balloon_page_chunk.page_bmap[i]);
> + vb->balloon_page_chunk.page_bmap[i] = NULL;
> + page_bmap_num--;
> + }
> +}
> +
> +static void clear_page_bmap(struct virtio_balloon *vb,
> + unsigned int page_bmap_num)
> +{
> + int i;
> +
> + for (i = 0; i < page_bmap_num; i++)
> + memset(vb->balloon_page_chunk.page_bmap[i], 0,
> + VIRTIO_BALLOON_PAGE_BMAP_SIZE);
> +}
> +
> +static void send_page_chunks(struct virtio_balloon *vb, struct virtqueue *vq)
> +{
> + unsigned int len, num;
> + struct vring_desc *desc = vb->balloon_page_chunk.desc_table;
> +
> + num = vb->balloon_page_chunk.chunk_num;
> + if (!virtqueue_indirect_desc_table_add(vq, desc, num)) {
> + virtqueue_kick(vq);
> + wait_event(vb->acked, virtqueue_get_buf(vq, &len));
> + vb->balloon_page_chunk.chunk_num = 0;
> + }
> +}
> +
> +/* Add a chunk to the buffer. */
> +static void add_one_chunk(struct virtio_balloon *vb, struct virtqueue *vq,
> + u64 base_addr, u32 size)
> +{
> + unsigned int *num = &vb->balloon_page_chunk.chunk_num;
> + struct vring_desc *desc = &vb->balloon_page_chunk.desc_table[*num];
> +
> + desc->addr = cpu_to_virtio64(vb->vdev, base_addr);
> + desc->len = cpu_to_virtio32(vb->vdev, size);
> + *num += 1;
> + if (*num == VIRTIO_BALLOON_MAX_PAGE_CHUNKS)
> + send_page_chunks(vb, vq);
> +}
> +
Poking at virtio internals like this is not nice. Pls move to virtio
code. Also, pages must be read descriptors as host might modify them.
This also lacks viommu support but this is not mandatory as
that is borken atm anyway. I'll send a patch to at least fail cleanly.
> +static void convert_bmap_to_chunks(struct virtio_balloon *vb,
> + struct virtqueue *vq,
> + unsigned long *bmap,
> + unsigned long pfn_start,
> + unsigned long size)
> +{
> + unsigned long next_one, next_zero, pos = 0;
> + u64 chunk_base_addr;
> + u32 chunk_size;
> +
> + while (pos < size) {
> + next_one = find_next_bit(bmap, size, pos);
> + /*
> + * No "1" bit found, which means that there is no pfn
> + * recorded in the rest of this bmap.
> + */
> + if (next_one == size)
> + break;
> + next_zero = find_next_zero_bit(bmap, size, next_one + 1);
> + /*
> + * A bit in page_bmap corresponds to a page of PAGE_SIZE.
> + * Convert it to be pages of 4KB balloon page size when
> + * adding it to a chunk.
This looks wrong. add_one_chunk assumes size in bytes. So should be just
PAGE_SIZE.
> + */
> + chunk_size = (next_zero - next_one) *
> + VIRTIO_BALLOON_PAGES_PER_PAGE;
How do you know this won't overflow a 32 bit integer? Needs a comment.
> + chunk_base_addr = (pfn_start + next_one) <<
> + VIRTIO_BALLOON_PFN_SHIFT;
Same here I think we've left pfns behind, we are using standard s/g now.
> + if (chunk_size) {
> + add_one_chunk(vb, vq, chunk_base_addr, chunk_size);
> + pos += next_zero + 1;
> + }
> + }
> +}
> +
> static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
> {
> struct scatterlist sg;
> @@ -124,7 +283,35 @@ static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
>
> /* When host has read buffer, this completes via balloon_ack */
> wait_event(vb->acked, virtqueue_get_buf(vq, &len));
> +}
> +
> +static void tell_host_from_page_bmap(struct virtio_balloon *vb,
> + struct virtqueue *vq,
> + unsigned long pfn_start,
> + unsigned long pfn_end,
> + unsigned int page_bmap_num)
> +{
> + unsigned long i, pfn_num;
>
> + for (i = 0; i < page_bmap_num; i++) {
> + /*
> + * For the last page_bmap, only the remaining number of pfns
> + * need to be searched rather than the entire page_bmap.
> + */
> + if (i + 1 == page_bmap_num)
> + pfn_num = (pfn_end - pfn_start) %
> + VIRTIO_BALLOON_PFNS_PER_PAGE_BMAP;
> + else
> + pfn_num = VIRTIO_BALLOON_PFNS_PER_PAGE_BMAP;
> +
> + convert_bmap_to_chunks(vb, vq,
> + vb->balloon_page_chunk.page_bmap[i],
> + pfn_start +
> + i * VIRTIO_BALLOON_PFNS_PER_PAGE_BMAP,
> + pfn_num);
> + }
> + if (vb->balloon_page_chunk.chunk_num > 0)
> + send_page_chunks(vb, vq);
> }
>
> static void set_page_pfns(struct virtio_balloon *vb,
> @@ -141,13 +328,89 @@ static void set_page_pfns(struct virtio_balloon *vb,
> page_to_balloon_pfn(page) + i);
> }
>
> +/*
> + * Send ballooned pages in chunks to host.
> + * The ballooned pages are recorded in page bitmaps. Each bit in a bitmap
> + * corresponds to a page of PAGE_SIZE. The page bitmaps are searched for
> + * continuous "1" bits, which correspond to continuous pages, to chunk.
> + * When packing those continuous pages into chunks, pages are converted into
> + * 4KB balloon pages.
> + *
> + * pfn_max and pfn_min form the range of pfns that need to use page bitmaps to
> + * record. If the range is too large to be recorded into the allocated page
> + * bitmaps, the page bitmaps are used multiple times to record the entire
> + * range of pfns.
> + */
> +static void tell_host_page_chunks(struct virtio_balloon *vb,
> + struct list_head *pages,
> + struct virtqueue *vq,
> + unsigned long pfn_max,
> + unsigned long pfn_min)
> +{
> + /*
> + * The pfn_start and pfn_end form the range of pfns that the allocated
> + * page_bmap can record in each round.
> + */
> + unsigned long pfn_start, pfn_end;
> + /* Total number of allocated page_bmap */
> + unsigned int page_bmap_num;
> + struct page *page;
> + bool found;
> +
> + /*
> + * In the case that one page_bmap is not sufficient to record the pfn
> + * range, page_bmap will be extended by allocating more numbers of
> + * page_bmap.
> + */
> + page_bmap_num = extend_page_bmap_size(vb, pfn_max - pfn_min + 1);
> +
> + /* Start from the beginning of the whole pfn range */
> + pfn_start = pfn_min;
> + while (pfn_start < pfn_max) {
> + pfn_end = pfn_start +
> + VIRTIO_BALLOON_PFNS_PER_PAGE_BMAP * page_bmap_num;
> + pfn_end = pfn_end < pfn_max ? pfn_end : pfn_max;
> + clear_page_bmap(vb, page_bmap_num);
> + found = false;
> +
> + list_for_each_entry(page, pages, lru) {
> + unsigned long bmap_idx, bmap_pos, this_pfn;
> +
> + this_pfn = page_to_pfn(page);
> + if (this_pfn < pfn_start || this_pfn > pfn_end)
> + continue;
> + bmap_idx = (this_pfn - pfn_start) /
> + VIRTIO_BALLOON_PFNS_PER_PAGE_BMAP;
> + bmap_pos = (this_pfn - pfn_start) %
> + VIRTIO_BALLOON_PFNS_PER_PAGE_BMAP;
> + set_bit(bmap_pos,
> + vb->balloon_page_chunk.page_bmap[bmap_idx]);
> +
> + found = true;
> + }
> + if (found)
> + tell_host_from_page_bmap(vb, vq, pfn_start, pfn_end,
> + page_bmap_num);
> + /*
> + * Start the next round when pfn_start and pfn_end couldn't
> + * cover the whole pfn range given by pfn_max and pfn_min.
> + */
> + pfn_start = pfn_end;
> + }
> + free_extended_page_bmap(vb, page_bmap_num);
> +}
> +
> static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> {
> struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
> unsigned num_allocated_pages;
> + bool chunking = virtio_has_feature(vb->vdev,
> + VIRTIO_BALLOON_F_PAGE_CHUNKS);
> + unsigned long pfn_max = 0, pfn_min = ULONG_MAX;
>
> /* We can only do one array worth at a time. */
> - num = min(num, ARRAY_SIZE(vb->pfns));
> + if (!chunking)
> + num = min(num, ARRAY_SIZE(vb->pfns));
>
> mutex_lock(&vb->balloon_lock);
> for (vb->num_pfns = 0; vb->num_pfns < num;
> @@ -162,7 +425,10 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> msleep(200);
> break;
> }
> - set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> + if (chunking)
> + update_pfn_range(vb, page, &pfn_min, &pfn_max);
> + else
> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> if (!virtio_has_feature(vb->vdev,
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> @@ -171,8 +437,14 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
>
> num_allocated_pages = vb->num_pfns;
> /* Did we get any? */
> - if (vb->num_pfns != 0)
> - tell_host(vb, vb->inflate_vq);
> + if (vb->num_pfns != 0) {
> + if (chunking)
> + tell_host_page_chunks(vb, &vb_dev_info->pages,
> + vb->inflate_vq,
> + pfn_max, pfn_min);
> + else
> + tell_host(vb, vb->inflate_vq);
> + }
> mutex_unlock(&vb->balloon_lock);
>
> return num_allocated_pages;
> @@ -198,9 +470,13 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
> struct page *page;
> struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
> LIST_HEAD(pages);
> + bool chunking = virtio_has_feature(vb->vdev,
> + VIRTIO_BALLOON_F_PAGE_CHUNKS);
> + unsigned long pfn_max = 0, pfn_min = ULONG_MAX;
>
> - /* We can only do one array worth at a time. */
> - num = min(num, ARRAY_SIZE(vb->pfns));
> + /* Traditionally, we can only do one array worth at a time. */
> + if (!chunking)
> + num = min(num, ARRAY_SIZE(vb->pfns));
>
> mutex_lock(&vb->balloon_lock);
> /* We can't release more pages than taken */
> @@ -210,7 +486,10 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
> page = balloon_page_dequeue(vb_dev_info);
> if (!page)
> break;
> - set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> + if (chunking)
> + update_pfn_range(vb, page, &pfn_min, &pfn_max);
> + else
> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> list_add(&page->lru, &pages);
> vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
> }
> @@ -221,8 +500,13 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
> * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> * is true, we *have* to do it in this order
> */
> - if (vb->num_pfns != 0)
> - tell_host(vb, vb->deflate_vq);
> + if (vb->num_pfns != 0) {
> + if (chunking)
> + tell_host_page_chunks(vb, &pages, vb->deflate_vq,
> + pfn_max, pfn_min);
> + else
> + tell_host(vb, vb->deflate_vq);
> + }
> release_pages_balloon(vb, &pages);
> mutex_unlock(&vb->balloon_lock);
> return num_freed_pages;
> @@ -442,6 +726,14 @@ static int init_vqs(struct virtio_balloon *vb)
> }
>
> #ifdef CONFIG_BALLOON_COMPACTION
> +
> +static void tell_host_one_page(struct virtio_balloon *vb,
> + struct virtqueue *vq, struct page *page)
> +{
> + add_one_chunk(vb, vq, page_to_pfn(page) << VIRTIO_BALLOON_PFN_SHIFT,
> + VIRTIO_BALLOON_PAGES_PER_PAGE);
> +}
> +
> /*
> * virtballoon_migratepage - perform the balloon page migration on behalf of
> * a compation thread. (called under page lock)
> @@ -465,6 +757,8 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
> {
> struct virtio_balloon *vb = container_of(vb_dev_info,
> struct virtio_balloon, vb_dev_info);
> + bool chunking = virtio_has_feature(vb->vdev,
> + VIRTIO_BALLOON_F_PAGE_CHUNKS);
> unsigned long flags;
>
> /*
> @@ -486,16 +780,22 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
> vb_dev_info->isolated_pages--;
> __count_vm_event(BALLOON_MIGRATE);
> spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags);
> - vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> - set_page_pfns(vb, vb->pfns, newpage);
> - tell_host(vb, vb->inflate_vq);
> -
> + if (chunking) {
> + tell_host_one_page(vb, vb->inflate_vq, newpage);
> + } else {
> + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> + set_page_pfns(vb, vb->pfns, newpage);
> + tell_host(vb, vb->inflate_vq);
> + }
> /* balloon's page migration 2nd step -- deflate "page" */
> balloon_page_delete(page);
> - vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> - set_page_pfns(vb, vb->pfns, page);
> - tell_host(vb, vb->deflate_vq);
> -
> + if (chunking) {
> + tell_host_one_page(vb, vb->deflate_vq, page);
> + } else {
> + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> + set_page_pfns(vb, vb->pfns, page);
> + tell_host(vb, vb->deflate_vq);
> + }
> mutex_unlock(&vb->balloon_lock);
>
> put_page(page); /* balloon reference */
> @@ -522,9 +822,78 @@ static struct file_system_type balloon_fs = {
>
> #endif /* CONFIG_BALLOON_COMPACTION */
>
> +static void free_page_bmap(struct virtio_balloon *vb)
> +{
> + int i;
> +
> + for (i = 0; i < VIRTIO_BALLOON_PAGE_BMAP_DEFAULT_NUM; i++) {
> + kfree(vb->balloon_page_chunk.page_bmap[i]);
> + vb->balloon_page_chunk.page_bmap[i] = NULL;
> + }
> +}
> +
> +static int balloon_page_chunk_init(struct virtio_balloon *vb)
> +{
> + int i;
> +
> + vb->balloon_page_chunk.desc_table = alloc_indirect(vb->vdev,
> + VIRTIO_BALLOON_MAX_PAGE_CHUNKS,
> + GFP_KERNEL);
This one's problematic, you aren't supposed to use APIs when device
is not inited yet. Seems to work by luck here. I suggest moving
this to probe, that's where we do a bunch of inits.
And then you can move private init back to allocate too.
> + if (!vb->balloon_page_chunk.desc_table)
> + goto err_page_chunk;
> + vb->balloon_page_chunk.chunk_num = 0;
> +
> + /*
> + * The default number of page_bmaps are allocated. More may be
> + * allocated on demand.
> + */
> + for (i = 0; i < VIRTIO_BALLOON_PAGE_BMAP_DEFAULT_NUM; i++) {
> + vb->balloon_page_chunk.page_bmap[i] =
> + kmalloc(VIRTIO_BALLOON_PAGE_BMAP_SIZE, GFP_KERNEL);
> + if (!vb->balloon_page_chunk.page_bmap[i])
> + goto err_page_bmap;
> + }
> +
> + return 0;
> +err_page_bmap:
> + free_page_bmap(vb);
> + kfree(vb->balloon_page_chunk.desc_table);
> + vb->balloon_page_chunk.desc_table = NULL;
> +err_page_chunk:
> + __virtio_clear_bit(vb->vdev, VIRTIO_BALLOON_F_PAGE_CHUNKS);
> + dev_warn(&vb->vdev->dev, "%s: failed\n", __func__);
> + return -ENOMEM;
> +}
> +
> +static int virtballoon_validate(struct virtio_device *vdev)
> +{
> + struct virtio_balloon *vb = NULL;
> + int err;
> +
> + vdev->priv = vb = kmalloc(sizeof(*vb), GFP_KERNEL);
> + if (!vb) {
> + err = -ENOMEM;
> + goto err_vb;
> + }
> + vb->vdev = vdev;
> +
> + if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_CHUNKS)) {
> + err = balloon_page_chunk_init(vb);
> + if (err < 0)
> + goto err_page_chunk;
> + }
> +
> + return 0;
> +
> +err_page_chunk:
> + kfree(vb);
> +err_vb:
> + return err;
> +}
> +
So here you are supposed to validate features, not handle OOM
conditions. BTW we need a fix for vIOMMU - I noticed balloon does not
support that yes.
> static int virtballoon_probe(struct virtio_device *vdev)
> {
> - struct virtio_balloon *vb;
> + struct virtio_balloon *vb = vdev->priv;
> int err;
>
> if (!vdev->config->get) {
> @@ -533,20 +902,14 @@ static int virtballoon_probe(struct virtio_device *vdev)
> return -EINVAL;
> }
>
> - vdev->priv = vb = kmalloc(sizeof(*vb), GFP_KERNEL);
> - if (!vb) {
> - err = -ENOMEM;
> - goto out;
> - }
> -
> INIT_WORK(&vb->update_balloon_stats_work, update_balloon_stats_func);
> INIT_WORK(&vb->update_balloon_size_work, update_balloon_size_func);
> spin_lock_init(&vb->stop_update_lock);
> vb->stop_update = false;
> vb->num_pages = 0;
> +
> mutex_init(&vb->balloon_lock);
> init_waitqueue_head(&vb->acked);
> - vb->vdev = vdev;
>
> balloon_devinfo_init(&vb->vb_dev_info);
>
> @@ -590,7 +953,6 @@ static int virtballoon_probe(struct virtio_device *vdev)
> vdev->config->del_vqs(vdev);
> out_free_vb:
> kfree(vb);
> -out:
> return err;
> }
>
> @@ -620,6 +982,8 @@ static void virtballoon_remove(struct virtio_device *vdev)
> cancel_work_sync(&vb->update_balloon_stats_work);
>
> remove_common(vb);
> + free_page_bmap(vb);
> + kfree(vb->balloon_page_chunk.desc_table);
> #ifdef CONFIG_BALLOON_COMPACTION
> if (vb->vb_dev_info.inode)
> iput(vb->vb_dev_info.inode);
> @@ -664,6 +1028,7 @@ static unsigned int features[] = {
> VIRTIO_BALLOON_F_MUST_TELL_HOST,
> VIRTIO_BALLOON_F_STATS_VQ,
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
> + VIRTIO_BALLOON_F_PAGE_CHUNKS,
> };
>
> static struct virtio_driver virtio_balloon_driver = {
> @@ -674,6 +1039,7 @@ static struct virtio_driver virtio_balloon_driver = {
> .id_table = id_table,
> .probe = virtballoon_probe,
> .remove = virtballoon_remove,
> + .validate = virtballoon_validate,
> .config_changed = virtballoon_changed,
> #ifdef CONFIG_PM_SLEEP
> .freeze = virtballoon_freeze,
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 409aeaa..0ea2512 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -235,8 +235,17 @@ static int vring_mapping_error(const struct vring_virtqueue *vq,
> return dma_mapping_error(vring_dma_dev(vq), addr);
> }
>
> -static struct vring_desc *alloc_indirect(struct virtqueue *_vq,
> - unsigned int total_sg, gfp_t gfp)
> +/**
> + * alloc_indirect - allocate an indirect desc table
> + * @vdev: the virtio_device that owns the indirect desc table.
> + * @num: the number of entries that the table will have.
> + * @gfp: how to do memory allocations (if necessary).
> + *
> + * Return NULL if the table allocation failed. Otherwise, return the address
> + * of the table.
> + */
> +struct vring_desc *alloc_indirect(struct virtio_device *vdev, unsigned int num,
> + gfp_t gfp)
> {
> struct vring_desc *desc;
> unsigned int i;
> @@ -248,14 +257,15 @@ static struct vring_desc *alloc_indirect(struct virtqueue *_vq,
> */
> gfp &= ~__GFP_HIGHMEM;
>
> - desc = kmalloc(total_sg * sizeof(struct vring_desc), gfp);
> + desc = kmalloc_array(num, sizeof(struct vring_desc), gfp);
> if (!desc)
> return NULL;
>
> - for (i = 0; i < total_sg; i++)
> - desc[i].next = cpu_to_virtio16(_vq->vdev, i + 1);
> + for (i = 0; i < num; i++)
> + desc[i].next = cpu_to_virtio16(vdev, i + 1);
> return desc;
> }
> +EXPORT_SYMBOL_GPL(alloc_indirect);
>
> static inline int virtqueue_add(struct virtqueue *_vq,
> struct scatterlist *sgs[],
> @@ -302,7 +312,7 @@ static inline int virtqueue_add(struct virtqueue *_vq,
> /* If the host supports indirect descriptor tables, and we have multiple
> * buffers, then go indirect. FIXME: tune this threshold */
> if (vq->indirect && total_sg > 1 && vq->vq.num_free)
> - desc = alloc_indirect(_vq, total_sg, gfp);
> + desc = alloc_indirect(_vq->vdev, total_sg, gfp);
> else
> desc = NULL;
>
> @@ -433,6 +443,104 @@ static inline int virtqueue_add(struct virtqueue *_vq,
> }
>
> /**
> + * virtqueue_indirect_desc_table_add - add an indirect desc table to the vq
> + * @_vq: the struct virtqueue we're talking about.
> + * @desc: the desc table we're talking about.
> + * @num: the number of entries that the desc table has.
> + *
> + * Returns zero or a negative error (ie. ENOSPC, EIO).
> + */
> +int virtqueue_indirect_desc_table_add(struct virtqueue *_vq,
> + struct vring_desc *desc,
> + unsigned int num)
> +{
> + struct vring_virtqueue *vq = to_vvq(_vq);
> + dma_addr_t desc_addr;
> + unsigned int i, avail;
> + int head;
> +
> + /* Sanity check */
> + if (!desc) {
> + pr_debug("%s: empty desc table\n", __func__);
> + return -EINVAL;
> + }
> +
> + START_USE(vq);
> +
> + if (unlikely(vq->broken)) {
> + END_USE(vq);
> + return -EIO;
> + }
> +
> + if (!vq->vq.num_free) {
> + pr_debug("%s: the virtioqueue is full\n", __func__);
> + END_USE(vq);
> + return -ENOSPC;
> + }
> +
> + /* Map and fill in the indirect table */
> + desc_addr = vring_map_single(vq, desc, num * sizeof(struct vring_desc),
> + DMA_TO_DEVICE);
> + if (vring_mapping_error(vq, desc_addr)) {
> + pr_debug("%s: map desc failed\n", __func__);
> + END_USE(vq);
> + return -EIO;
> + }
> +
> + /* Mark the flag of the table entries */
> + for (i = 0; i < num; i++)
> + desc[i].flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_NEXT);
> + /* The last one doesn't continue. */
> + desc[num - 1].flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);
> +
> + /* Get a ring entry to point to the indirect table */
> + head = vq->free_head;
> + vq->vring.desc[head].flags = cpu_to_virtio16(_vq->vdev,
> + VRING_DESC_F_INDIRECT);
> + vq->vring.desc[head].addr = cpu_to_virtio64(_vq->vdev, desc_addr);
> + vq->vring.desc[head].len = cpu_to_virtio32(_vq->vdev, num *
> + sizeof(struct vring_desc));
> + /* We're using 1 buffers from the free list. */
> + vq->vq.num_free--;
> + /* Update free pointer */
> + vq->free_head = virtio16_to_cpu(_vq->vdev, vq->vring.desc[head].next);
> +
> + /* Store token and indirect buffer state. */
> + vq->desc_state[head].data = desc;
> + /* Don't free the caller allocated indirect table when detach_buf. */
> + vq->desc_state[head].indir_desc = NULL;
> +
> + /*
> + * Put entry in available array (but don't update avail->idx until they
> + * do sync).
> + */
> + avail = vq->avail_idx_shadow & (vq->vring.num - 1);
> + vq->vring.avail->ring[avail] = cpu_to_virtio16(_vq->vdev, head);
> +
> + /*
> + * Descriptors and available array need to be set before we expose the
> + * new available array entries.
> + */
> + virtio_wmb(vq->weak_barriers);
> + vq->avail_idx_shadow++;
> + vq->vring.avail->idx = cpu_to_virtio16(_vq->vdev, vq->avail_idx_shadow);
> + vq->num_added++;
> +
> + pr_debug("%s: added buffer head %i to %p\n", __func__, head, vq);
> + END_USE(vq);
> +
> + /*
> + * This is very unlikely, but theoretically possible. Kick
> + * just in case.
> + */
> + if (unlikely(vq->num_added == (1 << 16) - 1))
> + virtqueue_kick(_vq);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(virtqueue_indirect_desc_table_add);
> +
I'm not really happy with the fact we are duplicating so much code. Most
of this is duplicated from virtqueue_add, isn't it? I imagine you just
need to factor out the code from the following place down:
/* If the host supports indirect descriptor tables, and we have multiple
* buffers, then go indirect. FIXME: tune this threshold */
if (vq->indirect && total_sg > 1 && vq->vq.num_free)
desc = alloc_indirect(_vq, total_sg, gfp);
else
desc = NULL;
then reuse that.
> +/**
> * virtqueue_add_sgs - expose buffers to other end
> * @vq: the struct virtqueue we're talking about.
> * @sgs: array of terminated scatterlists.
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 7edfbdb..01dad22 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -34,6 +34,13 @@ struct virtqueue {
> void *priv;
> };
>
> +struct vring_desc *alloc_indirect(struct virtio_device *vdev,
> + unsigned int num, gfp_t gfp);
> +
Please prefix with virtqueue or virtio (depending on 1st parameter).
You also want a free API to pair with this (even though it's just kfree
right now).
> +int virtqueue_indirect_desc_table_add(struct virtqueue *_vq,
> + struct vring_desc *desc,
> + unsigned int num);
> +
> int virtqueue_add_outbuf(struct virtqueue *vq,
> struct scatterlist sg[], unsigned int num,
> void *data,
> diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
> index 343d7dd..5ed3c7b 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -34,6 +34,7 @@
> #define VIRTIO_BALLOON_F_MUST_TELL_HOST 0 /* Tell before reclaiming pages */
> #define VIRTIO_BALLOON_F_STATS_VQ 1 /* Memory Stats virtqueue */
> #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon on OOM */
> +#define VIRTIO_BALLOON_F_PAGE_CHUNKS 3 /* Inflate/Deflate pages in chunks */
>
> /* Size of a PFN in the balloon interface. */
> #define VIRTIO_BALLOON_PFN_SHIFT 12
> diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
> index c072959..0499fb8 100644
> --- a/include/uapi/linux/virtio_ring.h
> +++ b/include/uapi/linux/virtio_ring.h
> @@ -111,6 +111,9 @@ struct vring {
> #define VRING_USED_ALIGN_SIZE 4
> #define VRING_DESC_ALIGN_SIZE 16
>
> +/* The supported max queue size */
> +#define VIRTQUEUE_MAX_SIZE 1024
> +
> /* The standard layout for the ring is a continuous chunk of memory which looks
> * like this. We assume num is a power of 2.
> *
Please do not add this to UAPI.
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH v11 6/6] virtio-balloon: VIRTIO_BALLOON_F_CMD_VQ
From: Wei Wang @ 2017-06-13 10:17 UTC (permalink / raw)
To: Dave Hansen, virtio-dev, linux-kernel, qemu-devel, virtualization,
kvm, linux-mm, mst, david, cornelia.huck, akpm, mgorman, aarcange,
amit.shah, pbonzini, liliang.opensource
In-Reply-To: <db8cc3d1-50fc-2412-af2f-1070dda38be3@intel.com>
On 06/12/2017 10:07 PM, Dave Hansen wrote:
> On 06/09/2017 03:41 AM, Wei Wang wrote:
>> + for_each_populated_zone(zone) {
>> + for (order = MAX_ORDER - 1; order > 0; order--) {
>> + for (migratetype = 0; migratetype < MIGRATE_TYPES;
>> + migratetype++) {
>> + do {
>> + ret = report_unused_page_block(zone,
>> + order, migratetype, &page);
>> + if (!ret) {
>> + pfn = (u64)page_to_pfn(page);
>> + add_one_chunk(vb, vq,
>> + PAGE_CHNUK_UNUSED_PAGE,
>> + pfn << VIRTIO_BALLOON_PFN_SHIFT,
>> + (u64)(1 << order) *
>> + VIRTIO_BALLOON_PAGES_PER_PAGE);
>> + }
>> + } while (!ret);
>> + }
>> + }
>> + }
> This is pretty unreadable. Please add some indentation. If you go
> over 80 cols, then you might need to break this up into a separate
> function. But, either way, it can't be left like this.
OK, I'll re-arrange it.
Best,
Wei
^ permalink raw reply
* Re: [PATCH v11 4/6] mm: function to offer a page block on the free list
From: Wei Wang @ 2017-06-13 2:56 UTC (permalink / raw)
To: Dave Hansen, Michael S. Tsirkin
Cc: aarcange, kvm, qemu-devel, amit.shah, liliang.opensource,
linux-kernel, virtualization, linux-mm, cornelia.huck, pbonzini,
akpm, mgorman
In-Reply-To: <d0811862-6633-a43c-90a5-629fe9b6d150@intel.com>
On 06/13/2017 04:54 AM, Dave Hansen wrote:
> On 06/12/2017 01:34 PM, Michael S. Tsirkin wrote:
>> On Mon, Jun 12, 2017 at 09:42:36AM -0700, Dave Hansen wrote:
>>> On 06/12/2017 09:28 AM, Michael S. Tsirkin wrote:
>>>>> The hypervisor is going to throw away the contents of these pages,
>>>>> right?
>>>> It should be careful and only throw away contents that was there before
>>>> report_unused_page_block was invoked. Hypervisor is responsible for not
>>>> corrupting guest memory. But that's not something an mm patch should
>>>> worry about.
>>> That makes sense. I'm struggling to imagine how the hypervisor makes
>>> use of this information, though. Does it make the pages read-only
>>> before this, and then it knows if there has not been a write *and* it
>>> gets notified via this new mechanism that it can throw the page away?
>> Yes, and specifically, this is how it works for migration. Normally you
>> start by migrating all of memory, then send updates incrementally if
>> pages have been modified. This mechanism allows skipping some pages in
>> the 1st stage, if they get changed they will be migrated in the 2nd
>> stage.
> OK, so the migration starts and marks everything read-only. All the
> pages now have read-only valuable data, or read-only worthless data in
> the case that the page is in the free lists. In order for a page to
> become non-worthless, it has to have a write done to it, which the
> hypervisor obviously knows about.
>
> With this mechanism, the hypervisor knows it can discard pages which
> have not had a write since they were known to have worthless contents.
>
> Correct?
Right. By the way, ready-only is one of the dirty page logging
methods that a hypervisor uses to capture the pages that are
written by the VM.
>
> That also seems like pretty good information to include in the
> changelog. Otherwise, folks are going to be left wondering what good
> the mechanism is. It's pretty non-trivial to figure out. :)
If necessary, I think it's better to keep the introduction at high-level:
Examples of using this API by a hypervisor:
To live migrate a VM from one physical machine to another,
the hypervisor usually transfers all the VM's memory content.
An optimization here is to skip the transfer of memory that are not
in use by the VM, because the content of the unused memory is
worthless.
This API is the used to report the unused pages to the hypervisor.
The pages that have been reported to the hypervisor as unused
pages may be used by the VM after the report. The hypervisor
has a good mechanism (i.e. dirty page logging) to capture
the change. Therefore, if the new used pages are written into some
data, the hypervisor will still transfer them to the destination machine.
What do you guys think?
Best,
Wei
^ permalink raw reply
* Re: [PATCH v11 4/6] mm: function to offer a page block on the free list
From: Dave Hansen @ 2017-06-12 20:54 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aarcange, kvm, qemu-devel, amit.shah, liliang.opensource,
linux-kernel, virtualization, linux-mm, cornelia.huck, pbonzini,
akpm, mgorman
In-Reply-To: <20170612194438-mutt-send-email-mst@kernel.org>
On 06/12/2017 01:34 PM, Michael S. Tsirkin wrote:
> On Mon, Jun 12, 2017 at 09:42:36AM -0700, Dave Hansen wrote:
>> On 06/12/2017 09:28 AM, Michael S. Tsirkin wrote:
>>>
>>>> The hypervisor is going to throw away the contents of these pages,
>>>> right?
>>> It should be careful and only throw away contents that was there before
>>> report_unused_page_block was invoked. Hypervisor is responsible for not
>>> corrupting guest memory. But that's not something an mm patch should
>>> worry about.
>>
>> That makes sense. I'm struggling to imagine how the hypervisor makes
>> use of this information, though. Does it make the pages read-only
>> before this, and then it knows if there has not been a write *and* it
>> gets notified via this new mechanism that it can throw the page away?
>
> Yes, and specifically, this is how it works for migration. Normally you
> start by migrating all of memory, then send updates incrementally if
> pages have been modified. This mechanism allows skipping some pages in
> the 1st stage, if they get changed they will be migrated in the 2nd
> stage.
OK, so the migration starts and marks everything read-only. All the
pages now have read-only valuable data, or read-only worthless data in
the case that the page is in the free lists. In order for a page to
become non-worthless, it has to have a write done to it, which the
hypervisor obviously knows about.
With this mechanism, the hypervisor knows it can discard pages which
have not had a write since they were known to have worthless contents.
Correct?
That also seems like pretty good information to include in the
changelog. Otherwise, folks are going to be left wondering what good
the mechanism is. It's pretty non-trivial to figure out. :)
^ permalink raw reply
* Re: [PATCH v11 4/6] mm: function to offer a page block on the free list
From: Michael S. Tsirkin @ 2017-06-12 20:34 UTC (permalink / raw)
To: Dave Hansen
Cc: aarcange, kvm, qemu-devel, amit.shah, liliang.opensource,
linux-kernel, virtualization, linux-mm, cornelia.huck, pbonzini,
akpm, mgorman
In-Reply-To: <9d0900f3-9df5-ac63-4069-2d796f2a5bc7@intel.com>
On Mon, Jun 12, 2017 at 09:42:36AM -0700, Dave Hansen wrote:
> On 06/12/2017 09:28 AM, Michael S. Tsirkin wrote:
> >
> >> The hypervisor is going to throw away the contents of these pages,
> >> right?
> > It should be careful and only throw away contents that was there before
> > report_unused_page_block was invoked. Hypervisor is responsible for not
> > corrupting guest memory. But that's not something an mm patch should
> > worry about.
>
> That makes sense. I'm struggling to imagine how the hypervisor makes
> use of this information, though. Does it make the pages read-only
> before this, and then it knows if there has not been a write *and* it
> gets notified via this new mechanism that it can throw the page away?
Yes, and specifically, this is how it works for migration. Normally you
start by migrating all of memory, then send updates incrementally if
pages have been modified. This mechanism allows skipping some pages in
the 1st stage, if they get changed they will be migrated in the 2nd
stage.
--
MST
^ permalink raw reply
* Re: [PATCH v11 4/6] mm: function to offer a page block on the free list
From: Dave Hansen @ 2017-06-12 16:42 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aarcange, kvm, qemu-devel, amit.shah, liliang.opensource,
linux-kernel, virtualization, linux-mm, cornelia.huck, pbonzini,
akpm, mgorman
In-Reply-To: <20170612181354-mutt-send-email-mst@kernel.org>
On 06/12/2017 09:28 AM, Michael S. Tsirkin wrote:
>
>> The hypervisor is going to throw away the contents of these pages,
>> right?
> It should be careful and only throw away contents that was there before
> report_unused_page_block was invoked. Hypervisor is responsible for not
> corrupting guest memory. But that's not something an mm patch should
> worry about.
That makes sense. I'm struggling to imagine how the hypervisor makes
use of this information, though. Does it make the pages read-only
before this, and then it knows if there has not been a write *and* it
gets notified via this new mechanism that it can throw the page away?
^ permalink raw reply
* Re: [PATCH v11 4/6] mm: function to offer a page block on the free list
From: Michael S. Tsirkin @ 2017-06-12 16:28 UTC (permalink / raw)
To: Dave Hansen
Cc: aarcange, kvm, qemu-devel, amit.shah, liliang.opensource,
linux-kernel, virtualization, linux-mm, cornelia.huck, pbonzini,
akpm, mgorman
In-Reply-To: <b92af473-f00e-b956-ea97-eb4626601789@intel.com>
On Mon, Jun 12, 2017 at 07:10:12AM -0700, Dave Hansen wrote:
> Please stop cc'ing me on things also sent to closed mailing lists
> (virtio-dev@lists.oasis-open.org). I'm happy to review things on open
> lists, but I'm not fond of the closed lists bouncing things at me.
>
> On 06/09/2017 03:41 AM, Wei Wang wrote:
> > Add a function to find a page block on the free list specified by the
> > caller. Pages from the page block may be used immediately after the
> > function returns. The caller is responsible for detecting or preventing
> > the use of such pages.
>
> This description doesn't tell me very much about what's going on here.
> Neither does the comment.
>
> "Pages from the page block may be used immediately after the
> function returns".
>
> Used by who? Does the "may" here mean that it is OK, or is it a warning
> that the contents will be thrown away immediately?
I agree here. Don't tell callers what they should do, say what does the
function does. "offer" also confuses. Here's a better comment
--->
mm: support reporting free page blocks
This adds support for reporting blocks of pages on the free list
specified by the caller.
As pages can leave the free list during this call or immediately
afterwards, they are not guaranteed to be free after the function
returns. The only guarantee this makes is that the page was on the free
list at some point in time after the function has been invoked.
Therefore, it is not safe for caller to use any pages on the returned
block or to discard data that is put there after the function returns.
However, it is safe for caller to discard data that was in one of these
pages before the function was invoked.
---
And repeat the last part in a code comment:
* Note: it is not safe for caller to use any pages on the returned
* block or to discard data that is put there after the function returns.
* However, it is safe for caller to discard data that was in one of these
* pages before the function was invoked.
> The hypervisor is going to throw away the contents of these pages,
> right?
It should be careful and only throw away contents that was there before
report_unused_page_block was invoked. Hypervisor is responsible for not
corrupting guest memory. But that's not something an mm patch should
worry about.
> As soon as the spinlock is released, someone can allocate a
> page, and put good data in it. What keeps the hypervisor from throwing
> away good data?
API should require this explicitly. Hopefully above answers this question.
--
MST
^ permalink raw reply
* Re: [PATCH v11 4/6] mm: function to offer a page block on the free list
From: Dave Hansen @ 2017-06-12 14:10 UTC (permalink / raw)
To: Wei Wang, linux-kernel, qemu-devel, virtualization, kvm, linux-mm,
mst, david, cornelia.huck, akpm, mgorman, aarcange, amit.shah,
pbonzini, liliang.opensource
In-Reply-To: <1497004901-30593-5-git-send-email-wei.w.wang@intel.com>
Please stop cc'ing me on things also sent to closed mailing lists
(virtio-dev@lists.oasis-open.org). I'm happy to review things on open
lists, but I'm not fond of the closed lists bouncing things at me.
On 06/09/2017 03:41 AM, Wei Wang wrote:
> Add a function to find a page block on the free list specified by the
> caller. Pages from the page block may be used immediately after the
> function returns. The caller is responsible for detecting or preventing
> the use of such pages.
This description doesn't tell me very much about what's going on here.
Neither does the comment.
"Pages from the page block may be used immediately after the
function returns".
Used by who? Does the "may" here mean that it is OK, or is it a warning
that the contents will be thrown away immediately?
The hypervisor is going to throw away the contents of these pages,
right? As soon as the spinlock is released, someone can allocate a
page, and put good data in it. What keeps the hypervisor from throwing
away good data?
^ permalink raw reply
* Re: [PATCH v11 6/6] virtio-balloon: VIRTIO_BALLOON_F_CMD_VQ
From: Dave Hansen @ 2017-06-12 14:07 UTC (permalink / raw)
To: Wei Wang, virtio-dev, linux-kernel, qemu-devel, virtualization,
kvm, linux-mm, mst, david, cornelia.huck, akpm, mgorman, aarcange,
amit.shah, pbonzini, liliang.opensource
In-Reply-To: <1497004901-30593-7-git-send-email-wei.w.wang@intel.com>
On 06/09/2017 03:41 AM, Wei Wang wrote:
> + for_each_populated_zone(zone) {
> + for (order = MAX_ORDER - 1; order > 0; order--) {
> + for (migratetype = 0; migratetype < MIGRATE_TYPES;
> + migratetype++) {
> + do {
> + ret = report_unused_page_block(zone,
> + order, migratetype, &page);
> + if (!ret) {
> + pfn = (u64)page_to_pfn(page);
> + add_one_chunk(vb, vq,
> + PAGE_CHNUK_UNUSED_PAGE,
> + pfn << VIRTIO_BALLOON_PFN_SHIFT,
> + (u64)(1 << order) *
> + VIRTIO_BALLOON_PAGES_PER_PAGE);
> + }
> + } while (!ret);
> + }
> + }
> + }
This is pretty unreadable. Please add some indentation. If you go
over 80 cols, then you might need to break this up into a separate
function. But, either way, it can't be left like this.
^ permalink raw reply
* [PATCH v1] virtio_blk: Use sysfs_match_string() helper
From: Andy Shevchenko @ 2017-06-09 12:07 UTC (permalink / raw)
To: virtualization; +Cc: Andy Shevchenko, Michael S. Tsirkin
Use sysfs_match_string() helper instead of open coded variant.
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/block/virtio_blk.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 553cc4c542b4..0e707b8cce9d 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -541,12 +541,9 @@ virtblk_cache_type_store(struct device *dev, struct device_attribute *attr,
int i;
BUG_ON(!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_CONFIG_WCE));
- for (i = ARRAY_SIZE(virtblk_cache_types); --i >= 0; )
- if (sysfs_streq(buf, virtblk_cache_types[i]))
- break;
-
+ i = sysfs_match_string(virtblk_cache_types, buf);
if (i < 0)
- return -EINVAL;
+ return i;
virtio_cwrite8(vdev, offsetof(struct virtio_blk_config, wce), i);
virtblk_update_cache_mode(vdev);
--
2.11.0
^ permalink raw reply related
* RE: [PATCH v11 0/6] Virtio-balloon Enhancement
From: Wang, Wei W @ 2017-06-09 11:18 UTC (permalink / raw)
To: virtio-dev@lists.oasis-open.org, linux-kernel@vger.kernel.org,
qemu-devel@nongnu.org, virtualization@lists.linux-foundation.org,
kvm@vger.kernel.org, linux-mm@kvack.org, mst@redhat.com,
david@redhat.com, Hansen, Dave, cornelia.huck@de.ibm.com,
akpm@linux-foundation.org, mgorman@techsingularity.net,
aarcange@redhat.com, amit.shah@redhat.com, pbonzini@redhat.com,
liliang.opensource@gmail.com
In-Reply-To: <1497004901-30593-1-git-send-email-wei.w.wang@intel.com>
On Friday, June 9, 2017 6:42 PM, Wang, Wei W wrote:
> To: virtio-dev@lists.oasis-open.org; linux-kernel@vger.kernel.org; qemu-
> devel@nongnu.org; virtualization@lists.linux-foundation.org;
> kvm@vger.kernel.org; linux-mm@kvack.org; mst@redhat.com;
> david@redhat.com; Hansen, Dave <dave.hansen@intel.com>;
> cornelia.huck@de.ibm.com; akpm@linux-foundation.org;
> mgorman@techsingularity.net; aarcange@redhat.com; amit.shah@redhat.com;
> pbonzini@redhat.com; Wang, Wei W <wei.w.wang@intel.com>;
> liliang.opensource@gmail.com
> Subject: [PATCH v11 0/6] Virtio-balloon Enhancement
>
> This patch series enhances the existing virtio-balloon with the following new
> features:
> 1) fast ballooning: transfer ballooned pages between the guest and host in
> chunks, instead of one by one; and
> 2) cmdq: a new virtqueue to send commands between the device and driver.
> Currently, it supports commands to report memory stats (replace the old statq
> mechanism) and report guest unused pages.
v10->v11 changes:
1) virtio_balloon: use vring_desc to describe a chunk;
2) virtio_ring: support to add an indirect desc table to virtqueue;
3) virtio_balloon: use cmdq to report guest memory statistics.
>
> Liang Li (1):
> virtio-balloon: deflate via a page list
>
> Wei Wang (5):
> virtio-balloon: coding format cleanup
> virtio-balloon: VIRTIO_BALLOON_F_PAGE_CHUNKS
> mm: function to offer a page block on the free list
> mm: export symbol of next_zone and first_online_pgdat
> virtio-balloon: VIRTIO_BALLOON_F_CMD_VQ
>
> drivers/virtio/virtio_balloon.c | 781 ++++++++++++++++++++++++++++++++--
> --
> drivers/virtio/virtio_ring.c | 120 +++++-
> include/linux/mm.h | 5 +
> include/linux/virtio.h | 7 +
> include/uapi/linux/virtio_balloon.h | 14 +
> include/uapi/linux/virtio_ring.h | 3 +
> mm/mmzone.c | 2 +
> mm/page_alloc.c | 91 +++++
> 8 files changed, 950 insertions(+), 73 deletions(-)
>
> --
> 2.7.4
^ permalink raw reply
* [PATCH v11 6/6] virtio-balloon: VIRTIO_BALLOON_F_CMD_VQ
From: Wei Wang @ 2017-06-09 10:41 UTC (permalink / raw)
To: virtio-dev, linux-kernel, qemu-devel, virtualization, kvm,
linux-mm, mst, david, dave.hansen, cornelia.huck, akpm, mgorman,
aarcange, amit.shah, pbonzini, wei.w.wang, liliang.opensource
In-Reply-To: <1497004901-30593-1-git-send-email-wei.w.wang@intel.com>
Add a new vq, cmdq, to handle requests between the device and driver.
This patch implements two commands send from the device and handled in
the driver.
1) cmd VIRTIO_BALLOON_CMDQ_REPORT_STATS: this command is used to report
the guest memory statistics to the host. The stats_vq mechanism is not
used when the cmdq mechanism is enabled.
2) cmd VIRTIO_BALLOON_CMDQ_REPORT_UNUSED_PAGES: this command is used to
report the guest unused pages to the host.
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
drivers/virtio/virtio_balloon.c | 363 ++++++++++++++++++++++++++++++++----
include/uapi/linux/virtio_balloon.h | 13 ++
2 files changed, 337 insertions(+), 39 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 0cf945c..4ac90a5 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -51,6 +51,10 @@ MODULE_PARM_DESC(oom_pages, "pages to free on OOM");
static struct vfsmount *balloon_mnt;
#endif
+/* Types of pages to chunk */
+#define PAGE_CHUNK_TYPE_BALLOON 0 /* Chunk of inflate/deflate pages */
+#define PAGE_CHNUK_UNUSED_PAGE 1 /* Chunk of unused pages */
+
/* The size of one page_bmap used to record inflated/deflated pages. */
#define VIRTIO_BALLOON_PAGE_BMAP_SIZE (8 * PAGE_SIZE)
/*
@@ -81,12 +85,25 @@ struct virtio_balloon_page_chunk {
unsigned long *page_bmap[VIRTIO_BALLOON_PAGE_BMAP_MAX_NUM];
};
+struct virtio_balloon_cmdq_unused_page {
+ struct virtio_balloon_cmdq_hdr hdr;
+ struct vring_desc *desc_table;
+ /* Number of added descriptors */
+ unsigned int num;
+};
+
+struct virtio_balloon_cmdq_stats {
+ struct virtio_balloon_cmdq_hdr hdr;
+ struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
+};
+
struct virtio_balloon {
struct virtio_device *vdev;
- struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
+ struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *cmd_vq;
/* The balloon servicing is delegated to a freezable workqueue. */
struct work_struct update_balloon_stats_work;
+ struct work_struct cmdq_handle_work;
struct work_struct update_balloon_size_work;
/* Prevent updating balloon when it is being canceled. */
@@ -115,8 +132,10 @@ struct virtio_balloon {
unsigned int num_pfns;
__virtio32 pfns[VIRTIO_BALLOON_ARRAY_PFNS_MAX];
- /* Memory statistics */
- struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
+ /* Cmdq msg buffer for memory statistics */
+ struct virtio_balloon_cmdq_stats cmdq_stats;
+ /* Cmdq msg buffer for reporting ununsed pages */
+ struct virtio_balloon_cmdq_unused_page cmdq_unused_page;
/* To register callback in oom notifier call chain */
struct notifier_block nb;
@@ -208,31 +227,77 @@ static void clear_page_bmap(struct virtio_balloon *vb,
VIRTIO_BALLOON_PAGE_BMAP_SIZE);
}
-static void send_page_chunks(struct virtio_balloon *vb, struct virtqueue *vq)
+static void send_page_chunks(struct virtio_balloon *vb, struct virtqueue *vq,
+ int type, bool busy_wait)
{
- unsigned int len, num;
- struct vring_desc *desc = vb->balloon_page_chunk.desc_table;
+ unsigned int len, *num, reset_num;
+ struct vring_desc *desc;
+
+ switch (type) {
+ case PAGE_CHUNK_TYPE_BALLOON:
+ desc = vb->balloon_page_chunk.desc_table;
+ num = &vb->balloon_page_chunk.chunk_num;
+ reset_num = 0;
+ break;
+ case PAGE_CHNUK_UNUSED_PAGE:
+ desc = vb->cmdq_unused_page.desc_table;
+ num = &vb->cmdq_unused_page.num;
+ /*
+ * The first desc is used for the cmdq_hdr, so chunks will be
+ * added from the second desc.
+ */
+ reset_num = 1;
+ break;
+ default:
+ dev_warn(&vb->vdev->dev, "%s: unknown page chunk type %d\n",
+ __func__, type);
+ return;
+ }
- num = vb->balloon_page_chunk.chunk_num;
- if (!virtqueue_indirect_desc_table_add(vq, desc, num)) {
+ if (!virtqueue_indirect_desc_table_add(vq, desc, *num)) {
virtqueue_kick(vq);
- wait_event(vb->acked, virtqueue_get_buf(vq, &len));
- vb->balloon_page_chunk.chunk_num = 0;
+ if (busy_wait)
+ while (!virtqueue_get_buf(vq, &len) &&
+ !virtqueue_is_broken(vq))
+ cpu_relax();
+ else
+ wait_event(vb->acked, virtqueue_get_buf(vq, &len));
+ /*
+ * Now, the descriptor have been delivered to the host. Reset
+ * the field in the structure that records the number of added
+ * descriptors, so that new added descriptor can be re-counted.
+ */
+ *num = reset_num;
}
}
/* Add a chunk to the buffer. */
static void add_one_chunk(struct virtio_balloon *vb, struct virtqueue *vq,
- u64 base_addr, u32 size)
+ int type, u64 base_addr, u32 size)
{
- unsigned int *num = &vb->balloon_page_chunk.chunk_num;
- struct vring_desc *desc = &vb->balloon_page_chunk.desc_table[*num];
+ unsigned int *num;
+ struct vring_desc *desc;
+
+ switch (type) {
+ case PAGE_CHUNK_TYPE_BALLOON:
+ num = &vb->balloon_page_chunk.chunk_num;
+ desc = &vb->balloon_page_chunk.desc_table[*num];
+ break;
+ case PAGE_CHNUK_UNUSED_PAGE:
+ num = &vb->cmdq_unused_page.num;
+ desc = &vb->cmdq_unused_page.desc_table[*num];
+ break;
+ default:
+ dev_warn(&vb->vdev->dev, "%s: chunk %d of unknown pages\n",
+ __func__, type);
+ return;
+ }
desc->addr = cpu_to_virtio64(vb->vdev, base_addr);
desc->len = cpu_to_virtio32(vb->vdev, size);
*num += 1;
if (*num == VIRTIO_BALLOON_MAX_PAGE_CHUNKS)
- send_page_chunks(vb, vq);
+ send_page_chunks(vb, vq, type, false);
}
static void convert_bmap_to_chunks(struct virtio_balloon *vb,
@@ -264,7 +329,8 @@ static void convert_bmap_to_chunks(struct virtio_balloon *vb,
chunk_base_addr = (pfn_start + next_one) <<
VIRTIO_BALLOON_PFN_SHIFT;
if (chunk_size) {
- add_one_chunk(vb, vq, chunk_base_addr, chunk_size);
+ add_one_chunk(vb, vq, PAGE_CHUNK_TYPE_BALLOON,
+ chunk_base_addr, chunk_size);
pos += next_zero + 1;
}
}
@@ -311,7 +377,7 @@ static void tell_host_from_page_bmap(struct virtio_balloon *vb,
pfn_num);
}
if (vb->balloon_page_chunk.chunk_num > 0)
- send_page_chunks(vb, vq);
+ send_page_chunks(vb, vq, PAGE_CHUNK_TYPE_BALLOON, false);
}
static void set_page_pfns(struct virtio_balloon *vb,
@@ -516,8 +582,8 @@ static inline void update_stat(struct virtio_balloon *vb, int idx,
u16 tag, u64 val)
{
BUG_ON(idx >= VIRTIO_BALLOON_S_NR);
- vb->stats[idx].tag = cpu_to_virtio16(vb->vdev, tag);
- vb->stats[idx].val = cpu_to_virtio64(vb->vdev, val);
+ vb->cmdq_stats.stats[idx].tag = cpu_to_virtio16(vb->vdev, tag);
+ vb->cmdq_stats.stats[idx].val = cpu_to_virtio64(vb->vdev, val);
}
#define pages_to_bytes(x) ((u64)(x) << PAGE_SHIFT)
@@ -582,7 +648,8 @@ static void stats_handle_request(struct virtio_balloon *vb)
vq = vb->stats_vq;
if (!virtqueue_get_buf(vq, &len))
return;
- sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats);
+ sg_init_one(&sg, vb->cmdq_stats.stats,
+ sizeof(vb->cmdq_stats.stats[0]) * num_stats);
virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
virtqueue_kick(vq);
}
@@ -686,43 +753,216 @@ static void update_balloon_size_func(struct work_struct *work)
queue_work(system_freezable_wq, work);
}
+static void cmdq_handle_stats(struct virtio_balloon *vb)
+{
+ struct scatterlist sg;
+ unsigned int num_stats;
+
+ spin_lock(&vb->stop_update_lock);
+ if (!vb->stop_update) {
+ num_stats = update_balloon_stats(vb);
+ sg_init_one(&sg, &vb->cmdq_stats,
+ sizeof(struct virtio_balloon_cmdq_hdr) +
+ sizeof(struct virtio_balloon_stat) * num_stats);
+ virtqueue_add_outbuf(vb->cmd_vq, &sg, 1, vb, GFP_KERNEL);
+ virtqueue_kick(vb->cmd_vq);
+ }
+ spin_unlock(&vb->stop_update_lock);
+}
+
+/*
+ * The header part of the message buffer is given to the device to send a
+ * command to the driver.
+ */
+static void host_cmd_buf_add(struct virtio_balloon *vb,
+ struct virtio_balloon_cmdq_hdr *hdr)
+{
+ struct scatterlist sg;
+
+ hdr->flags = 0;
+ sg_init_one(&sg, hdr, VIRTIO_BALLOON_CMDQ_HDR_SIZE);
+
+ if (virtqueue_add_inbuf(vb->cmd_vq, &sg, 1, hdr, GFP_KERNEL) < 0) {
+ __virtio_clear_bit(vb->vdev,
+ VIRTIO_BALLOON_F_CMD_VQ);
+ dev_warn(&vb->vdev->dev, "%s: add miscq msg buf err\n",
+ __func__);
+ return;
+ }
+
+ virtqueue_kick(vb->cmd_vq);
+}
+
+static void cmdq_handle_unused_pages(struct virtio_balloon *vb)
+{
+ struct virtqueue *vq = vb->cmd_vq;
+ struct vring_desc *hdr_desc = &vb->cmdq_unused_page.desc_table[0];
+ unsigned long hdr_pa;
+ unsigned int order = 0, migratetype = 0;
+ struct zone *zone = NULL;
+ struct page *page = NULL;
+ u64 pfn;
+ int ret = 0;
+
+ /* Put the hdr to the first desc */
+ hdr_pa = virt_to_phys((void *)&vb->cmdq_unused_page.hdr);
+ hdr_desc->addr = cpu_to_virtio64(vb->vdev, hdr_pa);
+ hdr_desc->len = cpu_to_virtio32(vb->vdev,
+ sizeof(struct virtio_balloon_cmdq_hdr));
+ vb->cmdq_unused_page.num = 1;
+
+ for_each_populated_zone(zone) {
+ for (order = MAX_ORDER - 1; order > 0; order--) {
+ for (migratetype = 0; migratetype < MIGRATE_TYPES;
+ migratetype++) {
+ do {
+ ret = report_unused_page_block(zone,
+ order, migratetype, &page);
+ if (!ret) {
+ pfn = (u64)page_to_pfn(page);
+ add_one_chunk(vb, vq,
+ PAGE_CHNUK_UNUSED_PAGE,
+ pfn << VIRTIO_BALLOON_PFN_SHIFT,
+ (u64)(1 << order) *
+ VIRTIO_BALLOON_PAGES_PER_PAGE);
+ }
+ } while (!ret);
+ }
+ }
+ }
+
+ /* Set the cmd completion flag. */
+ vb->cmdq_unused_page.hdr.flags |=
+ cpu_to_le32(VIRTIO_BALLOON_CMDQ_F_COMPLETION);
+ send_page_chunks(vb, vq, PAGE_CHNUK_UNUSED_PAGE, true);
+}
+
+static void cmdq_handle(struct virtio_balloon *vb)
+{
+ struct virtqueue *vq;
+ struct virtio_balloon_cmdq_hdr *hdr;
+ unsigned int len;
+
+ vq = vb->cmd_vq;
+ while ((hdr = (struct virtio_balloon_cmdq_hdr *)
+ virtqueue_get_buf(vq, &len)) != NULL) {
+ switch (hdr->cmd) {
+ case VIRTIO_BALLOON_CMDQ_REPORT_STATS:
+ cmdq_handle_stats(vb);
+ break;
+ case VIRTIO_BALLOON_CMDQ_REPORT_UNUSED_PAGES:
+ cmdq_handle_unused_pages(vb);
+ break;
+ default:
+ dev_warn(&vb->vdev->dev, "%s: wrong cmd\n", __func__);
+ return;
+ }
+ /*
+ * Replenish all the command buffer to the device after a
+ * command is handled. This is for the convenience of the
+ * device to rewind the cmdq to get back all the command
+ * buffer after live migration.
+ */
+ host_cmd_buf_add(vb, &vb->cmdq_stats.hdr);
+ host_cmd_buf_add(vb, &vb->cmdq_unused_page.hdr);
+ }
+}
+
+static void cmdq_handle_work_func(struct work_struct *work)
+{
+ struct virtio_balloon *vb;
+
+ vb = container_of(work, struct virtio_balloon,
+ cmdq_handle_work);
+ cmdq_handle(vb);
+}
+
+static void cmdq_callback(struct virtqueue *vq)
+{
+ struct virtio_balloon *vb = vq->vdev->priv;
+
+ queue_work(system_freezable_wq, &vb->cmdq_handle_work);
+}
+
static int init_vqs(struct virtio_balloon *vb)
{
- struct virtqueue *vqs[3];
- vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_request };
- static const char * const names[] = { "inflate", "deflate", "stats" };
- int err, nvqs;
+ struct virtqueue **vqs;
+ vq_callback_t **callbacks;
+ const char **names;
+ int err = -ENOMEM;
+ int nvqs;
+
+ /* Inflateq and deflateq are used unconditionally */
+ nvqs = 2;
+
+ if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_CMD_VQ) ||
+ virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ))
+ nvqs++;
+
+ /* Allocate space for find_vqs parameters */
+ vqs = kcalloc(nvqs, sizeof(*vqs), GFP_KERNEL);
+ if (!vqs)
+ goto err_vq;
+ callbacks = kmalloc_array(nvqs, sizeof(*callbacks), GFP_KERNEL);
+ if (!callbacks)
+ goto err_callback;
+ names = kmalloc_array(nvqs, sizeof(*names), GFP_KERNEL);
+ if (!names)
+ goto err_names;
+
+ callbacks[0] = balloon_ack;
+ names[0] = "inflate";
+ callbacks[1] = balloon_ack;
+ names[1] = "deflate";
/*
- * We expect two virtqueues: inflate and deflate, and
- * optionally stat.
+ * The stats_vq is used only when cmdq is not supported (or disabled)
+ * by the device.
*/
- nvqs = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2;
- err = vb->vdev->config->find_vqs(vb->vdev, nvqs, vqs, callbacks, names,
- NULL);
- if (err)
- return err;
+ if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_CMD_VQ)) {
+ callbacks[2] = cmdq_callback;
+ names[2] = "cmdq";
+ } else if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
+ callbacks[2] = stats_request;
+ names[2] = "stats";
+ }
+ err = vb->vdev->config->find_vqs(vb->vdev, nvqs, vqs, callbacks,
+ names, NULL);
+ if (err)
+ goto err_find;
vb->inflate_vq = vqs[0];
vb->deflate_vq = vqs[1];
- if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
+
+ if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_CMD_VQ)) {
+ vb->cmd_vq = vqs[2];
+ /* Prime the cmdq with the header buffer. */
+ host_cmd_buf_add(vb, &vb->cmdq_stats.hdr);
+ host_cmd_buf_add(vb, &vb->cmdq_unused_page.hdr);
+ } else if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
struct scatterlist sg;
- unsigned int num_stats;
- vb->stats_vq = vqs[2];
+ vb->stats_vq = vqs[2];
/*
* Prime this virtqueue with one buffer so the hypervisor can
* use it to signal us later (it can't be broken yet!).
*/
- num_stats = update_balloon_stats(vb);
-
- sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats);
+ sg_init_one(&sg, vb->cmdq_stats.stats,
+ sizeof(vb->cmdq_stats.stats));
if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)
< 0)
BUG();
virtqueue_kick(vb->stats_vq);
}
- return 0;
+
+err_find:
+ kfree(names);
+err_names:
+ kfree(callbacks);
+err_callback:
+ kfree(vqs);
+err_vq:
+ return err;
}
#ifdef CONFIG_BALLOON_COMPACTION
@@ -730,7 +970,8 @@ static int init_vqs(struct virtio_balloon *vb)
static void tell_host_one_page(struct virtio_balloon *vb,
struct virtqueue *vq, struct page *page)
{
- add_one_chunk(vb, vq, page_to_pfn(page) << VIRTIO_BALLOON_PFN_SHIFT,
+ add_one_chunk(vb, vq, PAGE_CHUNK_TYPE_BALLOON,
+ page_to_pfn(page) << VIRTIO_BALLOON_PFN_SHIFT,
VIRTIO_BALLOON_PAGES_PER_PAGE);
}
@@ -865,6 +1106,40 @@ static int balloon_page_chunk_init(struct virtio_balloon *vb)
return -ENOMEM;
}
+/*
+ * Each type of command is handled one in-flight each time. So, we allocate
+ * one message buffer for each type of command. The header part of the message
+ * buffer will be offered to the device, so that the device can send a command
+ * using the corresponding command buffer to the driver later.
+ */
+static int cmdq_init(struct virtio_balloon *vb)
+{
+ vb->cmdq_unused_page.desc_table = alloc_indirect(vb->vdev,
+ VIRTIO_BALLOON_MAX_PAGE_CHUNKS,
+ GFP_KERNEL);
+ if (!vb->cmdq_unused_page.desc_table) {
+ dev_warn(&vb->vdev->dev, "%s: failed\n", __func__);
+ __virtio_clear_bit(vb->vdev,
+ VIRTIO_BALLOON_F_CMD_VQ);
+ return -ENOMEM;
+ }
+ vb->cmdq_unused_page.num = 0;
+
+ /*
+ * The header is initialized to let the device know which type of
+ * command buffer it receives. The device will later use a buffer
+ * according to the type of command that it needs to send.
+ */
+ vb->cmdq_stats.hdr.cmd = VIRTIO_BALLOON_CMDQ_REPORT_STATS;
+ vb->cmdq_stats.hdr.flags = 0;
+ vb->cmdq_unused_page.hdr.cmd = VIRTIO_BALLOON_CMDQ_REPORT_UNUSED_PAGES;
+ vb->cmdq_unused_page.hdr.flags = 0;
+
+ INIT_WORK(&vb->cmdq_handle_work, cmdq_handle_work_func);
+
+ return 0;
+}
+
static int virtballoon_validate(struct virtio_device *vdev)
{
struct virtio_balloon *vb = NULL;
@@ -883,6 +1158,11 @@ static int virtballoon_validate(struct virtio_device *vdev)
goto err_page_chunk;
}
+ if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_CMD_VQ)) {
+ err = cmdq_init(vb);
+ if (err < 0)
+ goto err_vb;
+ }
return 0;
err_page_chunk:
@@ -902,7 +1182,10 @@ static int virtballoon_probe(struct virtio_device *vdev)
return -EINVAL;
}
- INIT_WORK(&vb->update_balloon_stats_work, update_balloon_stats_func);
+ if (!virtio_has_feature(vdev, VIRTIO_BALLOON_F_CMD_VQ) &&
+ virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ))
+ INIT_WORK(&vb->update_balloon_stats_work,
+ update_balloon_stats_func);
INIT_WORK(&vb->update_balloon_size_work, update_balloon_size_func);
spin_lock_init(&vb->stop_update_lock);
vb->stop_update = false;
@@ -980,6 +1263,7 @@ static void virtballoon_remove(struct virtio_device *vdev)
spin_unlock_irq(&vb->stop_update_lock);
cancel_work_sync(&vb->update_balloon_size_work);
cancel_work_sync(&vb->update_balloon_stats_work);
+ cancel_work_sync(&vb->cmdq_handle_work);
remove_common(vb);
free_page_bmap(vb);
@@ -1029,6 +1313,7 @@ static unsigned int features[] = {
VIRTIO_BALLOON_F_STATS_VQ,
VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
VIRTIO_BALLOON_F_PAGE_CHUNKS,
+ VIRTIO_BALLOON_F_CMD_VQ,
};
static struct virtio_driver virtio_balloon_driver = {
diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
index 5ed3c7b..cb66c1a 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -35,6 +35,7 @@
#define VIRTIO_BALLOON_F_STATS_VQ 1 /* Memory Stats virtqueue */
#define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon on OOM */
#define VIRTIO_BALLOON_F_PAGE_CHUNKS 3 /* Inflate/Deflate pages in chunks */
+#define VIRTIO_BALLOON_F_CMD_VQ 4 /* Command virtqueue */
/* Size of a PFN in the balloon interface. */
#define VIRTIO_BALLOON_PFN_SHIFT 12
@@ -83,4 +84,16 @@ struct virtio_balloon_stat {
__virtio64 val;
} __attribute__((packed));
+/* Use the memory of a vring_desc to place the cmdq header */
+#define VIRTIO_BALLOON_CMDQ_HDR_SIZE sizeof(struct vring_desc)
+
+struct virtio_balloon_cmdq_hdr {
+#define VIRTIO_BALLOON_CMDQ_REPORT_STATS 0
+#define VIRTIO_BALLOON_CMDQ_REPORT_UNUSED_PAGES 1
+ __le32 cmd;
+/* Flag to indicate the completion of handling a command */
+#define VIRTIO_BALLOON_CMDQ_F_COMPLETION 1
+ __le32 flags;
+};
+
#endif /* _LINUX_VIRTIO_BALLOON_H */
--
2.7.4
^ permalink raw reply related
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