Re: [GIT PULL v2] vhost: cleanups and fixes
The pull request you sent on Mon, 20 Apr 2020 16:00:01 -0400: > https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/189522da8b3a796d56d802e067d591d27f40 Thank you! -- Deet-doot-dot, I am a bot. https://korg.wiki.kernel.org/userdoc/prtracker ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1] drm/bochs: fix an issue of ioremap() leak
On Tue, Apr 21, 2020 at 7:45 PM Dejin Zheng wrote:
>
> It forgot to call bochs_hw_fini() to release related resources when
> bochs_pci_probe() fail. eg: io virtual address get by ioremap().
Good start, although I think the best is to switch this driver to use
pcim_*() functions and drop tons of legacy code.
> Fixes: 81da8c3b8d3df6 ("drm/bochs: add drm_driver.release callback.")
> CC: Andy Shevchenko
> Signed-off-by: Dejin Zheng
> ---
> drivers/gpu/drm/bochs/bochs_drv.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/bochs/bochs_drv.c
> b/drivers/gpu/drm/bochs/bochs_drv.c
> index addb0568c1af..210a60135c8a 100644
> --- a/drivers/gpu/drm/bochs/bochs_drv.c
> +++ b/drivers/gpu/drm/bochs/bochs_drv.c
> @@ -138,6 +138,7 @@ static int bochs_pci_probe(struct pci_dev *pdev,
> return ret;
>
> err_unload:
> + bochs_hw_fini(dev);
> bochs_unload(dev);
> err_free_dev:
> drm_dev_put(dev);
> --
> 2.25.0
>
--
With Best Regards,
Andy Shevchenko
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net] vsock/virtio: postpone packet delivery to monitoring devices
On Tue, Apr 21, 2020 at 04:42:46PM +0100, Stefan Hajnoczi wrote: > On Tue, Apr 21, 2020 at 11:25:27AM +0200, Stefano Garzarella wrote: > > We delivering packets to monitoring devices, before to check if > > the virtqueue has enough space. > > "We [are] delivering packets" and "before to check" -> "before > checking". Perhaps it can be rewritten as: > > Packets are delivered to monitoring devices before checking if the > virtqueue has enough space. > Yeah, it is better :-) > > > > If the virtqueue is full, the transmitting packet is queued up > > and it will be sent in the next iteration. This causes the same > > packet to be delivered multiple times to monitoring devices. > > > > This patch fixes this issue, postponing the packet delivery > > to monitoring devices, only when it is properly queued in the > > s/,// > > > virqueue. > > s/virqueue/virtqueue/ > Thanks, I'll fix in the v2! > > @@ -137,6 +135,11 @@ virtio_transport_send_pkt_work(struct work_struct > > *work) > > break; > > } > > > > + /* Deliver to monitoring devices all correctly transmitted > > +* packets. > > +*/ > > + virtio_transport_deliver_tap_pkt(pkt); > > + > > The device may see the tx packet and therefore receive a reply to it > before we can call virtio_transport_deliver_tap_pkt(). Does this mean > that replies can now appear in the packet capture before the transmitted > packet? hmm, you are right! And the same thing can already happen in vhost-vsock where we call virtio_transport_deliver_tap_pkt() after the vhost_add_used(), right? The vhost-vsock case can be fixed in a simple way, but here do you think we should serialize them? (e.g. mutex, spinlock) In this case I'm worried about performance. Or is there some virtqueue API to check availability? Thanks, Stefano ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [virtio-dev] Re: [PATCH] virtio-balloon: Disable free page hinting/reporting if page poison is disabled
On Tue, Apr 21, 2020 at 8:18 AM David Hildenbrand wrote: > > >> > >> That leaves us with either > >> > >> 1. "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT might get replaced > >> by zero page. However, as soon as the page is written by the guest (even > >> before the hinting request was processed by the host), the modified page > >> will stay - whereby the unwritten parts might either be from the old, or > >> from the zero page." - a QEMU BUG. > > > > How is this a bug? This is the behavior you would see with the current > > balloon driver. When you put a page into a balloon it has the option > > to either madvise it away, defer it, or just skip it because he > > balloon is disabled. Is the "zero page" a typo? If it was > > uninitialized data that would be a bug, but I don't see how a zero > > page or the old data would be a bug. > > Sorry, I meant if this was the original design idea of hinting, then we > would have a QEMU BUG as of now, as we might get get uninitialized data. > Makes sense? Yes, that makes sense. So the bug is that we aren't doing this right now. > > > >>> > The current QEMU implementation would be to simply migrate all hinted > pages. In the future we could optimize. Not sure if it's worth the > trouble. > >>> > >>> So the trick for me is how best to go about sorting this all out. > >>> There are two ways I see of doing it. > >>> > >>> The first approach would be to just assume that hinting should be > >>> disassociated from poisoning. If I go that route that would more > >>> closely match up with what happened in QEMU. The downside is that it > >>> locks in the unmodified/uninitialized behavior and would require pages > >>> to be rewritten when they come out of the balloon. However this is the > >>> behavior we have now so it would only require specification > >>> documentation changes. > >> > >> Right now, for simplicity, I prefer this and define > >> VIRTIO_BALLOON_F_PAGE_POISON only for explicit deflation (balloon > >> deflation via the deflate queue) and implicit deflation > >> (VIRTIO_BALLOON_F_REPORTING). > > > > I don't recall us talking about the explicit deflation case before. > > The interesting part is that drivers/virtio/virtio_balloon.c mentions: > > "Let the hypervisor know that we are expecting a specific value to be > written back in balloon pages.". > > But I just realized that you introduced this comment, not the original > VIRTIO_BALLOON_F_PAGE_POISON commit. > > Should this have been "in reported pages when implicitly deflating them > by reusing them." or sth. like that? Yeah, probably. I should have probably used "reported" instead of "balloon" in the comment. > > What is the expectation there? I assume we are saying either > > poison_val or unmodified? If so I would think the inflate case makes > > much more sense as that is where the madvise is called that will > > discard the data. If so it would be pretty easy to just add a check > > for the poison value to the same spot we check > > qemu_balloon_is_inhibited. > > Okay, we have basically no idea what was the intention of > VIRTIO_BALLOON_F_PAGE_POISON with basic deflation/inflation as well. So > I think we can define what suits us. > > On the deflate path, we could always simply fill with poison_val. But > there are nasty corner cases (esp. no VIRTIO_BALLOON_F_MUST_TELL_HOST). > > What would be your suggestion? Also don't care about > VIRTIO_BALLOON_F_PAGE_POISON on ordinary inflation/deflation? At this > point, I think this makes sense. That is kind of what I was thinking. The problem is that once again the current implementation works when page poisoning is enabled. Us disabling that wouldn't make much sense. The whole thing with the reporting is that we are essentially just ballooning in place. What we may do at some point in the future would be to add an additional feature bit to do that for the standard balloon/hinting case. Then when that is set, and we know the contents won't match we can then just skip the madvise or hinting calls. That way it becomes an opt-in which is what the poison was supposed to be, but wasn't because the QEMU side was never implemented. In the meantime I still have to make more changes to my QEMU patch set. The way the config_size logic is implemented is somewhat of a pain when you factor in the way the host_features and poison were handled. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net] vsock/virtio: postpone packet delivery to monitoring devices
On Tue, Apr 21, 2020 at 11:25:27AM +0200, Stefano Garzarella wrote: > We delivering packets to monitoring devices, before to check if > the virtqueue has enough space. "We [are] delivering packets" and "before to check" -> "before checking". Perhaps it can be rewritten as: Packets are delivered to monitoring devices before checking if the virtqueue has enough space. > > If the virtqueue is full, the transmitting packet is queued up > and it will be sent in the next iteration. This causes the same > packet to be delivered multiple times to monitoring devices. > > This patch fixes this issue, postponing the packet delivery > to monitoring devices, only when it is properly queued in the s/,// > virqueue. s/virqueue/virtqueue/ > @@ -137,6 +135,11 @@ virtio_transport_send_pkt_work(struct work_struct *work) > break; > } > > + /* Deliver to monitoring devices all correctly transmitted > + * packets. > + */ > + virtio_transport_deliver_tap_pkt(pkt); > + The device may see the tx packet and therefore receive a reply to it before we can call virtio_transport_deliver_tap_pkt(). Does this mean that replies can now appear in the packet capture before the transmitted packet? signature.asc Description: PGP signature ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [virtio-dev] Re: [PATCH] virtio-balloon: Disable free page hinting/reporting if page poison is disabled
>> >> That leaves us with either >> >> 1. "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT might get replaced >> by zero page. However, as soon as the page is written by the guest (even >> before the hinting request was processed by the host), the modified page >> will stay - whereby the unwritten parts might either be from the old, or >> from the zero page." - a QEMU BUG. > > How is this a bug? This is the behavior you would see with the current > balloon driver. When you put a page into a balloon it has the option > to either madvise it away, defer it, or just skip it because he > balloon is disabled. Is the "zero page" a typo? If it was > uninitialized data that would be a bug, but I don't see how a zero > page or the old data would be a bug. Sorry, I meant if this was the original design idea of hinting, then we would have a QEMU BUG as of now, as we might get get uninitialized data. Makes sense? [...] > >>> The current QEMU implementation would be to simply migrate all hinted pages. In the future we could optimize. Not sure if it's worth the trouble. >>> >>> So the trick for me is how best to go about sorting this all out. >>> There are two ways I see of doing it. >>> >>> The first approach would be to just assume that hinting should be >>> disassociated from poisoning. If I go that route that would more >>> closely match up with what happened in QEMU. The downside is that it >>> locks in the unmodified/uninitialized behavior and would require pages >>> to be rewritten when they come out of the balloon. However this is the >>> behavior we have now so it would only require specification >>> documentation changes. >> >> Right now, for simplicity, I prefer this and define >> VIRTIO_BALLOON_F_PAGE_POISON only for explicit deflation (balloon >> deflation via the deflate queue) and implicit deflation >> (VIRTIO_BALLOON_F_REPORTING). > > I don't recall us talking about the explicit deflation case before. The interesting part is that drivers/virtio/virtio_balloon.c mentions: "Let the hypervisor know that we are expecting a specific value to be written back in balloon pages.". But I just realized that you introduced this comment, not the original VIRTIO_BALLOON_F_PAGE_POISON commit. Should this have been "in reported pages when implicitly deflating them by reusing them." or sth. like that? > What is the expectation there? I assume we are saying either > poison_val or unmodified? If so I would think the inflate case makes > much more sense as that is where the madvise is called that will > discard the data. If so it would be pretty easy to just add a check > for the poison value to the same spot we check > qemu_balloon_is_inhibited. Okay, we have basically no idea what was the intention of VIRTIO_BALLOON_F_PAGE_POISON with basic deflation/inflation as well. So I think we can define what suits us. On the deflate path, we could always simply fill with poison_val. But there are nasty corner cases (esp. no VIRTIO_BALLOON_F_MUST_TELL_HOST). What would be your suggestion? Also don't care about VIRTIO_BALLOON_F_PAGE_POISON on ordinary inflation/deflation? At this point, I think this makes sense. -- Thanks, David / dhildenb ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [virtio-dev] Re: [PATCH] virtio-balloon: Disable free page hinting/reporting if page poison is disabled
On Tue, Apr 21, 2020 at 12:29 AM David Hildenbrand wrote: > > >>> 2. Can we assume that the guest will always rewrite the page after the > >>> deflate in the case of init_on_free or poison? > >> > >> Depends on what we think is the right way to do - IOW if we think "some > >> other content" as mentioned above is a BUG or not. > > > > So I wouldn't consider it a but as the zero page probably doesn't > > apply. We are basically just indicating we don't care about the > > contents, we aren't giving it a value. At least that is how I see it > > working based on how it was implemented. > > > >>> 3. Can we assume that free page hinting will always function as a > >>> balloon setup, so no moving it over to a page reporting type setup? > >> > >> I think we have to define the valid semantics. That implies what would > >> be valid to do with it. Unfortunately, we have to reverse-engineer here. > > > > So at this point a "hinted" page is a page that can essentially > > transition to uninitialized while it is sitting in the balloon. I > > suspect that is how we are going to have to treat it since that is the > > behavior that it has right now. > > At least it's not what Michael initially thought should be done - IIRC. > > "We need to remember that the whole HINT thing is not a mandate for host > to corrupt memory. It's just some info about page use guest gives host. > If host corrupts memory it's broken ..."). > > So the question remains: Does QEMU have a BUG or do we actually allow to > corrupt guest memory. > > That leaves us with either > > 1. "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT might get replaced > by zero page. However, as soon as the page is written by the guest (even > before the hinting request was processed by the host), the modified page > will stay - whereby the unwritten parts might either be from the old, or > from the zero page." - a QEMU BUG. How is this a bug? This is the behavior you would see with the current balloon driver. When you put a page into a balloon it has the option to either madvise it away, defer it, or just skip it because he balloon is disabled. Is the "zero page" a typo? If it was uninitialized data that would be a bug, but I don't see how a zero page or the old data would be a bug. The caveat for the hinting is that if the page is modified from the point it is placed on the ring the dirty flag will be enforced for it and will not be skipped as it will be captured in the next bitmap sync. > 2. "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT are considered > unused and will contain an undefined/uninitialized content once hinted. > As soon as the page is written by the guest (even before the hinting > request was processed by the host), the modified page will stay. The > guest should reinitialized the full page in case it cares about the > actual content (e.g., page poisoning)." > > > I tend to favor 2 - although it basically leaves no room for future > improvement regarding skipping re-initialization in the guest after > migration. I agree. The main advantage would be that we get to keep all of the existing functionality and wouldn't have to shut things down for poison being enabled. However we are limited in that any future design won't be able to skip over having to have the guest re-poison the pages. However making changes to behave more like 1 would break existing use cases since right now page poisoning can be enabled and the guest can make it work. If we refactored QEMU to make 1 work then we would lose that. > >>> > >>> If we assume the above 3 items then there isn't any point in worrying > >>> about poison when it comes to free page hinting. It doesn't make sense > >>> to since they essentially negate it. As such I could go through this > >>> patch and the QEMU patches and clean up any associations since the to > >>> are not really tied together in any way. > >> > >> The big question is, if we want to support VIRTIO_BALLOON_F_PAGE_POISON > >> with free page hinting. e.g.,: > >> > >> "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT might get replaced by > >> a page full of X. However, as soon as the page is written by the guest > >> (even before the hinting request was processed by the host), the > >> modified page will stay - whereby the unwritten parts might either be > >> from the old, or from a page filled with X. Without > >> VIRTIO_BALLOON_F_PAGE_POISON, X is zero, otherwise it is poison_val." > > [...] > > > > > With the VIRTIO_BALLOON_F_PAGE_POISON we could make it so that when > > the page comes out of the balloon it is either unmodified or it is > > poison_val. Without the VIRTIO_BALLOON_F_PAGE_POISON feature present > > you cannot make that guarantee and are stuck with the page being > > treated as either unmodified or uninitialized memory. > > While it would be possible, I doubt it will be easy to implement, and I > still wonder if we should really care about optimizing an undocumented > feature that takes three people to figure out the
Re: [PATCH v3] virtio: force spec specified alignment on types
On Tue, Apr 21, 2020 at 10:39:19AM +0800, Jason Wang wrote:
>
> On 2020/4/21 上午4:46, Michael S. Tsirkin wrote:
> > The ring element addresses are passed between components with different
> > alignments assumptions. Thus, if guest/userspace selects a pointer and
> > host then gets and dereferences it, we might need to decrease the
> > compiler-selected alignment to prevent compiler on the host from
> > assuming pointer is aligned.
> >
> > This actually triggers on ARM with -mabi=apcs-gnu - which is a
> > deprecated configuration, but it seems safer to handle this
> > generally.
> >
> > Note that userspace that allocates the memory is actually OK and does
> > not need to be fixed, but userspace that gets it from guest or another
> > process does need to be fixed. The later doesn't generally talk to the
> > kernel so while it might be buggy it's not talking to the kernel in the
> > buggy way - it's just using the header in the buggy way - so fixing
> > header and asking userspace to recompile is the best we can do.
> >
> > I verified that the produced kernel binary on x86 is exactly identical
> > before and after the change.
> >
> > Signed-off-by: Michael S. Tsirkin
> > ---
> >
> > changes from v2:
> > add vring_used_elem_t to ensure alignment for substructures
> > changes from v1:
> > swicth all __user to the new typedefs
> >
> > drivers/vhost/vhost.c| 8 +++---
> > drivers/vhost/vhost.h| 6 ++---
> > drivers/vhost/vringh.c | 6 ++---
> > include/linux/vringh.h | 6 ++---
> > include/uapi/linux/virtio_ring.h | 43
> > 5 files changed, 45 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index d450e16c5c25..bc77b0f465fd 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -1244,9 +1244,9 @@ static int vhost_iotlb_miss(struct vhost_virtqueue
> > *vq, u64 iova, int access)
> > }
> > static bool vq_access_ok(struct vhost_virtqueue *vq, unsigned int num,
> > -struct vring_desc __user *desc,
> > -struct vring_avail __user *avail,
> > -struct vring_used __user *used)
> > +vring_desc_t __user *desc,
> > +vring_avail_t __user *avail,
> > +vring_used_t __user *used)
> > {
> > return access_ok(desc, vhost_get_desc_size(vq, num)) &&
> > @@ -2301,7 +2301,7 @@ static int __vhost_add_used_n(struct vhost_virtqueue
> > *vq,
> > struct vring_used_elem *heads,
> > unsigned count)
> > {
> > - struct vring_used_elem __user *used;
> > + vring_used_elem_t __user *used;
> > u16 old, new;
> > int start;
> > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > index f8403bd46b85..60cab4c78229 100644
> > --- a/drivers/vhost/vhost.h
> > +++ b/drivers/vhost/vhost.h
> > @@ -67,9 +67,9 @@ struct vhost_virtqueue {
> > /* The actual ring of buffers. */
> > struct mutex mutex;
> > unsigned int num;
> > - struct vring_desc __user *desc;
> > - struct vring_avail __user *avail;
> > - struct vring_used __user *used;
> > + vring_desc_t __user *desc;
> > + vring_avail_t __user *avail;
> > + vring_used_t __user *used;
> > const struct vhost_iotlb_map *meta_iotlb[VHOST_NUM_ADDRS];
> > struct file *kick;
> > struct eventfd_ctx *call_ctx;
> > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > index ba8e0d6cfd97..e059a9a47cdf 100644
> > --- a/drivers/vhost/vringh.c
> > +++ b/drivers/vhost/vringh.c
> > @@ -620,9 +620,9 @@ static inline int xfer_to_user(const struct vringh *vrh,
> >*/
> > int vringh_init_user(struct vringh *vrh, u64 features,
> > unsigned int num, bool weak_barriers,
> > -struct vring_desc __user *desc,
> > -struct vring_avail __user *avail,
> > -struct vring_used __user *used)
> > +vring_desc_t __user *desc,
> > +vring_avail_t __user *avail,
> > +vring_used_t __user *used)
> > {
> > /* Sane power of 2 please! */
> > if (!num || num > 0x || (num & (num - 1))) {
> > diff --git a/include/linux/vringh.h b/include/linux/vringh.h
> > index 9e2763d7c159..59bd50f99291 100644
> > --- a/include/linux/vringh.h
> > +++ b/include/linux/vringh.h
> > @@ -105,9 +105,9 @@ struct vringh_kiov {
> > /* Helpers for userspace vrings. */
> > int vringh_init_user(struct vringh *vrh, u64 features,
> > unsigned int num, bool weak_barriers,
> > -struct vring_desc __user *desc,
> > -struct vring_avail __user *avail,
> > -struct vring_used __user *used);
> > +vring_desc_t __user *desc,
> > +vring_avail_t __user *avail,
> > +vring_used_t __user *used);
> > static inline void
[PATCH net] vsock/virtio: postpone packet delivery to monitoring devices
We delivering packets to monitoring devices, before to check if
the virtqueue has enough space.
If the virtqueue is full, the transmitting packet is queued up
and it will be sent in the next iteration. This causes the same
packet to be delivered multiple times to monitoring devices.
This patch fixes this issue, postponing the packet delivery
to monitoring devices, only when it is properly queued in the
virqueue.
Fixes: 82dfb540aeb2 ("VSOCK: Add virtio vsock vsockmon hooks")
Signed-off-by: Stefano Garzarella
---
net/vmw_vsock/virtio_transport.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
index dfbaf6bd8b1c..d8db837a96fe 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -115,8 +115,6 @@ virtio_transport_send_pkt_work(struct work_struct *work)
list_del_init(>list);
spin_unlock_bh(>send_pkt_list_lock);
- virtio_transport_deliver_tap_pkt(pkt);
-
reply = pkt->reply;
sg_init_one(, >hdr, sizeof(pkt->hdr));
@@ -137,6 +135,11 @@ virtio_transport_send_pkt_work(struct work_struct *work)
break;
}
+ /* Deliver to monitoring devices all correctly transmitted
+* packets.
+*/
+ virtio_transport_deliver_tap_pkt(pkt);
+
if (reply) {
struct virtqueue *rx_vq = vsock->vqs[VSOCK_VQ_RX];
int val;
--
2.25.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/1] drm/qxl: add mutex_lock/mutex_unlock to ensure the order in which resources are released.
On Sat, Apr 18, 2020 at 02:39:17PM +0800, Caicai wrote:
> When a qxl resource is released, the list that needs to be released is
> fetched from the linked list ring and cleared. When you empty the list,
> instead of trying to determine whether the ttm buffer object for each
> qxl in the list is locked, you release the qxl object and remove the
> element from the list until the list is empty. It was found that the
> linked list was cleared first, and that the lock on the corresponding
> ttm Bo for the QXL had not been released, so that the new qxl could not
> be locked when it used the TTM.
So the dma_resv_reserve_shared() call in qxl_release_validate_bo() is
unbalanced? Because the dma_resv_unlock() call in
qxl_release_fence_buffer_objects() never happens due to
qxl_release_free_list() clearing the list beforehand? Is that correct?
The only way I see for this to happen is that the guest is preempted
between qxl_push_{cursor,command}_ring_release() and
qxl_release_fence_buffer_objects() calls. The host can complete the qxl
command then, signal the guest, and the IRQ handler calls
qxl_release_free_list() before qxl_release_fence_buffer_objects() runs.
Looking through the code I think it should be safe to simply swap the
qxl_release_fence_buffer_objects() +
qxl_push_{cursor,command}_ring_release() calls to close that race
window. Can you try that and see if it fixes the bug for you?
> if (flush)
> - flush_work(>gc_work);
> + //can't flush work, it may lead to deadlock
> + usleep_range(500, 1000);
> +
The commit message doesn't explain this chunk.
take care,
Gerd
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 59/59] drm/bochs: Remove explicit drm_connector_register
On Wed, Apr 15, 2020 at 09:40:34AM +0200, Daniel Vetter wrote: > This is leftovers from the old drm_driver->load callback > upside-down issues. It doesn't do anything for not-hotplugged > connectors since drm_dev_register takes care of that. > > Signed-off-by: Daniel Vetter > Cc: Gerd Hoffmann > Cc: virtualization@lists.linux-foundation.org Acked-by: Gerd Hoffmann ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 37/59] drm/cirrus: Move to drm/tiny
On Wed, Apr 15, 2020 at 09:40:12AM +0200, Daniel Vetter wrote: > Because it is. Indeed. Acked-by: Gerd Hoffmann take care, Gerd ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH v2 1/3] iommu/virtio: Add topology description to virtio-iommu config space
> From: Jean-Philippe Brucker
> Sent: Saturday, February 29, 2020 1:26 AM
>
> Platforms without device-tree do not currently have a method for
> describing the vIOMMU topology. Provide a topology description embedded
> into the virtio device.
>
> Use PCI FIXUP to probe the config space early, because we need to
> discover the topology before any DMA configuration takes place, and the
> virtio driver may be loaded much later. Since we discover the topology
> description when probing the PCI hierarchy, the virtual IOMMU cannot
> manage other platform devices discovered earlier.
>
> This solution isn't elegant nor foolproof, but is the best we can do at
> the moment and works with existing virtio-iommu implementations. It also
> enables an IOMMU for lightweight hypervisors that do not rely on
> firmware methods for booting.
>
> Signed-off-by: Eric Auger
> Signed-off-by: Jean-Philippe Brucker
> ---
> MAINTAINERS | 2 +
> drivers/iommu/Kconfig | 10 +
> drivers/iommu/Makefile| 1 +
> drivers/iommu/virtio-iommu-topology.c | 343
> ++
> drivers/iommu/virtio-iommu.c | 3 +
> include/linux/virt_iommu.h| 19 ++
> include/uapi/linux/virtio_iommu.h | 26 ++
> 7 files changed, 404 insertions(+)
> create mode 100644 drivers/iommu/virtio-iommu-topology.c
> create mode 100644 include/linux/virt_iommu.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fcd79fc38928..65a03ce53096 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17781,6 +17781,8 @@ M:Jean-Philippe Brucker phili...@linaro.org>
> L: virtualization@lists.linux-foundation.org
> S: Maintained
> F: drivers/iommu/virtio-iommu.c
> +F: drivers/iommu/virtio-iommu-topology.c
> +F: include/linux/virt_iommu.h
> F: include/uapi/linux/virtio_iommu.h
>
> VIRTUAL BOX GUEST DEVICE DRIVER
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index c5df570ef84a..f8cb45d84bb0 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -516,4 +516,14 @@ config VIRTIO_IOMMU
>
> Say Y here if you intend to run this kernel as a guest.
>
> +config VIRTIO_IOMMU_TOPOLOGY
> + bool "Topology properties for the virtio-iommu"
> + depends on VIRTIO_IOMMU
> + default y
> + help
> + Enable early probing of the virtio-iommu device, to detect the
> + built-in topology description.
> +
> + 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 9f33fdb3bb05..5da24280d08c 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -37,3 +37,4 @@ obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
> obj-$(CONFIG_QCOM_IOMMU) += qcom_iommu.o
> obj-$(CONFIG_HYPERV_IOMMU) += hyperv-iommu.o
> obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
> +obj-$(CONFIG_VIRTIO_IOMMU_TOPOLOGY) += virtio-iommu-topology.o
> diff --git a/drivers/iommu/virtio-iommu-topology.c b/drivers/iommu/virtio-
> iommu-topology.c
> new file mode 100644
> index ..2188624ef216
> --- /dev/null
> +++ b/drivers/iommu/virtio-iommu-topology.c
> @@ -0,0 +1,343 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +struct viommu_cap_config {
> + u8 bar;
> + u32 length; /* structure size */
> + u32 offset; /* structure offset within the bar */
> +};
> +
> +union viommu_topo_cfg {
> + __le16 type;
> + struct virtio_iommu_topo_pci_range pci;
> + struct virtio_iommu_topo_endpoint ep;
> +};
> +
> +struct viommu_spec {
> + struct device *dev; /* transport device */
> + struct fwnode_handle*fwnode;
> + struct iommu_ops*ops;
> + struct list_headlist;
> + size_t num_items;
Intel DMAR allows an IOMMU to claim INCLUDE_ALL thus avoid listing
every endpoint one-by-one. It is especially useful when there is only
one IOMMU device in the system. Do you think whether making sense
to allow such optimization in this spec? It doesn't work for ARM since
you need ID mapping to find the MSI doorbell. But for architectures
where only topology info is required, it makes the enumeration process
much simpler.
Thanks
Kevin
> + /* The config array of length num_items follows */
> + union viommu_topo_cfg cfg[];
> +};
> +
> +static LIST_HEAD(viommus);
> +static DEFINE_MUTEX(viommus_lock);
> +
> +#define VPCI_FIELD(field) offsetof(struct virtio_pci_cap, field)
> +
> +static inline int viommu_pci_find_capability(struct pci_dev *dev, u8
> cfg_type,
> + struct viommu_cap_config *cap)
> +{
> + int pos;
> + u8 bar;
> +
> +
Re: [virtio-dev] Re: [PATCH] virtio-balloon: Disable free page hinting/reporting if page poison is disabled
>>> 2. Can we assume that the guest will always rewrite the page after the >>> deflate in the case of init_on_free or poison? >> >> Depends on what we think is the right way to do - IOW if we think "some >> other content" as mentioned above is a BUG or not. > > So I wouldn't consider it a but as the zero page probably doesn't > apply. We are basically just indicating we don't care about the > contents, we aren't giving it a value. At least that is how I see it > working based on how it was implemented. > >>> 3. Can we assume that free page hinting will always function as a >>> balloon setup, so no moving it over to a page reporting type setup? >> >> I think we have to define the valid semantics. That implies what would >> be valid to do with it. Unfortunately, we have to reverse-engineer here. > > So at this point a "hinted" page is a page that can essentially > transition to uninitialized while it is sitting in the balloon. I > suspect that is how we are going to have to treat it since that is the > behavior that it has right now. At least it's not what Michael initially thought should be done - IIRC. "We need to remember that the whole HINT thing is not a mandate for host to corrupt memory. It's just some info about page use guest gives host. If host corrupts memory it's broken ..."). So the question remains: Does QEMU have a BUG or do we actually allow to corrupt guest memory. That leaves us with either 1. "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT might get replaced by zero page. However, as soon as the page is written by the guest (even before the hinting request was processed by the host), the modified page will stay - whereby the unwritten parts might either be from the old, or from the zero page." - a QEMU BUG. 2. "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT are considered unused and will contain an undefined/uninitialized content once hinted. As soon as the page is written by the guest (even before the hinting request was processed by the host), the modified page will stay. The guest should reinitialized the full page in case it cares about the actual content (e.g., page poisoning)." I tend to favor 2 - although it basically leaves no room for future improvement regarding skipping re-initialization in the guest after migration. >>> >>> If we assume the above 3 items then there isn't any point in worrying >>> about poison when it comes to free page hinting. It doesn't make sense >>> to since they essentially negate it. As such I could go through this >>> patch and the QEMU patches and clean up any associations since the to >>> are not really tied together in any way. >> >> The big question is, if we want to support VIRTIO_BALLOON_F_PAGE_POISON >> with free page hinting. e.g.,: >> >> "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT might get replaced by >> a page full of X. However, as soon as the page is written by the guest >> (even before the hinting request was processed by the host), the >> modified page will stay - whereby the unwritten parts might either be >> from the old, or from a page filled with X. Without >> VIRTIO_BALLOON_F_PAGE_POISON, X is zero, otherwise it is poison_val." [...] > > With the VIRTIO_BALLOON_F_PAGE_POISON we could make it so that when > the page comes out of the balloon it is either unmodified or it is > poison_val. Without the VIRTIO_BALLOON_F_PAGE_POISON feature present > you cannot make that guarantee and are stuck with the page being > treated as either unmodified or uninitialized memory. While it would be possible, I doubt it will be easy to implement, and I still wonder if we should really care about optimizing an undocumented feature that takes three people to figure out the semantics. > >> The current QEMU implementation would be to simply migrate all hinted >> pages. In the future we could optimize. Not sure if it's worth the trouble. > > So the trick for me is how best to go about sorting this all out. > There are two ways I see of doing it. > > The first approach would be to just assume that hinting should be > disassociated from poisoning. If I go that route that would more > closely match up with what happened in QEMU. The downside is that it > locks in the unmodified/uninitialized behavior and would require pages > to be rewritten when they come out of the balloon. However this is the > behavior we have now so it would only require specification > documentation changes. Right now, for simplicity, I prefer this and define VIRTIO_BALLOON_F_PAGE_POISON only for explicit deflation (balloon deflation via the deflate queue) and implicit deflation (VIRTIO_BALLOON_F_REPORTING). Let's see if Michael has another opinion. -- Thanks, David / dhildenb ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
