On Thu, Jun 07, 2018 at 01:29:22PM +0800, Wei Wang wrote:
> On 06/07/2018 11:17 AM, Peter Xu wrote:
> > On Wed, Jun 06, 2018 at 06:11:50PM +0800, Wei Wang wrote:
> >
> > I got similar comments from Michael, and it will be
> > while (1) {
> > lock;
> > func();
> > unlock();
> > }
> >
> > All the unlock inside the body will be gone.
> > Ok I think I have more question on this part...
> >
> > Actually AFAICT this new feature uses iothread in a way very similar
> > to the block layer, so I digged a bit on how block layer used the
> > iothreads. I see that the block code is using something like
> > virtio_queue_aio_set_host_notifier_handler() to hook up the
> > iothread/aiocontext and the ioeventfd, however here you are manually
> > creating one QEMUBH and bound that to the new context. Should you
> > also use something like the block layer? Then IMHO you can avoid
> > using a busy loop there (assuming the performance does not really
> > matter that much here for page hintings), and all the packet handling
> > can again be based on interrupts from the guest (ioeventfd).
> >
> > [1]
>
> Also mentioned in another discussion thread that it's better to not let
> guest send notifications. Otherwise, we would have used the virtqueue door
> bell to notify host.
> So we need to use polling here, and Michael suggested to implemented in BH,
> which sounds good to me.
(We're discussing the same problem in the other thread, so let's do it
there)
>
>
> >
> > > > [...]
> > > > > +static const VMStateDescription
> > > > > vmstate_virtio_balloon_free_page_report = {
> > > > > + .name = "virtio-balloon-device/free-page-report",
> > > > > + .version_id = 1,
> > > > > + .minimum_version_id = 1,
> > > > > + .needed = virtio_balloon_free_page_support,
> > > > > + .fields = (VMStateField[]) {
> > > > > + VMSTATE_UINT32(free_page_report_cmd_id, VirtIOBalloon),
> > > > > + VMSTATE_UINT32(poison_val, VirtIOBalloon),
> > > > (could we move all the poison-related lines into another patch or
> > > > postpone? after all we don't support it yet, do we?)
> > > >
> > > We don't support migrating poison value, but guest maybe use it, so we
> > > are
> > > actually disabling this feature in that case. Probably good to leave the
> > > code together to handle that case.
> > Could we just avoid declaring that feature bit in emulation code
> > completely? I mean, we support VIRTIO_BALLOON_F_FREE_PAGE_HINT first
> > as the first step (as you mentioned in commit message, the POISON is a
> > TODO). Then when you really want to completely support the POISON
> > bit, you can put all that into a separate patch. Would that work?
> >
>
> Not really. The F_PAGE_POISON isn't a feature configured via QEMU cmd line
> like F_FREE_PAGE_HINT. We always set F_PAGE_POISON if F_FREE_PAGE_HINT is
> enabled. It is used to detect if the guest is using page poison.
Ok I think I kind of understand. But it seems strange to me to have
this as a feature bit. I thought it suites more to be a config field
so that guest could setup. Like, we can have 1 byte to setup "whether
PAGE_POISON is used in the guest", another 1 byte to setup "what is
the PAGE_POISON value if it's enabled".
Asked since I see this in virtio spec (v1.0, though I guess it won't
change) in chapter "2.2.1 Driver Requirements: Feature Bits":
"The driver MUST NOT accept a feature which the device did not offer"
Then I'm curious what would happen if:
- a emulator (not QEMU) only offered F_FREE_PAGE_HINT, not F_POISON
- a guest that enabled PAGE_POISON
Then how the driver could tell the host that PAGE_POISON is enabled
considering that guest should never set that feature bit if the
emulation code didn't provide it?
--
Peter Xu
