Re: [virtio-dev] Re: [PATCH] virtio-balloon: Disable free page hinting/reporting if page poison is disabled
On Fri, Apr 17, 2020 at 3:09 AM David Hildenbrand wrote: > > > What do you call "hinting ends" though? The fact we put > > a page in the VQ is not a guarantee that it's been consumed > > by the hypervisor. > > > > I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE. The key bit to this is that there are 4 states, and quasi unlimited command IDs, although I believe the first 2 are matched up to the states. So the VIRTIO_BALLOON_CMD_ID_DONE is matched up with FREE_PAGE_REPORT_S_DONE, and CMD_ID_STOP with S_STOP, but really all it means is that we are done with the current epoch so we need to flush the memory and move on. The state is more important to the hypervisor as it will switch to "STOP" while it is synching the dirty bits, "REQUESTED" once that has been completed and it will increment the command ID, "START" once the first hint is received with the matching command ID, and "DONE" once the migration is complete. As long as it is in the "START" state and the command ID in the hint matches it will use the information to clear the dirty bits as it runs in parallel with the migration task. The piece I think I was missing was that the balloon is staying (mostly) inflated until the migration is complete. If that is the case then I suppose we could leave this enabled at least on the guest side, assuming the balloon doesn't give back too many pages when it hits the shrinker. > > > > I think a strict definition is this: > > - hint includes a command ID > > - hint implies "page was unused at some point after guest reading command > > ID" > > > > > > Hypervisor can use dirty tracking tricks to get from that to > > "page is unused at the moment". > > > >> Whereby X is > >> currently assumed to be 0, correct? > > > > > > > > Now we are talking about what's safe to do with the page. > > > > If POISON flag is set by hypervisor but clear by guest, > > or poison_val is 0, then it's clear it's safe to blow > > away the page if we can figure out it's unused. > > > > Otherwise, it's much less clear :) > > Hah! Agreed :D That isn't quite true. The problem is in the case of hinting it isn't setting the page to 0. It is simply not migrating it. So if there is data from an earlier pass it is stuck at that value. So the balloon will see the poison/init on some pages cleared, however I suppose the balloon doesn't care about the contents of the page. For the pages that are leaked back out via the shrinker they will be dirtied so they will end up being migrated with the correct value eventually. > > I'll have to come back and re-read the rest next week, this > > is complex stuff and I'm too rushed with other things today. > > Yeah, I'm also loaded with other stuff. Maybe Alex has time to > understand the details as well. So after looking it over again it makes a bit more sense why this hasn't caused any issues so far, and I can see why the poison enabled setup and hinting can work. The problem is I am left wondering what assumptions we are allowed to leave in place. 1. Can we assume that we don't care about the contents in the pages in the balloon changing? 2. Can we assume that the guest will always rewrite the page after the deflate in the case of init_on_free or poison? 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? 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. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: improve use_mm / unuse_mm v2
On 4/15/20 11:31 PM, Christoph Hellwig wrote: > Hi all, > > this series improves the use_mm / unuse_mm interface by better > documenting the assumptions, and my taking the set_fs manipulations > spread over the callers into the core API. > > Changes since v1: > - drop a few patches > - fix a comment typo > - cover the newly merged use_mm/unuse_mm caller in vfio You can add my reviewed-by/tested-by to the patches, passes the io_uring regression tests. -- Jens Axboe ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 05/70] x86/insn: Make inat-tables.c suitable for pre-decompression code
On Fri, Apr 17, 2020 at 09:50:00PM +0900, Masami Hiramatsu wrote: > On Thu, 16 Apr 2020 17:24:06 +0200 > Joerg Roedel wrote: > Ah, I got it. So you intended to port the instruction decoder to > pre-decompression boot code, correct? Right, it is needed there to decode instructions which cause #VC exceptions when running as an SEV-ES guest. > > The call-site is added with the patch that includes the > > instruction decoder into the pre-decompression code. If possible I'd > > like to keep those things separate, as both patches are already pretty > > big by themselfes (and they do different things, in different parts of > > the code). > > OK, if you will send v2, please CC both to me. Will do, I added you to the cc-list for future posts of this series. Regards, Joerg ___ 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 17.04.20 13:02, Michael S. Tsirkin wrote: > On Fri, Apr 17, 2020 at 12:31:14PM +0200, David Hildenbrand wrote: >> On 17.04.20 12:29, Michael S. Tsirkin wrote: >>> On Fri, Apr 17, 2020 at 12:26:24PM +0200, David Hildenbrand wrote: On 17.04.20 12:19, Michael S. Tsirkin wrote: > On Fri, Apr 17, 2020 at 12:09:38PM +0200, David Hildenbrand wrote: >> > What do you call "hinting ends" though? The fact we put >>> a page in the VQ is not a guarantee that it's been consumed >>> by the hypervisor. >>> >> >> I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE. > > Can't find that one anywhere. what did I miss? Sorry, the QEMU implementation is confusing. FREE_PAGE_REPORT_S_DONE is translated to VIRTIO_BALLOON_CMD_ID_DONE >>> >>> Well VIRTIO_BALLOON_CMD_ID_DONE just means "don't give me any >>> more hints, I finished migration". >>> Guest will stop hinting even without that once it scans all >>> free memory. >> >> Yeah, that's the end of the whole process where you can be sure the host >> processed all requests definetly. > > It's not guaranteed to happen :) Sending an interrupt at the end > of each scan doubles the overhead ... Yeah, AFAIKs you either get a VIRTIO_BALLOON_CMD_ID_STOP or a VIRTIO_BALLOON_CMD_ID_DONE at the end. And AFAIK both will guarantee that all previous hints were handled. ... but reconstructing this protocol from code is probably too much for a friday afternoon, lol -- 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 Fri, Apr 17, 2020 at 12:31:14PM +0200, David Hildenbrand wrote: > On 17.04.20 12:29, Michael S. Tsirkin wrote: > > On Fri, Apr 17, 2020 at 12:26:24PM +0200, David Hildenbrand wrote: > >> On 17.04.20 12:19, Michael S. Tsirkin wrote: > >>> On Fri, Apr 17, 2020 at 12:09:38PM +0200, David Hildenbrand wrote: > > What do you call "hinting ends" though? The fact we put > > a page in the VQ is not a guarantee that it's been consumed > > by the hypervisor. > > > > I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE. > >>> > >>> Can't find that one anywhere. what did I miss? > >> > >> Sorry, the QEMU implementation is confusing. FREE_PAGE_REPORT_S_DONE is > >> translated to VIRTIO_BALLOON_CMD_ID_DONE > > > > Well VIRTIO_BALLOON_CMD_ID_DONE just means "don't give me any > > more hints, I finished migration". > > Guest will stop hinting even without that once it scans all > > free memory. > > Yeah, that's the end of the whole process where you can be sure the host > processed all requests definetly. It's not guaranteed to happen :) Sending an interrupt at the end of each scan doubles the overhead ... > -- > 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 17.04.20 12:29, Michael S. Tsirkin wrote: > On Fri, Apr 17, 2020 at 12:26:24PM +0200, David Hildenbrand wrote: >> On 17.04.20 12:19, Michael S. Tsirkin wrote: >>> On Fri, Apr 17, 2020 at 12:09:38PM +0200, David Hildenbrand wrote: > What do you call "hinting ends" though? The fact we put > a page in the VQ is not a guarantee that it's been consumed > by the hypervisor. > I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE. >>> >>> Can't find that one anywhere. what did I miss? >> >> Sorry, the QEMU implementation is confusing. FREE_PAGE_REPORT_S_DONE is >> translated to VIRTIO_BALLOON_CMD_ID_DONE > > Well VIRTIO_BALLOON_CMD_ID_DONE just means "don't give me any > more hints, I finished migration". > Guest will stop hinting even without that once it scans all > free memory. Yeah, that's the end of the whole process where you can be sure the host processed all requests definetly. -- 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 Fri, Apr 17, 2020 at 12:26:24PM +0200, David Hildenbrand wrote:
> On 17.04.20 12:19, Michael S. Tsirkin wrote:
> > On Fri, Apr 17, 2020 at 12:09:38PM +0200, David Hildenbrand wrote:
> >> > What do you call "hinting ends" though? The fact we put
> >>> a page in the VQ is not a guarantee that it's been consumed
> >>> by the hypervisor.
> >>>
> >>
> >> I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE.
> >
> > Can't find that one anywhere. what did I miss?
>
> Sorry, the QEMU implementation is confusing. FREE_PAGE_REPORT_S_DONE is
> translated to VIRTIO_BALLOON_CMD_ID_DONE
Well VIRTIO_BALLOON_CMD_ID_DONE just means "don't give me any
more hints, I finished migration".
Guest will stop hinting even without that once it scans all
free memory.
> QEMU:
>
> hw/virtio/virtio-balloon.c:virtio_balloon_free_page_report_notify()
> -> virtio_balloon_free_page_done(dev)
> -> s->free_page_report_status = FREE_PAGE_REPORT_S_DONE;
>virtio_notify_config(vdev);
>
> When the guest reads the config
> hw/virtio/virtio-balloon.c:virtio_balloon_get_config()
> -> if (dev->free_page_report_status == FREE_PAGE_REPORT_S_DONE)
> -> config.free_page_report_cmd_id = ... VIRTIO_BALLOON_CMD_ID_DONE
>
>
> Linux:
>
> drivers/virtio/virtio_balloon.c:report_free_page_func()
> -> if (cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
> -> return_free_pages_to_mm()
>
>
> So it's VIRTIO_BALLOON_CMD_ID_DONE.
>
>
> --
> 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 17.04.20 12:19, Michael S. Tsirkin wrote:
> On Fri, Apr 17, 2020 at 12:09:38PM +0200, David Hildenbrand wrote:
>> > What do you call "hinting ends" though? The fact we put
>>> a page in the VQ is not a guarantee that it's been consumed
>>> by the hypervisor.
>>>
>>
>> I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE.
>
> Can't find that one anywhere. what did I miss?
Sorry, the QEMU implementation is confusing. FREE_PAGE_REPORT_S_DONE is
translated to VIRTIO_BALLOON_CMD_ID_DONE
QEMU:
hw/virtio/virtio-balloon.c:virtio_balloon_free_page_report_notify()
-> virtio_balloon_free_page_done(dev)
-> s->free_page_report_status = FREE_PAGE_REPORT_S_DONE;
virtio_notify_config(vdev);
When the guest reads the config
hw/virtio/virtio-balloon.c:virtio_balloon_get_config()
-> if (dev->free_page_report_status == FREE_PAGE_REPORT_S_DONE)
-> config.free_page_report_cmd_id = ... VIRTIO_BALLOON_CMD_ID_DONE
Linux:
drivers/virtio/virtio_balloon.c:report_free_page_func()
-> if (cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
-> return_free_pages_to_mm()
So it's VIRTIO_BALLOON_CMD_ID_DONE.
--
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 Fri, Apr 17, 2020 at 12:09:38PM +0200, David Hildenbrand wrote: > > What do you call "hinting ends" though? The fact we put > > a page in the VQ is not a guarantee that it's been consumed > > by the hypervisor. > > > > I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE. Can't find that one anywhere. what did I miss? > > > > I think a strict definition is this: > > - hint includes a command ID > > - hint implies "page was unused at some point after guest reading command > > ID" > > > > > > Hypervisor can use dirty tracking tricks to get from that to > > "page is unused at the moment". > > > >> Whereby X is > >> currently assumed to be 0, correct? > > > > > > > > Now we are talking about what's safe to do with the page. > > > > If POISON flag is set by hypervisor but clear by guest, > > or poison_val is 0, then it's clear it's safe to blow > > away the page if we can figure out it's unused. > > > > Otherwise, it's much less clear :) > > Hah! Agreed :D > > > > > > > > > I'll have to come back and re-read the rest next week, this > > is complex stuff and I'm too rushed with other things today. > > Yeah, I'm also loaded with other stuff. Maybe Alex has time to > understand the details as well. > > -- > 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
> What do you call "hinting ends" though? The fact we put > a page in the VQ is not a guarantee that it's been consumed > by the hypervisor. > I'd say hinting ends once the hypervisor sets FREE_PAGE_REPORT_S_DONE. > > I think a strict definition is this: > - hint includes a command ID > - hint implies "page was unused at some point after guest reading command ID" > > > Hypervisor can use dirty tracking tricks to get from that to > "page is unused at the moment". > >> Whereby X is >> currently assumed to be 0, correct? > > > > Now we are talking about what's safe to do with the page. > > If POISON flag is set by hypervisor but clear by guest, > or poison_val is 0, then it's clear it's safe to blow > away the page if we can figure out it's unused. > > Otherwise, it's much less clear :) Hah! Agreed :D > > > > I'll have to come back and re-read the rest next week, this > is complex stuff and I'm too rushed with other things today. Yeah, I'm also loaded with other stuff. Maybe Alex has time to understand the details as well. -- 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 Fri, Apr 17, 2020 at 11:51:31AM +0200, David Hildenbrand wrote: > > >>> I'll need to think about this. > >>> 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 ... > >> > >> I don't think that's true, > > > > Do you refer to "If host corrupts memory it's broken"? > > You think that's not true? > > Let me think this through. IMHO it's a "hint with the option for the > hypervisor to assume the content is X and optimize (e.g., not migrate a > page) unless the page is reused before hinting ends". What do you call "hinting ends" though? The fact we put a page in the VQ is not a guarantee that it's been consumed by the hypervisor. I think a strict definition is this: - hint includes a command ID - hint implies "page was unused at some point after guest reading command ID" Hypervisor can use dirty tracking tricks to get from that to "page is unused at the moment". > Whereby X is > currently assumed to be 0, correct? Now we are talking about what's safe to do with the page. If POISON flag is set by hypervisor but clear by guest, or poison_val is 0, then it's clear it's safe to blow away the page if we can figure out it's unused. Otherwise, it's much less clear :) I'll have to come back and re-read the rest next week, this is complex stuff and I'm too rushed with other things today. > Assume migrated starts, guest hints a page, migration ends. Guest reads > the page again. > > a) It could be the original page (still migrated) > b) It could be the zero page (not migrated). > > And I think that's a valid corruption of the page content, no? > > > Now, it's more tricky when we have the following > > Migrated starts, guest hints a page, guest reuses the page (e.g., writes > first byte), migration ends. Guest reads the page again. > > Here, I (hope) always the original page content is maintained via the > 2-bitmap magic. > > Or am I missing something important? > > > > >> and that's not what we currently implement in > >> the hypervisor for speeding up migration. I still consider > >> VIRTIO_BALLOON_F_FREE_PAGE_HINT as an alternative technique of > >> temporarily inflating/deflating. > > > > Reporting is like that. But hinting isn't, simply because > > by the time host gets the hint page may already be in use. > > Blowing it out unconditionally would lead to easily > > reproducible guest crashes. > > Agreed that the semantics are different. But "eventually getting a zero > page after migration" was part of the whole invention, no? That's the > whole point of the optimization. > > > > >> E.g., we don't migrate any of these > >> pages in the hypervisor, so the content will be zeroed out on the > >> migration target. > > > > Not exactly true. We do a delicate play with > > the two dirty bits so they *are* migrated sometimes. > > Agreed. Will something like this catch the semantics? > > "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT will > always have the original page content when written before hinting > stops. However, if pages are not written before hinting stops, the > hypervisor might replace them by a zero page." > > > > >> If migration fails, the ld content will remain. You > >> can call that "corrupting memory" - it's exactly what it has been > >> invented for. > > > > Well no, original author repeatedly stated it's "hinting" > > because page can be in use actually. > > Agreed. > > > > >> > >> IIRC we decided to glue the semantics of VIRTIO_BALLOON_F_PAGE_POISON to > >> VIRTIO_BALLOON_F_FREE_PAGE_HINT, which makes in my opinion perfect sense. > >> > >> So I propose: > >> > >> VIRTIO_BALLOON_F_PAGE_POISON not implemented in the hypervisor: > >> - Pages inflated/deflated via VIRTIO_BALLOON_F_FREE_PAGE_HINT will > >> either have the old page content or will be filled with 0. > >> - This matches what we currently do. > >> > >> VIRTIO_BALLOON_F_PAGE_POISON implemented in the hypervisor: > >> - Pages inflated/deflated via VIRTIO_BALLOON_F_FREE_PAGE_HINT will > >> either have the old page content or will be filled with poison_val. > >> - This matches what we should do also during ordinary > >> inflation/deflation and free page reporting. > > > > It's a reasonable option. however ... > > > >> Again, nothing is currently broken as we free_page() the pages and don't > >> care about an eventually changed page content. > > > > I don't see how you can say that. ATM on a host without POISON > > and with HINT, with poison val != 0 and with validation, > > host can blow a free page away and then guest will detect > > that as corruption. > > (At this point I start to hate the whole free page hinting feature again > :D It starts messing with my brain again.) > > As soon as we do the free_page(), the page will be written to and > definitely get migrated, right? If the hypervisor would blow out the > page after the free_page(), we would b
Re: [virtio-dev] Re: [PATCH] virtio-balloon: Disable free page hinting/reporting if page poison is disabled
>>> I'll need to think about this. >>> 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 ... >> >> I don't think that's true, > > Do you refer to "If host corrupts memory it's broken"? > You think that's not true? Let me think this through. IMHO it's a "hint with the option for the hypervisor to assume the content is X and optimize (e.g., not migrate a page) unless the page is reused before hinting ends". Whereby X is currently assumed to be 0, correct? Assume migrated starts, guest hints a page, migration ends. Guest reads the page again. a) It could be the original page (still migrated) b) It could be the zero page (not migrated). And I think that's a valid corruption of the page content, no? Now, it's more tricky when we have the following Migrated starts, guest hints a page, guest reuses the page (e.g., writes first byte), migration ends. Guest reads the page again. Here, I (hope) always the original page content is maintained via the 2-bitmap magic. Or am I missing something important? > >> and that's not what we currently implement in >> the hypervisor for speeding up migration. I still consider >> VIRTIO_BALLOON_F_FREE_PAGE_HINT as an alternative technique of >> temporarily inflating/deflating. > > Reporting is like that. But hinting isn't, simply because > by the time host gets the hint page may already be in use. > Blowing it out unconditionally would lead to easily > reproducible guest crashes. Agreed that the semantics are different. But "eventually getting a zero page after migration" was part of the whole invention, no? That's the whole point of the optimization. > >> E.g., we don't migrate any of these >> pages in the hypervisor, so the content will be zeroed out on the >> migration target. > > Not exactly true. We do a delicate play with > the two dirty bits so they *are* migrated sometimes. Agreed. Will something like this catch the semantics? "Pages hinted via VIRTIO_BALLOON_F_FREE_PAGE_HINT will always have the original page content when written before hinting stops. However, if pages are not written before hinting stops, the hypervisor might replace them by a zero page." > >> If migration fails, the ld content will remain. You >> can call that "corrupting memory" - it's exactly what it has been >> invented for. > > Well no, original author repeatedly stated it's "hinting" > because page can be in use actually. Agreed. > >> >> IIRC we decided to glue the semantics of VIRTIO_BALLOON_F_PAGE_POISON to >> VIRTIO_BALLOON_F_FREE_PAGE_HINT, which makes in my opinion perfect sense. >> >> So I propose: >> >> VIRTIO_BALLOON_F_PAGE_POISON not implemented in the hypervisor: >> - Pages inflated/deflated via VIRTIO_BALLOON_F_FREE_PAGE_HINT will >> either have the old page content or will be filled with 0. >> - This matches what we currently do. >> >> VIRTIO_BALLOON_F_PAGE_POISON implemented in the hypervisor: >> - Pages inflated/deflated via VIRTIO_BALLOON_F_FREE_PAGE_HINT will >> either have the old page content or will be filled with poison_val. >> - This matches what we should do also during ordinary >> inflation/deflation and free page reporting. > > It's a reasonable option. however ... > >> Again, nothing is currently broken as we free_page() the pages and don't >> care about an eventually changed page content. > > I don't see how you can say that. ATM on a host without POISON > and with HINT, with poison val != 0 and with validation, > host can blow a free page away and then guest will detect > that as corruption. (At this point I start to hate the whole free page hinting feature again :D It starts messing with my brain again.) As soon as we do the free_page(), the page will be written to and definitely get migrated, right? If the hypervisor would blow out the page after the free_page(), we would be in trouble. What am I missing? -- Thanks, David / dhildenb ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2] vhost: do not enable VHOST_MENU by default
On 2020/4/17 下午5:38, Michael S. Tsirkin wrote: On Fri, Apr 17, 2020 at 05:33:56PM +0800, Jason Wang wrote: On 2020/4/17 下午5:01, Michael S. Tsirkin wrote: There could be some misunderstanding here. I thought it's somehow similar: a CONFIG_VHOST_MENU=y will be left in the defconfigs even if CONFIG_VHOST is not set. Thanks BTW do entries with no prompt actually appear in defconfig? Yes. I can see CONFIG_VHOST_DPN=y after make ARCH=m68k defconfig You see it in .config right? So that's harmless right? Yes. Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2] vhost: do not enable VHOST_MENU by default
On Fri, Apr 17, 2020 at 05:33:56PM +0800, Jason Wang wrote: > > On 2020/4/17 下午5:01, Michael S. Tsirkin wrote: > > > There could be some misunderstanding here. I thought it's somehow > > > similar: a > > > CONFIG_VHOST_MENU=y will be left in the defconfigs even if CONFIG_VHOST is > > > not set. > > > > > > Thanks > > > > > BTW do entries with no prompt actually appear in defconfig? > > > > Yes. I can see CONFIG_VHOST_DPN=y after make ARCH=m68k defconfig You see it in .config right? So that's harmless right? -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2] vhost: do not enable VHOST_MENU by default
On 2020/4/17 下午5:01, Michael S. Tsirkin wrote: There could be some misunderstanding here. I thought it's somehow similar: a CONFIG_VHOST_MENU=y will be left in the defconfigs even if CONFIG_VHOST is not set. Thanks BTW do entries with no prompt actually appear in defconfig? Yes. I can see CONFIG_VHOST_DPN=y after make ARCH=m68k defconfig Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2] vhost: do not enable VHOST_MENU by default
On 2020/4/17 下午5:25, Geert Uytterhoeven wrote:
Hi Michael,
On Fri, Apr 17, 2020 at 10:57 AM Michael S. Tsirkin wrote:
On Fri, Apr 17, 2020 at 04:51:19PM +0800, Jason Wang wrote:
On 2020/4/17 下午4:46, Michael S. Tsirkin wrote:
On Fri, Apr 17, 2020 at 04:39:49PM +0800, Jason Wang wrote:
On 2020/4/17 下午4:29, Michael S. Tsirkin wrote:
On Fri, Apr 17, 2020 at 03:36:52PM +0800, Jason Wang wrote:
On 2020/4/17 下午2:33, Michael S. Tsirkin wrote:
On Fri, Apr 17, 2020 at 11:12:14AM +0800, Jason Wang wrote:
On 2020/4/17 上午6:55, Michael S. Tsirkin wrote:
On Wed, Apr 15, 2020 at 10:43:56AM +0800, Jason Wang wrote:
We try to keep the defconfig untouched after decoupling CONFIG_VHOST
out of CONFIG_VIRTUALIZATION in commit 20c384f1ea1a
("vhost: refine vhost and vringh kconfig") by enabling VHOST_MENU by
default. Then the defconfigs can keep enabling CONFIG_VHOST_NET
without the caring of CONFIG_VHOST.
But this will leave a "CONFIG_VHOST_MENU=y" in all defconfigs and even
for the ones that doesn't want vhost. So it actually shifts the
burdens to the maintainers of all other to add "CONFIG_VHOST_MENU is
not set". So this patch tries to enable CONFIG_VHOST explicitly in
defconfigs that enables CONFIG_VHOST_NET and CONFIG_VHOST_VSOCK.
Acked-by: Christian Borntraeger(s390)
Acked-by: Michael Ellerman(powerpc)
Cc: Thomas Bogendoerfer
Cc: Benjamin Herrenschmidt
Cc: Paul Mackerras
Cc: Michael Ellerman
Cc: Heiko Carstens
Cc: Vasily Gorbik
Cc: Christian Borntraeger
Reported-by: Geert Uytterhoeven
Signed-off-by: Jason Wang
I rebased this on top of OABI fix since that
seems more orgent to fix.
Pushed to my vhost branch pls take a look and
if possible test.
Thanks!
I test this patch by generating the defconfigs that wants vhost_net or
vhost_vsock. All looks fine.
But having CONFIG_VHOST_DPN=y may end up with the similar situation that
this patch want to address.
Maybe we can let CONFIG_VHOST depends on !ARM || AEABI then add another
menuconfig for VHOST_RING and do something similar?
Thanks
Sorry I don't understand. After this patch CONFIG_VHOST_DPN is just
an internal variable for the OABI fix. I kept it separate
so it's easy to revert for 5.8. Yes we could squash it into
VHOST directly but I don't see how that changes logic at all.
Sorry for being unclear.
I meant since it was enabled by default, "CONFIG_VHOST_DPN=y" will be left
in the defconfigs.
But who cares?
FYI, please seehttps://www.spinics.net/lists/kvm/msg212685.html
The complaint was not about the symbol IIUC. It was that we caused
everyone to build vhost unless they manually disabled it.
There could be some misunderstanding here. I thought it's somehow similar: a
CONFIG_VHOST_MENU=y will be left in the defconfigs even if CONFIG_VHOST is
not set.
Thanks
Hmm. So looking at Documentation/kbuild/kconfig-language.rst :
Things that merit "default y/m" include:
a) A new Kconfig option for something that used to always be built
should be "default y".
b) A new gatekeeping Kconfig option that hides/shows other Kconfig
options (but does not generate any code of its own), should be
"default y" so people will see those other options.
c) Sub-driver behavior or similar options for a driver that is
"default n". This allows you to provide sane defaults.
So it looks like VHOST_MENU is actually matching rule b).
So what's the problem we are trying to solve with this patch, exactly?
Geert could you clarify pls?
I can confirm VHOST_MENU is matching rule b), so it is safe to always
enable it.
Gr{oetje,eeting}s,
Geert
Right, so I think we can drop this patch.
Thanks
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2] vhost: do not enable VHOST_MENU by default
Hi Michael,
On Fri, Apr 17, 2020 at 10:57 AM Michael S. Tsirkin wrote:
> On Fri, Apr 17, 2020 at 04:51:19PM +0800, Jason Wang wrote:
> > On 2020/4/17 下午4:46, Michael S. Tsirkin wrote:
> > > On Fri, Apr 17, 2020 at 04:39:49PM +0800, Jason Wang wrote:
> > > > On 2020/4/17 下午4:29, Michael S. Tsirkin wrote:
> > > > > On Fri, Apr 17, 2020 at 03:36:52PM +0800, Jason Wang wrote:
> > > > > > On 2020/4/17 下午2:33, Michael S. Tsirkin wrote:
> > > > > > > On Fri, Apr 17, 2020 at 11:12:14AM +0800, Jason Wang wrote:
> > > > > > > > On 2020/4/17 上午6:55, Michael S. Tsirkin wrote:
> > > > > > > > > On Wed, Apr 15, 2020 at 10:43:56AM +0800, Jason Wang wrote:
> > > > > > > > > > We try to keep the defconfig untouched after decoupling
> > > > > > > > > > CONFIG_VHOST
> > > > > > > > > > out of CONFIG_VIRTUALIZATION in commit 20c384f1ea1a
> > > > > > > > > > ("vhost: refine vhost and vringh kconfig") by enabling
> > > > > > > > > > VHOST_MENU by
> > > > > > > > > > default. Then the defconfigs can keep enabling
> > > > > > > > > > CONFIG_VHOST_NET
> > > > > > > > > > without the caring of CONFIG_VHOST.
> > > > > > > > > >
> > > > > > > > > > But this will leave a "CONFIG_VHOST_MENU=y" in all
> > > > > > > > > > defconfigs and even
> > > > > > > > > > for the ones that doesn't want vhost. So it actually shifts
> > > > > > > > > > the
> > > > > > > > > > burdens to the maintainers of all other to add
> > > > > > > > > > "CONFIG_VHOST_MENU is
> > > > > > > > > > not set". So this patch tries to enable CONFIG_VHOST
> > > > > > > > > > explicitly in
> > > > > > > > > > defconfigs that enables CONFIG_VHOST_NET and
> > > > > > > > > > CONFIG_VHOST_VSOCK.
> > > > > > > > > >
> > > > > > > > > > Acked-by: Christian Borntraeger
> > > > > > > > > > (s390)
> > > > > > > > > > Acked-by: Michael Ellerman (powerpc)
> > > > > > > > > > Cc: Thomas Bogendoerfer
> > > > > > > > > > Cc: Benjamin Herrenschmidt
> > > > > > > > > > Cc: Paul Mackerras
> > > > > > > > > > Cc: Michael Ellerman
> > > > > > > > > > Cc: Heiko Carstens
> > > > > > > > > > Cc: Vasily Gorbik
> > > > > > > > > > Cc: Christian Borntraeger
> > > > > > > > > > Reported-by: Geert Uytterhoeven
> > > > > > > > > > Signed-off-by: Jason Wang
> > > > > > > > > I rebased this on top of OABI fix since that
> > > > > > > > > seems more orgent to fix.
> > > > > > > > > Pushed to my vhost branch pls take a look and
> > > > > > > > > if possible test.
> > > > > > > > > Thanks!
> > > > > > > > I test this patch by generating the defconfigs that wants
> > > > > > > > vhost_net or
> > > > > > > > vhost_vsock. All looks fine.
> > > > > > > >
> > > > > > > > But having CONFIG_VHOST_DPN=y may end up with the similar
> > > > > > > > situation that
> > > > > > > > this patch want to address.
> > > > > > > > Maybe we can let CONFIG_VHOST depends on !ARM || AEABI then add
> > > > > > > > another
> > > > > > > > menuconfig for VHOST_RING and do something similar?
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > Sorry I don't understand. After this patch CONFIG_VHOST_DPN is
> > > > > > > just
> > > > > > > an internal variable for the OABI fix. I kept it separate
> > > > > > > so it's easy to revert for 5.8. Yes we could squash it into
> > > > > > > VHOST directly but I don't see how that changes logic at all.
> > > > > > Sorry for being unclear.
> > > > > >
> > > > > > I meant since it was enabled by default, "CONFIG_VHOST_DPN=y" will
> > > > > > be left
> > > > > > in the defconfigs.
> > > > > But who cares?
> > > > FYI, please seehttps://www.spinics.net/lists/kvm/msg212685.html
> > > The complaint was not about the symbol IIUC. It was that we caused
> > > everyone to build vhost unless they manually disabled it.
> >
> > There could be some misunderstanding here. I thought it's somehow similar: a
> > CONFIG_VHOST_MENU=y will be left in the defconfigs even if CONFIG_VHOST is
> > not set.
> >
> > Thanks
>
> Hmm. So looking at Documentation/kbuild/kconfig-language.rst :
>
> Things that merit "default y/m" include:
>
> a) A new Kconfig option for something that used to always be built
>should be "default y".
>
> b) A new gatekeeping Kconfig option that hides/shows other Kconfig
>options (but does not generate any code of its own), should be
>"default y" so people will see those other options.
>
> c) Sub-driver behavior or similar options for a driver that is
>"default n". This allows you to provide sane defaults.
>
>
> So it looks like VHOST_MENU is actually matching rule b).
> So what's the problem we are trying to solve with this patch, exactly?
>
> Geert could you clarify pls?
I can confirm VHOST_MENU is matching rule b), so it is safe to always
enable it.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists
Re: [virtio-dev] Re: [PATCH] virtio-balloon: Disable free page hinting/reporting if page poison is disabled
On Fri, Apr 17, 2020 at 11:07:38AM +0200, David Hildenbrand wrote:
> On 17.04.20 10:50, Michael S. Tsirkin wrote:
> > On Fri, Apr 17, 2020 at 09:49:03AM +0200, David Hildenbrand wrote:
> >> On 17.04.20 08:28, Michael S. Tsirkin wrote:
> >>> On Thu, Apr 16, 2020 at 04:52:42PM -0700, Alexander Duyck wrote:
> On Thu, Apr 16, 2020 at 3:13 PM Michael S. Tsirkin
> wrote:
> >
> > On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck wrote:
> >> From: Alexander Duyck
> >>
> >> If we have free page hinting or page reporting enabled we should
> >> disable it
> >> if the pages are poisoned or initialized on free and we cannot notify
> >> the
> >> hypervisor.
> >>
> >> Fixes: 5d757c8d518d ("virtio-balloon: add support for providing free
> >> page reports to host")
> >>
> >> Signed-off-by: Alexander Duyck
> >
> >
> > Why not put this logic in the hypervisor?
>
> I did that too. This is to cover the case where somebody is running
> the code prior to my QEMU changes where the page poison feature wasn't
> being enabled.
> >>>
> >>>
> >>> Hmm so that one looks like a QEMU bug (does not expose poison flag). In
> >>> the past we just said need to fix the bug where it's found unless the
> >>> issue is very widespread and major. Let's assume QEMU learns to always
> >>> expose POISON with HINT. Then this configuration (HINT && !POISON)
> >>> allows us to detect old QEMU (pre your changes).
> >>
> >> Don't see why this is a QEMU bug. It's just a feature it does not
> >> implement. Perfectly valid.
> >
> > I'll need to think about this.
> > 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 ...
>
> I don't think that's true,
Do you refer to "If host corrupts memory it's broken"?
You think that's not true?
> and that's not what we currently implement in
> the hypervisor for speeding up migration. I still consider
> VIRTIO_BALLOON_F_FREE_PAGE_HINT as an alternative technique of
> temporarily inflating/deflating.
Reporting is like that. But hinting isn't, simply because
by the time host gets the hint page may already be in use.
Blowing it out unconditionally would lead to easily
reproducible guest crashes.
> E.g., we don't migrate any of these
> pages in the hypervisor, so the content will be zeroed out on the
> migration target.
Not exactly true. We do a delicate play with
the two dirty bits so they *are* migrated sometimes.
> If migration fails, the ld content will remain. You
> can call that "corrupting memory" - it's exactly what it has been
> invented for.
Well no, original author repeatedly stated it's "hinting"
because page can be in use actually.
>
> IIRC we decided to glue the semantics of VIRTIO_BALLOON_F_PAGE_POISON to
> VIRTIO_BALLOON_F_FREE_PAGE_HINT, which makes in my opinion perfect sense.
>
> So I propose:
>
> VIRTIO_BALLOON_F_PAGE_POISON not implemented in the hypervisor:
> - Pages inflated/deflated via VIRTIO_BALLOON_F_FREE_PAGE_HINT will
> either have the old page content or will be filled with 0.
> - This matches what we currently do.
>
> VIRTIO_BALLOON_F_PAGE_POISON implemented in the hypervisor:
> - Pages inflated/deflated via VIRTIO_BALLOON_F_FREE_PAGE_HINT will
> either have the old page content or will be filled with poison_val.
> - This matches what we should do also during ordinary
> inflation/deflation and free page reporting.
It's a reasonable option. however ...
> Again, nothing is currently broken as we free_page() the pages and don't
> care about an eventually changed page content.
I don't see how you can say that. ATM on a host without POISON
and with HINT, with poison val != 0 and with validation,
host can blow a free page away and then guest will detect
that as corruption.
If guest crashes then either guest or host are broken ;)
> It's only relevant once
> we ant to change that - and then we can rely on
> VIRTIO_BALLOON_F_PAGE_POISON.
>
> The problem is we cannot communicate the full situation to the
> hypervisor without the page poison feature being present. As such I
> would worry about that backfiring on us due to the hypervisor acting
> on incomplete data.
> >>>
> >>>
> >>> I'll try to think about VIRTIO_BALLOON_F_FREE_PAGE_HINT situation
> >>> over the weekend. But for the new page reporting, why not
> >>
> >> I shared my thoughts about VIRTIO_BALLOON_F_FREE_PAGE_HINT in the other
> >> thread and think we should simply not care at all for now.
> >>
> >>> assume host implementation will be sane?
> >>
> >> I don't think we should enforce having poison support around. See my
> >> reply to this mail for an alternative.
> >
> > OK so you basically say leave this to host to handle? That's more or
> > less what I'm saying too.
>
> Yes, for now. We should at some point change the guest to avoid
> re-po
Re: [virtio-dev] Re: [PATCH] virtio-balloon: Disable free page hinting/reporting if page poison is disabled
On 17.04.20 10:50, Michael S. Tsirkin wrote:
> On Fri, Apr 17, 2020 at 09:49:03AM +0200, David Hildenbrand wrote:
>> On 17.04.20 08:28, Michael S. Tsirkin wrote:
>>> On Thu, Apr 16, 2020 at 04:52:42PM -0700, Alexander Duyck wrote:
On Thu, Apr 16, 2020 at 3:13 PM Michael S. Tsirkin wrote:
>
> On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck wrote:
>> From: Alexander Duyck
>>
>> If we have free page hinting or page reporting enabled we should disable
>> it
>> if the pages are poisoned or initialized on free and we cannot notify the
>> hypervisor.
>>
>> Fixes: 5d757c8d518d ("virtio-balloon: add support for providing free
>> page reports to host")
>>
>> Signed-off-by: Alexander Duyck
>
>
> Why not put this logic in the hypervisor?
I did that too. This is to cover the case where somebody is running
the code prior to my QEMU changes where the page poison feature wasn't
being enabled.
>>>
>>>
>>> Hmm so that one looks like a QEMU bug (does not expose poison flag). In
>>> the past we just said need to fix the bug where it's found unless the
>>> issue is very widespread and major. Let's assume QEMU learns to always
>>> expose POISON with HINT. Then this configuration (HINT && !POISON)
>>> allows us to detect old QEMU (pre your changes).
>>
>> Don't see why this is a QEMU bug. It's just a feature it does not
>> implement. Perfectly valid.
>
> I'll need to think about this.
> 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 ...
I don't think that's true, and that's not what we currently implement in
the hypervisor for speeding up migration. I still consider
VIRTIO_BALLOON_F_FREE_PAGE_HINT as an alternative technique of
temporarily inflating/deflating. E.g., we don't migrate any of these
pages in the hypervisor, so the content will be zeroed out on the
migration target. If migration fails, the ld content will remain. You
can call that "corrupting memory" - it's exactly what it has been
invented for.
IIRC we decided to glue the semantics of VIRTIO_BALLOON_F_PAGE_POISON to
VIRTIO_BALLOON_F_FREE_PAGE_HINT, which makes in my opinion perfect sense.
So I propose:
VIRTIO_BALLOON_F_PAGE_POISON not implemented in the hypervisor:
- Pages inflated/deflated via VIRTIO_BALLOON_F_FREE_PAGE_HINT will
either have the old page content or will be filled with 0.
- This matches what we currently do.
VIRTIO_BALLOON_F_PAGE_POISON implemented in the hypervisor:
- Pages inflated/deflated via VIRTIO_BALLOON_F_FREE_PAGE_HINT will
either have the old page content or will be filled with poison_val.
- This matches what we should do also during ordinary
inflation/deflation and free page reporting.
Again, nothing is currently broken as we free_page() the pages and don't
care about an eventually changed page content. It's only relevant once
we ant to change that - and then we can rely on
VIRTIO_BALLOON_F_PAGE_POISON.
The problem is we cannot communicate the full situation to the
hypervisor without the page poison feature being present. As such I
would worry about that backfiring on us due to the hypervisor acting
on incomplete data.
>>>
>>>
>>> I'll try to think about VIRTIO_BALLOON_F_FREE_PAGE_HINT situation
>>> over the weekend. But for the new page reporting, why not
>>
>> I shared my thoughts about VIRTIO_BALLOON_F_FREE_PAGE_HINT in the other
>> thread and think we should simply not care at all for now.
>>
>>> assume host implementation will be sane?
>>
>> I don't think we should enforce having poison support around. See my
>> reply to this mail for an alternative.
>
> OK so you basically say leave this to host to handle? That's more or
> less what I'm saying too.
Yes, for now. We should at some point change the guest to avoid
re-poisoning/zeroing by not using free_page(). For now, I don't think
there is anything broken, it's just not as efficient as it could get at
this point - tolerable as we don't usually expect our guests to poison
pages or per-initialize them with zero.
--
Thanks,
David / dhildenb
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2] vhost: do not enable VHOST_MENU by default
On Fri, Apr 17, 2020 at 04:51:19PM +0800, Jason Wang wrote:
>
> On 2020/4/17 下午4:46, Michael S. Tsirkin wrote:
> > On Fri, Apr 17, 2020 at 04:39:49PM +0800, Jason Wang wrote:
> > > On 2020/4/17 下午4:29, Michael S. Tsirkin wrote:
> > > > On Fri, Apr 17, 2020 at 03:36:52PM +0800, Jason Wang wrote:
> > > > > On 2020/4/17 下午2:33, Michael S. Tsirkin wrote:
> > > > > > On Fri, Apr 17, 2020 at 11:12:14AM +0800, Jason Wang wrote:
> > > > > > > On 2020/4/17 上午6:55, Michael S. Tsirkin wrote:
> > > > > > > > On Wed, Apr 15, 2020 at 10:43:56AM +0800, Jason Wang wrote:
> > > > > > > > > We try to keep the defconfig untouched after decoupling
> > > > > > > > > CONFIG_VHOST
> > > > > > > > > out of CONFIG_VIRTUALIZATION in commit 20c384f1ea1a
> > > > > > > > > ("vhost: refine vhost and vringh kconfig") by enabling
> > > > > > > > > VHOST_MENU by
> > > > > > > > > default. Then the defconfigs can keep enabling
> > > > > > > > > CONFIG_VHOST_NET
> > > > > > > > > without the caring of CONFIG_VHOST.
> > > > > > > > >
> > > > > > > > > But this will leave a "CONFIG_VHOST_MENU=y" in all defconfigs
> > > > > > > > > and even
> > > > > > > > > for the ones that doesn't want vhost. So it actually shifts
> > > > > > > > > the
> > > > > > > > > burdens to the maintainers of all other to add
> > > > > > > > > "CONFIG_VHOST_MENU is
> > > > > > > > > not set". So this patch tries to enable CONFIG_VHOST
> > > > > > > > > explicitly in
> > > > > > > > > defconfigs that enables CONFIG_VHOST_NET and
> > > > > > > > > CONFIG_VHOST_VSOCK.
> > > > > > > > >
> > > > > > > > > Acked-by: Christian Borntraeger
> > > > > > > > > (s390)
> > > > > > > > > Acked-by: Michael Ellerman (powerpc)
> > > > > > > > > Cc: Thomas Bogendoerfer
> > > > > > > > > Cc: Benjamin Herrenschmidt
> > > > > > > > > Cc: Paul Mackerras
> > > > > > > > > Cc: Michael Ellerman
> > > > > > > > > Cc: Heiko Carstens
> > > > > > > > > Cc: Vasily Gorbik
> > > > > > > > > Cc: Christian Borntraeger
> > > > > > > > > Reported-by: Geert Uytterhoeven
> > > > > > > > > Signed-off-by: Jason Wang
> > > > > > > > I rebased this on top of OABI fix since that
> > > > > > > > seems more orgent to fix.
> > > > > > > > Pushed to my vhost branch pls take a look and
> > > > > > > > if possible test.
> > > > > > > > Thanks!
> > > > > > > I test this patch by generating the defconfigs that wants
> > > > > > > vhost_net or
> > > > > > > vhost_vsock. All looks fine.
> > > > > > >
> > > > > > > But having CONFIG_VHOST_DPN=y may end up with the similar
> > > > > > > situation that
> > > > > > > this patch want to address.
> > > > > > > Maybe we can let CONFIG_VHOST depends on !ARM || AEABI then add
> > > > > > > another
> > > > > > > menuconfig for VHOST_RING and do something similar?
> > > > > > >
> > > > > > > Thanks
> > > > > > Sorry I don't understand. After this patch CONFIG_VHOST_DPN is just
> > > > > > an internal variable for the OABI fix. I kept it separate
> > > > > > so it's easy to revert for 5.8. Yes we could squash it into
> > > > > > VHOST directly but I don't see how that changes logic at all.
> > > > > Sorry for being unclear.
> > > > >
> > > > > I meant since it was enabled by default, "CONFIG_VHOST_DPN=y" will be
> > > > > left
> > > > > in the defconfigs.
> > > > But who cares?
> > > FYI, please seehttps://www.spinics.net/lists/kvm/msg212685.html
> > The complaint was not about the symbol IIUC. It was that we caused
> > everyone to build vhost unless they manually disabled it.
>
>
> There could be some misunderstanding here. I thought it's somehow similar: a
> CONFIG_VHOST_MENU=y will be left in the defconfigs even if CONFIG_VHOST is
> not set.
>
> Thanks
>
BTW do entries with no prompt actually appear in defconfig?
> >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2] vhost: do not enable VHOST_MENU by default
On Fri, Apr 17, 2020 at 04:51:19PM +0800, Jason Wang wrote:
>
> On 2020/4/17 下午4:46, Michael S. Tsirkin wrote:
> > On Fri, Apr 17, 2020 at 04:39:49PM +0800, Jason Wang wrote:
> > > On 2020/4/17 下午4:29, Michael S. Tsirkin wrote:
> > > > On Fri, Apr 17, 2020 at 03:36:52PM +0800, Jason Wang wrote:
> > > > > On 2020/4/17 下午2:33, Michael S. Tsirkin wrote:
> > > > > > On Fri, Apr 17, 2020 at 11:12:14AM +0800, Jason Wang wrote:
> > > > > > > On 2020/4/17 上午6:55, Michael S. Tsirkin wrote:
> > > > > > > > On Wed, Apr 15, 2020 at 10:43:56AM +0800, Jason Wang wrote:
> > > > > > > > > We try to keep the defconfig untouched after decoupling
> > > > > > > > > CONFIG_VHOST
> > > > > > > > > out of CONFIG_VIRTUALIZATION in commit 20c384f1ea1a
> > > > > > > > > ("vhost: refine vhost and vringh kconfig") by enabling
> > > > > > > > > VHOST_MENU by
> > > > > > > > > default. Then the defconfigs can keep enabling
> > > > > > > > > CONFIG_VHOST_NET
> > > > > > > > > without the caring of CONFIG_VHOST.
> > > > > > > > >
> > > > > > > > > But this will leave a "CONFIG_VHOST_MENU=y" in all defconfigs
> > > > > > > > > and even
> > > > > > > > > for the ones that doesn't want vhost. So it actually shifts
> > > > > > > > > the
> > > > > > > > > burdens to the maintainers of all other to add
> > > > > > > > > "CONFIG_VHOST_MENU is
> > > > > > > > > not set". So this patch tries to enable CONFIG_VHOST
> > > > > > > > > explicitly in
> > > > > > > > > defconfigs that enables CONFIG_VHOST_NET and
> > > > > > > > > CONFIG_VHOST_VSOCK.
> > > > > > > > >
> > > > > > > > > Acked-by: Christian Borntraeger
> > > > > > > > > (s390)
> > > > > > > > > Acked-by: Michael Ellerman (powerpc)
> > > > > > > > > Cc: Thomas Bogendoerfer
> > > > > > > > > Cc: Benjamin Herrenschmidt
> > > > > > > > > Cc: Paul Mackerras
> > > > > > > > > Cc: Michael Ellerman
> > > > > > > > > Cc: Heiko Carstens
> > > > > > > > > Cc: Vasily Gorbik
> > > > > > > > > Cc: Christian Borntraeger
> > > > > > > > > Reported-by: Geert Uytterhoeven
> > > > > > > > > Signed-off-by: Jason Wang
> > > > > > > > I rebased this on top of OABI fix since that
> > > > > > > > seems more orgent to fix.
> > > > > > > > Pushed to my vhost branch pls take a look and
> > > > > > > > if possible test.
> > > > > > > > Thanks!
> > > > > > > I test this patch by generating the defconfigs that wants
> > > > > > > vhost_net or
> > > > > > > vhost_vsock. All looks fine.
> > > > > > >
> > > > > > > But having CONFIG_VHOST_DPN=y may end up with the similar
> > > > > > > situation that
> > > > > > > this patch want to address.
> > > > > > > Maybe we can let CONFIG_VHOST depends on !ARM || AEABI then add
> > > > > > > another
> > > > > > > menuconfig for VHOST_RING and do something similar?
> > > > > > >
> > > > > > > Thanks
> > > > > > Sorry I don't understand. After this patch CONFIG_VHOST_DPN is just
> > > > > > an internal variable for the OABI fix. I kept it separate
> > > > > > so it's easy to revert for 5.8. Yes we could squash it into
> > > > > > VHOST directly but I don't see how that changes logic at all.
> > > > > Sorry for being unclear.
> > > > >
> > > > > I meant since it was enabled by default, "CONFIG_VHOST_DPN=y" will be
> > > > > left
> > > > > in the defconfigs.
> > > > But who cares?
> > > FYI, please seehttps://www.spinics.net/lists/kvm/msg212685.html
> > The complaint was not about the symbol IIUC. It was that we caused
> > everyone to build vhost unless they manually disabled it.
>
>
> There could be some misunderstanding here. I thought it's somehow similar: a
> CONFIG_VHOST_MENU=y will be left in the defconfigs even if CONFIG_VHOST is
> not set.
>
> Thanks
Hmm. So looking at Documentation/kbuild/kconfig-language.rst :
Things that merit "default y/m" include:
a) A new Kconfig option for something that used to always be built
should be "default y".
b) A new gatekeeping Kconfig option that hides/shows other Kconfig
options (but does not generate any code of its own), should be
"default y" so people will see those other options.
c) Sub-driver behavior or similar options for a driver that is
"default n". This allows you to provide sane defaults.
So it looks like VHOST_MENU is actually matching rule b).
So what's the problem we are trying to solve with this patch, exactly?
Geert could you clarify pls?
>
> >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2] vhost: do not enable VHOST_MENU by default
On 2020/4/17 下午4:46, Michael S. Tsirkin wrote:
On Fri, Apr 17, 2020 at 04:39:49PM +0800, Jason Wang wrote:
On 2020/4/17 下午4:29, Michael S. Tsirkin wrote:
On Fri, Apr 17, 2020 at 03:36:52PM +0800, Jason Wang wrote:
On 2020/4/17 下午2:33, Michael S. Tsirkin wrote:
On Fri, Apr 17, 2020 at 11:12:14AM +0800, Jason Wang wrote:
On 2020/4/17 上午6:55, Michael S. Tsirkin wrote:
On Wed, Apr 15, 2020 at 10:43:56AM +0800, Jason Wang wrote:
We try to keep the defconfig untouched after decoupling CONFIG_VHOST
out of CONFIG_VIRTUALIZATION in commit 20c384f1ea1a
("vhost: refine vhost and vringh kconfig") by enabling VHOST_MENU by
default. Then the defconfigs can keep enabling CONFIG_VHOST_NET
without the caring of CONFIG_VHOST.
But this will leave a "CONFIG_VHOST_MENU=y" in all defconfigs and even
for the ones that doesn't want vhost. So it actually shifts the
burdens to the maintainers of all other to add "CONFIG_VHOST_MENU is
not set". So this patch tries to enable CONFIG_VHOST explicitly in
defconfigs that enables CONFIG_VHOST_NET and CONFIG_VHOST_VSOCK.
Acked-by: Christian Borntraeger (s390)
Acked-by: Michael Ellerman (powerpc)
Cc: Thomas Bogendoerfer
Cc: Benjamin Herrenschmidt
Cc: Paul Mackerras
Cc: Michael Ellerman
Cc: Heiko Carstens
Cc: Vasily Gorbik
Cc: Christian Borntraeger
Reported-by: Geert Uytterhoeven
Signed-off-by: Jason Wang
I rebased this on top of OABI fix since that
seems more orgent to fix.
Pushed to my vhost branch pls take a look and
if possible test.
Thanks!
I test this patch by generating the defconfigs that wants vhost_net or
vhost_vsock. All looks fine.
But having CONFIG_VHOST_DPN=y may end up with the similar situation that
this patch want to address.
Maybe we can let CONFIG_VHOST depends on !ARM || AEABI then add another
menuconfig for VHOST_RING and do something similar?
Thanks
Sorry I don't understand. After this patch CONFIG_VHOST_DPN is just
an internal variable for the OABI fix. I kept it separate
so it's easy to revert for 5.8. Yes we could squash it into
VHOST directly but I don't see how that changes logic at all.
Sorry for being unclear.
I meant since it was enabled by default, "CONFIG_VHOST_DPN=y" will be left
in the defconfigs.
But who cares?
FYI, please seehttps://www.spinics.net/lists/kvm/msg212685.html
The complaint was not about the symbol IIUC. It was that we caused
everyone to build vhost unless they manually disabled it.
There could be some misunderstanding here. I thought it's somehow
similar: a CONFIG_VHOST_MENU=y will be left in the defconfigs even if
CONFIG_VHOST is not set.
Thanks
___
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 Fri, Apr 17, 2020 at 09:49:03AM +0200, David Hildenbrand wrote:
> On 17.04.20 08:28, Michael S. Tsirkin wrote:
> > On Thu, Apr 16, 2020 at 04:52:42PM -0700, Alexander Duyck wrote:
> >> On Thu, Apr 16, 2020 at 3:13 PM Michael S. Tsirkin wrote:
> >>>
> >>> On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck wrote:
> From: Alexander Duyck
>
> If we have free page hinting or page reporting enabled we should disable
> it
> if the pages are poisoned or initialized on free and we cannot notify the
> hypervisor.
>
> Fixes: 5d757c8d518d ("virtio-balloon: add support for providing free
> page reports to host")
>
> Signed-off-by: Alexander Duyck
> >>>
> >>>
> >>> Why not put this logic in the hypervisor?
> >>
> >> I did that too. This is to cover the case where somebody is running
> >> the code prior to my QEMU changes where the page poison feature wasn't
> >> being enabled.
> >
> >
> > Hmm so that one looks like a QEMU bug (does not expose poison flag). In
> > the past we just said need to fix the bug where it's found unless the
> > issue is very widespread and major. Let's assume QEMU learns to always
> > expose POISON with HINT. Then this configuration (HINT && !POISON)
> > allows us to detect old QEMU (pre your changes).
>
> Don't see why this is a QEMU bug. It's just a feature it does not
> implement. Perfectly valid.
I'll need to think about this.
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 ...
> [...]
> >>
> >> The problem is we cannot communicate the full situation to the
> >> hypervisor without the page poison feature being present. As such I
> >> would worry about that backfiring on us due to the hypervisor acting
> >> on incomplete data.
> >
> >
> > I'll try to think about VIRTIO_BALLOON_F_FREE_PAGE_HINT situation
> > over the weekend. But for the new page reporting, why not
>
> I shared my thoughts about VIRTIO_BALLOON_F_FREE_PAGE_HINT in the other
> thread and think we should simply not care at all for now.
>
> > assume host implementation will be sane?
>
> I don't think we should enforce having poison support around. See my
> reply to this mail for an alternative.
OK so you basically say leave this to host to handle? That's more or
less what I'm saying too.
> --
> Thanks,
>
> David / dhildenb
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2] vhost: do not enable VHOST_MENU by default
On Fri, Apr 17, 2020 at 04:39:49PM +0800, Jason Wang wrote:
>
> On 2020/4/17 下午4:29, Michael S. Tsirkin wrote:
> > On Fri, Apr 17, 2020 at 03:36:52PM +0800, Jason Wang wrote:
> > > On 2020/4/17 下午2:33, Michael S. Tsirkin wrote:
> > > > On Fri, Apr 17, 2020 at 11:12:14AM +0800, Jason Wang wrote:
> > > > > On 2020/4/17 上午6:55, Michael S. Tsirkin wrote:
> > > > > > On Wed, Apr 15, 2020 at 10:43:56AM +0800, Jason Wang wrote:
> > > > > > > We try to keep the defconfig untouched after decoupling
> > > > > > > CONFIG_VHOST
> > > > > > > out of CONFIG_VIRTUALIZATION in commit 20c384f1ea1a
> > > > > > > ("vhost: refine vhost and vringh kconfig") by enabling VHOST_MENU
> > > > > > > by
> > > > > > > default. Then the defconfigs can keep enabling CONFIG_VHOST_NET
> > > > > > > without the caring of CONFIG_VHOST.
> > > > > > >
> > > > > > > But this will leave a "CONFIG_VHOST_MENU=y" in all defconfigs and
> > > > > > > even
> > > > > > > for the ones that doesn't want vhost. So it actually shifts the
> > > > > > > burdens to the maintainers of all other to add "CONFIG_VHOST_MENU
> > > > > > > is
> > > > > > > not set". So this patch tries to enable CONFIG_VHOST explicitly in
> > > > > > > defconfigs that enables CONFIG_VHOST_NET and CONFIG_VHOST_VSOCK.
> > > > > > >
> > > > > > > Acked-by: Christian Borntraeger (s390)
> > > > > > > Acked-by: Michael Ellerman (powerpc)
> > > > > > > Cc: Thomas Bogendoerfer
> > > > > > > Cc: Benjamin Herrenschmidt
> > > > > > > Cc: Paul Mackerras
> > > > > > > Cc: Michael Ellerman
> > > > > > > Cc: Heiko Carstens
> > > > > > > Cc: Vasily Gorbik
> > > > > > > Cc: Christian Borntraeger
> > > > > > > Reported-by: Geert Uytterhoeven
> > > > > > > Signed-off-by: Jason Wang
> > > > > > I rebased this on top of OABI fix since that
> > > > > > seems more orgent to fix.
> > > > > > Pushed to my vhost branch pls take a look and
> > > > > > if possible test.
> > > > > > Thanks!
> > > > > I test this patch by generating the defconfigs that wants vhost_net or
> > > > > vhost_vsock. All looks fine.
> > > > >
> > > > > But having CONFIG_VHOST_DPN=y may end up with the similar situation
> > > > > that
> > > > > this patch want to address.
> > > > > Maybe we can let CONFIG_VHOST depends on !ARM || AEABI then add
> > > > > another
> > > > > menuconfig for VHOST_RING and do something similar?
> > > > >
> > > > > Thanks
> > > > Sorry I don't understand. After this patch CONFIG_VHOST_DPN is just
> > > > an internal variable for the OABI fix. I kept it separate
> > > > so it's easy to revert for 5.8. Yes we could squash it into
> > > > VHOST directly but I don't see how that changes logic at all.
> > >
> > > Sorry for being unclear.
> > >
> > > I meant since it was enabled by default, "CONFIG_VHOST_DPN=y" will be left
> > > in the defconfigs.
> > But who cares?
>
>
> FYI, please see https://www.spinics.net/lists/kvm/msg212685.html
The complaint was not about the symbol IIUC. It was that we caused
everyone to build vhost unless they manually disabled it.
>
> > That does not add any code, does it?
>
>
> It doesn't.
>
> Thanks
>
>
> >
> > > This requires the arch maintainers to add
> > > "CONFIG_VHOST_VDPN is not set". (Geert complains about this)
> > >
> > > Thanks
> > >
> > >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2] vhost: do not enable VHOST_MENU by default
On 2020/4/17 下午4:29, Michael S. Tsirkin wrote:
On Fri, Apr 17, 2020 at 03:36:52PM +0800, Jason Wang wrote:
On 2020/4/17 下午2:33, Michael S. Tsirkin wrote:
On Fri, Apr 17, 2020 at 11:12:14AM +0800, Jason Wang wrote:
On 2020/4/17 上午6:55, Michael S. Tsirkin wrote:
On Wed, Apr 15, 2020 at 10:43:56AM +0800, Jason Wang wrote:
We try to keep the defconfig untouched after decoupling CONFIG_VHOST
out of CONFIG_VIRTUALIZATION in commit 20c384f1ea1a
("vhost: refine vhost and vringh kconfig") by enabling VHOST_MENU by
default. Then the defconfigs can keep enabling CONFIG_VHOST_NET
without the caring of CONFIG_VHOST.
But this will leave a "CONFIG_VHOST_MENU=y" in all defconfigs and even
for the ones that doesn't want vhost. So it actually shifts the
burdens to the maintainers of all other to add "CONFIG_VHOST_MENU is
not set". So this patch tries to enable CONFIG_VHOST explicitly in
defconfigs that enables CONFIG_VHOST_NET and CONFIG_VHOST_VSOCK.
Acked-by: Christian Borntraeger (s390)
Acked-by: Michael Ellerman (powerpc)
Cc: Thomas Bogendoerfer
Cc: Benjamin Herrenschmidt
Cc: Paul Mackerras
Cc: Michael Ellerman
Cc: Heiko Carstens
Cc: Vasily Gorbik
Cc: Christian Borntraeger
Reported-by: Geert Uytterhoeven
Signed-off-by: Jason Wang
I rebased this on top of OABI fix since that
seems more orgent to fix.
Pushed to my vhost branch pls take a look and
if possible test.
Thanks!
I test this patch by generating the defconfigs that wants vhost_net or
vhost_vsock. All looks fine.
But having CONFIG_VHOST_DPN=y may end up with the similar situation that
this patch want to address.
Maybe we can let CONFIG_VHOST depends on !ARM || AEABI then add another
menuconfig for VHOST_RING and do something similar?
Thanks
Sorry I don't understand. After this patch CONFIG_VHOST_DPN is just
an internal variable for the OABI fix. I kept it separate
so it's easy to revert for 5.8. Yes we could squash it into
VHOST directly but I don't see how that changes logic at all.
Sorry for being unclear.
I meant since it was enabled by default, "CONFIG_VHOST_DPN=y" will be left
in the defconfigs.
But who cares?
FYI, please see https://www.spinics.net/lists/kvm/msg212685.html
That does not add any code, does it?
It doesn't.
Thanks
This requires the arch maintainers to add
"CONFIG_VHOST_VDPN is not set". (Geert complains about this)
Thanks
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2] vhost: do not enable VHOST_MENU by default
On Fri, Apr 17, 2020 at 03:36:52PM +0800, Jason Wang wrote:
>
> On 2020/4/17 下午2:33, Michael S. Tsirkin wrote:
> > On Fri, Apr 17, 2020 at 11:12:14AM +0800, Jason Wang wrote:
> > > On 2020/4/17 上午6:55, Michael S. Tsirkin wrote:
> > > > On Wed, Apr 15, 2020 at 10:43:56AM +0800, Jason Wang wrote:
> > > > > We try to keep the defconfig untouched after decoupling CONFIG_VHOST
> > > > > out of CONFIG_VIRTUALIZATION in commit 20c384f1ea1a
> > > > > ("vhost: refine vhost and vringh kconfig") by enabling VHOST_MENU by
> > > > > default. Then the defconfigs can keep enabling CONFIG_VHOST_NET
> > > > > without the caring of CONFIG_VHOST.
> > > > >
> > > > > But this will leave a "CONFIG_VHOST_MENU=y" in all defconfigs and even
> > > > > for the ones that doesn't want vhost. So it actually shifts the
> > > > > burdens to the maintainers of all other to add "CONFIG_VHOST_MENU is
> > > > > not set". So this patch tries to enable CONFIG_VHOST explicitly in
> > > > > defconfigs that enables CONFIG_VHOST_NET and CONFIG_VHOST_VSOCK.
> > > > >
> > > > > Acked-by: Christian Borntraeger (s390)
> > > > > Acked-by: Michael Ellerman (powerpc)
> > > > > Cc: Thomas Bogendoerfer
> > > > > Cc: Benjamin Herrenschmidt
> > > > > Cc: Paul Mackerras
> > > > > Cc: Michael Ellerman
> > > > > Cc: Heiko Carstens
> > > > > Cc: Vasily Gorbik
> > > > > Cc: Christian Borntraeger
> > > > > Reported-by: Geert Uytterhoeven
> > > > > Signed-off-by: Jason Wang
> > > > I rebased this on top of OABI fix since that
> > > > seems more orgent to fix.
> > > > Pushed to my vhost branch pls take a look and
> > > > if possible test.
> > > > Thanks!
> > >
> > > I test this patch by generating the defconfigs that wants vhost_net or
> > > vhost_vsock. All looks fine.
> > >
> > > But having CONFIG_VHOST_DPN=y may end up with the similar situation that
> > > this patch want to address.
> > > Maybe we can let CONFIG_VHOST depends on !ARM || AEABI then add another
> > > menuconfig for VHOST_RING and do something similar?
> > >
> > > Thanks
> > Sorry I don't understand. After this patch CONFIG_VHOST_DPN is just
> > an internal variable for the OABI fix. I kept it separate
> > so it's easy to revert for 5.8. Yes we could squash it into
> > VHOST directly but I don't see how that changes logic at all.
>
>
> Sorry for being unclear.
>
> I meant since it was enabled by default, "CONFIG_VHOST_DPN=y" will be left
> in the defconfigs.
But who cares? That does not add any code, does it?
> This requires the arch maintainers to add
> "CONFIG_VHOST_VDPN is not set". (Geert complains about this)
>
> Thanks
>
>
> >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 7/8] tools/virtio: Reset index in virtio_test --reset.
On Fri, Apr 17, 2020 at 09:04:04AM +0200, Eugenio Perez Martin wrote:
> On Fri, Apr 17, 2020 at 12:34 AM Michael S. Tsirkin wrote:
> >
> > On Thu, Apr 16, 2020 at 09:56:42AM +0200, Eugenio Pérez wrote:
> > > This way behavior for vhost is more like a VM.
> > >
> > > Signed-off-by: Eugenio Pérez
> >
> > I dropped --reset from 5.7 since Linus felt it's unappropriate.
> > I guess I should squash this in with --reset?
> >
>
> Yes please.
>
> If you prefer I can do it using the base you want, so all commits
> messages are right.
>
> Thanks!
OK so I dropped new tests from vhost for now, pls rebased on
top of that.
Thanks!
> > > ---
> > > tools/virtio/virtio_test.c | 33 ++---
> > > 1 file changed, 26 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/tools/virtio/virtio_test.c b/tools/virtio/virtio_test.c
> > > index 18d5347003eb..dca64d36a882 100644
> > > --- a/tools/virtio/virtio_test.c
> > > +++ b/tools/virtio/virtio_test.c
> > > @@ -20,7 +20,6 @@
> > > #include "../../drivers/vhost/test.h"
> > >
> > > #define RANDOM_BATCH -1
> > > -#define RANDOM_RESET -1
> > >
> > > /* Unused */
> > > void *__kmalloc_fake, *__kfree_ignore_start, *__kfree_ignore_end;
> > > @@ -49,6 +48,7 @@ struct vdev_info {
> > >
> > > static const struct vhost_vring_file no_backend = { .fd = -1 },
> > >backend = { .fd = 1 };
> > > +static const struct vhost_vring_state null_state = {};
> > >
> > > bool vq_notify(struct virtqueue *vq)
> > > {
> > > @@ -174,14 +174,19 @@ static void run_test(struct vdev_info *dev, struct
> > > vq_info *vq,
> > > unsigned len;
> > > long long spurious = 0;
> > > const bool random_batch = batch == RANDOM_BATCH;
> > > +
> > > r = ioctl(dev->control, VHOST_TEST_RUN, &test);
> > > assert(r >= 0);
> > > + if (!reset_n) {
> > > + next_reset = INT_MAX;
> > > + }
> > > +
> > > for (;;) {
> > > virtqueue_disable_cb(vq->vq);
> > > completed_before = completed;
> > > started_before = started;
> > > do {
> > > - const bool reset = reset_n && completed >
> > > next_reset;
> > > + const bool reset = completed > next_reset;
> > > if (random_batch)
> > > batch = (random() % vq->vring.num) + 1;
> > >
> > > @@ -224,10 +229,24 @@ static void run_test(struct vdev_info *dev, struct
> > > vq_info *vq,
> > > }
> > >
> > > if (reset) {
> > > + struct vhost_vring_state s = { .index = 0 };
> > > +
> > > + vq_reset(vq, vq->vring.num, &dev->vdev);
> > > +
> > > + r = ioctl(dev->control,
> > > VHOST_GET_VRING_BASE,
> > > + &s);
> > > + assert(!r);
> > > +
> > > + s.num = 0;
> > > + r = ioctl(dev->control,
> > > VHOST_SET_VRING_BASE,
> > > + &null_state);
> > > + assert(!r);
> > > +
> > > r = ioctl(dev->control,
> > > VHOST_TEST_SET_BACKEND,
> > > &backend);
> > > assert(!r);
> > >
> > > + started = completed;
> > > while (completed > next_reset)
> > > next_reset += completed;
> > > }
> > > @@ -249,7 +268,9 @@ static void run_test(struct vdev_info *dev, struct
> > > vq_info *vq,
> > > test = 0;
> > > r = ioctl(dev->control, VHOST_TEST_RUN, &test);
> > > assert(r >= 0);
> > > - fprintf(stderr, "spurious wakeups: 0x%llx\n", spurious);
> > > + fprintf(stderr,
> > > + "spurious wakeups: 0x%llx started=0x%lx completed=0x%lx\n",
> > > + spurious, started, completed);
> > > }
> > >
> > > const char optstring[] = "h";
> > > @@ -312,7 +333,7 @@ static void help(void)
> > > " [--no-virtio-1]"
> > > " [--delayed-interrupt]"
> > > " [--batch=random/N]"
> > > - " [--reset=random/N]"
> > > + " [--reset=N]"
> > > "\n");
> > > }
> > >
> > > @@ -360,11 +381,9 @@ int main(int argc, char **argv)
> > > case 'r':
> > > if (!optarg) {
> > > reset = 1;
> > > - } else if (0 == strcmp(optarg, "random")) {
> > > - reset = RANDOM_RESET;
> > > } else {
> > > reset = strtol(optarg, NULL, 10);
> > > - assert(reset >= 0);
> > > + assert(reset > 0);
> > > assert(reset < (long)
[PATCH v9] virtio: force spec specified alignment on types
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
---
I am still not sure this is even needed. Does compiler make
optimizations based on ABI alignment assumptions?
Changes from v8:
- better commit log
- go back to vring in UAPI
drivers/vhost/vhost.h| 6 ++---
include/uapi/linux/virtio_ring.h | 38 +++-
2 files changed, 31 insertions(+), 13 deletions(-)
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/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
index 9223c3a5c46a..177227f0d9cd 100644
--- a/include/uapi/linux/virtio_ring.h
+++ b/include/uapi/linux/virtio_ring.h
@@ -118,16 +118,6 @@ struct vring_used {
struct vring_used_elem ring[];
};
-struct vring {
- unsigned int num;
-
- struct vring_desc *desc;
-
- struct vring_avail *avail;
-
- struct vring_used *used;
-};
-
/* Alignment requirements for vring elements.
* When using pre-virtio 1.0 layout, these fall out naturally.
*/
@@ -135,6 +125,34 @@ struct vring {
#define VRING_USED_ALIGN_SIZE 4
#define VRING_DESC_ALIGN_SIZE 16
+/*
+ * The ring element addresses are passed between components with different
+ * alignments assumptions. Thus, we might need to decrease the
compiler-selected
+ * alignment, and so must use a typedef to make sure the __aligned attribute
+ * actually takes hold:
+ *
+ *
https://gcc.gnu.org/onlinedocs//gcc/Common-Type-Attributes.html#Common-Type-Attributes
+ *
+ * When used on a struct, or struct member, the aligned attribute can only
+ * increase the alignment; in order to decrease it, the packed attribute must
+ * be specified as well. When used as part of a typedef, the aligned attribute
+ * can both increase and decrease alignment, and specifying the packed
+ * attribute generates a warning.
+ */
+typedef struct vring_desc __aligned(VRING_DESC_ALIGN_SIZE) vring_desc_t;
+typedef struct vring_avail __aligned(VRING_AVAIL_ALIGN_SIZE) vring_avail_t;
+typedef struct vring_used __aligned(VRING_USED_ALIGN_SIZE) vring_used_t;
+
+struct vring {
+ unsigned int num;
+
+ vring_desc_t *desc;
+
+ vring_avail_t *avail;
+
+ vring_used_t *used;
+};
+
#ifndef VIRTIO_RING_NO_LEGACY
/* The standard layout for the ring is a continuous chunk of memory which looks
--
MST
___
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 17.04.20 08:28, Michael S. Tsirkin wrote:
> On Thu, Apr 16, 2020 at 04:52:42PM -0700, Alexander Duyck wrote:
>> On Thu, Apr 16, 2020 at 3:13 PM Michael S. Tsirkin wrote:
>>>
>>> On Thu, Apr 16, 2020 at 12:30:38PM -0700, Alexander Duyck wrote:
From: Alexander Duyck
If we have free page hinting or page reporting enabled we should disable it
if the pages are poisoned or initialized on free and we cannot notify the
hypervisor.
Fixes: 5d757c8d518d ("virtio-balloon: add support for providing free page
reports to host")
Signed-off-by: Alexander Duyck
>>>
>>>
>>> Why not put this logic in the hypervisor?
>>
>> I did that too. This is to cover the case where somebody is running
>> the code prior to my QEMU changes where the page poison feature wasn't
>> being enabled.
>
>
> Hmm so that one looks like a QEMU bug (does not expose poison flag). In
> the past we just said need to fix the bug where it's found unless the
> issue is very widespread and major. Let's assume QEMU learns to always
> expose POISON with HINT. Then this configuration (HINT && !POISON)
> allows us to detect old QEMU (pre your changes).
Don't see why this is a QEMU bug. It's just a feature it does not
implement. Perfectly valid.
[...]
>>
>> The problem is we cannot communicate the full situation to the
>> hypervisor without the page poison feature being present. As such I
>> would worry about that backfiring on us due to the hypervisor acting
>> on incomplete data.
>
>
> I'll try to think about VIRTIO_BALLOON_F_FREE_PAGE_HINT situation
> over the weekend. But for the new page reporting, why not
I shared my thoughts about VIRTIO_BALLOON_F_FREE_PAGE_HINT in the other
thread and think we should simply not care at all for now.
> assume host implementation will be sane?
I don't think we should enforce having poison support around. See my
reply to this mail for an alternative.
--
Thanks,
David / dhildenb
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] MAINTAINERS: Update PARAVIRT_OPS_INTERFACE and VMWARE_HYPERVISOR_INTERFACE
On 17.04.20 08:22, Jürgen Groß wrote: On 17.04.20 01:45, Deep Shah wrote: Thomas Hellstrom will be handing over VMware's maintainership of these interfaces to Deep Shah. Signed-off-by: Deep Shah Acked-by: Thomas Hellstrom Acked-by: Juergen Gross BTW, I can carry this patch through the Xen tree if nobody else wants to take it... Juergen ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-balloon: Disable free page hinting/reporting if page poison is disabled
On 16.04.20 21:30, Alexander Duyck wrote:
> From: Alexander Duyck
>
> If we have free page hinting or page reporting enabled we should disable it
> if the pages are poisoned or initialized on free and we cannot notify the
> hypervisor.
>
Please split that up into an actual fix (Fixes:) for free page reporting
and an optimization for free page hinting. Also, please document why we
are doing stuff.
Regarding the free page reporting part, I propose something like this instead
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 0ef16566c3f3..9b1e56da1e29 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -1107,6 +1107,20 @@ static int virtballoon_restore(struct virtio_device
*vdev)
static int virtballoon_validate(struct virtio_device *vdev)
{
+ /*
+* Free page reporting relies on the fact that any access after
+* pages were reported (esp. from the buddy) will result in them getting
+* deflated automatically. In case we care about the page content, we
+* need support from the hypervisor to provide us with the same page
+* (poisoned) content we originally had in the page. Without
+* VIRTIO_BALLOON_F_PAGE_POISON, we will receive either the original
+* page or a zeroed page.
+*/
+ if (!virtio_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON) &&
+ !IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY) &&
+ page_poisoning_enabled())
+ __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_REPORTING);
+
/* Tell the host whether we care about poisoned pages. */
if (!want_init_on_free() &&
(IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY) ||
> Fixes: 5d757c8d518d ("virtio-balloon: add support for providing free page
> reports to host")
>
> Signed-off-by: Alexander Duyck
> ---
> drivers/virtio/virtio_balloon.c |6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 95d9c2f0a7be..08bc86a6e468 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -1110,8 +1110,12 @@ static int virtballoon_validate(struct virtio_device
> *vdev)
> /* Tell the host whether we care about poisoned pages. */
> if (!want_init_on_free() &&
> (IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY) ||
> - !page_poisoning_enabled()))
> + !page_poisoning_enabled())) {
> __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
> + } else if (!virtio_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
> + __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT);
> + __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_REPORTING);
> + }
>
> __virtio_clear_bit(vdev, VIRTIO_F_IOMMU_PLATFORM);
> return 0;
>
--
Thanks,
David / dhildenb
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2] vhost: do not enable VHOST_MENU by default
On 2020/4/17 下午2:33, Michael S. Tsirkin wrote:
On Fri, Apr 17, 2020 at 11:12:14AM +0800, Jason Wang wrote:
On 2020/4/17 上午6:55, Michael S. Tsirkin wrote:
On Wed, Apr 15, 2020 at 10:43:56AM +0800, Jason Wang wrote:
We try to keep the defconfig untouched after decoupling CONFIG_VHOST
out of CONFIG_VIRTUALIZATION in commit 20c384f1ea1a
("vhost: refine vhost and vringh kconfig") by enabling VHOST_MENU by
default. Then the defconfigs can keep enabling CONFIG_VHOST_NET
without the caring of CONFIG_VHOST.
But this will leave a "CONFIG_VHOST_MENU=y" in all defconfigs and even
for the ones that doesn't want vhost. So it actually shifts the
burdens to the maintainers of all other to add "CONFIG_VHOST_MENU is
not set". So this patch tries to enable CONFIG_VHOST explicitly in
defconfigs that enables CONFIG_VHOST_NET and CONFIG_VHOST_VSOCK.
Acked-by: Christian Borntraeger (s390)
Acked-by: Michael Ellerman (powerpc)
Cc: Thomas Bogendoerfer
Cc: Benjamin Herrenschmidt
Cc: Paul Mackerras
Cc: Michael Ellerman
Cc: Heiko Carstens
Cc: Vasily Gorbik
Cc: Christian Borntraeger
Reported-by: Geert Uytterhoeven
Signed-off-by: Jason Wang
I rebased this on top of OABI fix since that
seems more orgent to fix.
Pushed to my vhost branch pls take a look and
if possible test.
Thanks!
I test this patch by generating the defconfigs that wants vhost_net or
vhost_vsock. All looks fine.
But having CONFIG_VHOST_DPN=y may end up with the similar situation that
this patch want to address.
Maybe we can let CONFIG_VHOST depends on !ARM || AEABI then add another
menuconfig for VHOST_RING and do something similar?
Thanks
Sorry I don't understand. After this patch CONFIG_VHOST_DPN is just
an internal variable for the OABI fix. I kept it separate
so it's easy to revert for 5.8. Yes we could squash it into
VHOST directly but I don't see how that changes logic at all.
Sorry for being unclear.
I meant since it was enabled by default, "CONFIG_VHOST_DPN=y" will be
left in the defconfigs. This requires the arch maintainers to add
"CONFIG_VHOST_VDPN is not set". (Geert complains about this)
Thanks
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
