Re: [Qemu-devel] [PATCH v7 4/5] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT

[Wei Wang <wei.w.wang@intel.com>]

On 06/07/2018 02:58 PM, Peter Xu wrote:
> On Thu, Jun 07, 2018 at 01:29:22PM +0800, Wei Wang wrote:
>>>>> [...]
>>>>>> +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".

This is also suggested by Michael, which sounds good to me. Using config 
is doable, but that doesn't show advantages over using feature bits.



>
> 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?
>

All the emulator implementations need to follow the virtio spec. We will 
finally have this feature written to the virtio-balloon device section, 
and state that the F_PAGE_POISON needs to be set on the device when 
F_FREE_PAGE_HINT is set on the device.


Best,
Wei