Re: [PATCH V10 1/3] libxl: Add support for Virtio disk configuration

[Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>]

On 15.07.22 18:24, Anthony PERARD wrote:


Hello Anthony

> On Wed, Jul 13, 2022 at 08:03:17AM +0000, Oleksandr Tyshchenko wrote:
>> On 13.07.22 03:01, George Dunlap wrote:
>>
>> Hello George, Anthony
>>
>>>
>>>> On 30 Jun 2022, at 22:18, Oleksandr <olekstysh@xxxxxxxxx> wrote:
>>>>
>>>>
>>>> Dear all.
>>>>
>>>>
>>>> On 25.06.22 17:32, Oleksandr wrote:
>>>>> On 24.06.22 15:59, George Dunlap wrote:
>>>>>
>>>>> Hello George
>>>>>
>>>>>>> On 17 Jun 2022, at 17:14, Oleksandr Tyshchenko
>>>>>>> <olekstysh@xxxxxxxxx> wrote:
>>>>>>>
>>>>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>>>>>>>
>>>>>>> This patch adds basic support for configuring and assisting
>>>>>>> virtio-mmio
>>>>>>> based virtio-disk backend (emulator) which is intended to run out of
>>>>>>> Qemu and could be run in any domain.
>>>>>>> Although the Virtio block device is quite different from traditional
>>>>>>> Xen PV block device (vbd) from the toolstack's point of view:
>>>>>>> - as the frontend is virtio-blk which is not a Xenbus driver, nothing
>>>>>>> written to Xenstore are fetched by the frontend currently ("vdev"
>>>>>>> is not passed to the frontend). But this might need to be revised
>>>>>>> in future, so frontend data might be written to Xenstore in order to
>>>>>>> support hotplugging virtio devices or passing the backend domain id
>>>>>>> on arch where the device-tree is not available.
>>>>>>> - the ring-ref/event-channel are not used for the backend<->frontend
>>>>>>> communication, the proposed IPC for Virtio is IOREQ/DM
>>>>>>> it is still a "block device" and ought to be integrated in existing
>>>>>>> "disk" handling. So, re-use (and adapt) "disk" parsing/configuration
>>>>>>> logic to deal with Virtio devices as well.
>>>>>>>
>>>>>>> For the immediate purpose and an ability to extend that support for
>>>>>>> other use-cases in future (Qemu, virtio-pci, etc) perform the
>>>>>>> following
>>>>>>> actions:
>>>>>>> - Add new disk backend type (LIBXL_DISK_BACKEND_OTHER) and reflect
>>>>>>> that in the configuration
>>>>>>> - Introduce new disk "specification" and "transport" fields to struct
>>>>>>> libxl_device_disk. Both are written to the Xenstore. The transport
>>>>>>> field is only used for the specification "virtio" and it assumes
>>>>>>> only "mmio" value for now.
>>>>>>> - Introduce new "specification" option with "xen" communication
>>>>>>> protocol being default value.
>>>>>>> - Add new device kind (LIBXL__DEVICE_KIND_VIRTIO_DISK) as current
>>>>>>> one (LIBXL__DEVICE_KIND_VBD) doesn't fit into Virtio disk model
>>>>>>>
>>>>>>> An example of domain configuration for Virtio disk:
>>>>>>> disk = [ 'phy:/dev/mmcblk0p3, xvda1, backendtype=other,
>>>>>>> specification=virtio']
>>>>>>>
>>>>>>> Nothing has changed for default Xen disk configuration.
>>>>>>>
>>>>>>> Please note, this patch is not enough for virtio-disk to work
>>>>>>> on Xen (Arm), as for every Virtio device (including disk) we need
>>>>>>> to allocate Virtio MMIO params (IRQ and memory region) and pass
>>>>>>> them to the backend, also update Guest device-tree. The subsequent
>>>>>>> patch will add these missing bits. For the current patch,
>>>>>>> the default "irq" and "base" are just written to the Xenstore.
>>>>>>> This is not an ideal splitting, but this way we avoid breaking
>>>>>>> the bisectability.
>>>>>>>
>>>>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>>>>>> OK, I am *really* sorry for coming in here at the last minute and
>>>>>> quibbling about things.
>>>>>
>>>>> no problem
>>>>>
>>>>>
>>>>>> But this introduces a public interface which looks really wrong to
>>>>>> me.  I’ve replied to the mail from December where Juergen proposed
>>>>>> the “Other” protocol.
>>>>>>
>>>>>> Hopefully this will be a simple matter of finding a better name
>>>>>> than “other”. (Or you guys convincing me that “other” is really the
>>>>>> best name for this value; or even Anthony asserting his right as a
>>>>>> maintainer to overrule my objection if he thinks I’m out of line.)
>>>>>
>>>>> I saw your reply to V6 and Juergen's answer. I share Juergen's
>>>>> opinion as well as I understand your concern. I think, this is
>>>>> exactly the situation when finding a perfect name (obvious, short,
>>>>> etc) for the backendtype (in our particular case) is really difficult.
>>>>>
>>>>> Personally I tend to leave "other", because there is no better
>>>>> alternative from my PoV. Also I might be completely wrong here, but
>>>>> I don't think we will have to extend the "backendtype" for
>>>>> supporting other possible virtio backend implementations in the
>>>>> foreseeable future:
>>>>>
>>>>> - when Qemu gains the required support we will choose qdisk:
>>>>> backendtype qdisk specification virtio
>>>>> - for the possible virtio alternative of the blkback we will choose
>>>>> phy: backendtype phy specification virtio
>>>>>
>>>>> If there will be a need to keep various implementation, we will be
>>>>> able to describe that by using "transport" (mmio, pci, xenbus, argo,
>>>>> whatever).
>>>>> Actually this is why we also introduced "specification" and "transport".
>>>>>
>>>>> IIRC, there were other (suggested?) names except "other" which are
>>>>> "external" and "daemon". If you think that one of them is better
>>>>> than "other", I will happy to use it.
>>>>
>>>> Could we please make a decision on this?
>>>>
>>>> If "other" is not unambiguous, then maybe we could choose "daemon" to
>>>> describe arbitrary user-level backends, any thought?
>>> Unfortunately I didn’t have time to dig into this; I’m just going to
>>> have to withdraw my objection, and let you & Juergen decide what to
>>> call it.
>> George, thanks for letting me know. Juergen proposed to use "standalone"
>> for the new backendtype name which is far more specific. I agree with that.
>>
>>
>> Anthony, would you be happy with that renaming?
> I tried to figure out what backendtype is supposed to mean, how it's
> used. I feel it's quite messy at the moment.
>
> Man page xl-disk-configuration says it's a backend implementation to
> use. Beside 'phy', which I guess is the kernel or blkback, the two other
> point to QEMU ('qdisk') and tapdisk ('tap').
>
> The, backendtype is used throughout libxl to deal with the different
> backend implementation, and the value is stored in the xenstore key
> "type". From "blkif.h", "type" should be 'file' or 'phy' or 'tap', but
> we store 'qdisk' for 'qdisk'... so the "type" note in xenstore is
> probably useless for qdisk, but maybe useful for 'phy'? (This "type"
> node is only for the backend, so probably useless for a front end.)

I might mistake, but even the presence of "type" node (or other nodes) 
in Xenstore is useless for some backend implementations,

this provides us an ability to retrieve almost whole libxl_device_disk 
instance from the Xenstore (please see libxl__disk_from_xenstore()).

>
> Anyway, it seems to me that backendtype should be the name of the
> implementation of the backend we want to use. It is just a parameter
> to tell libxl how to communicate with the backend.

I also think the same


>   At the moment libxl
> uses xenstore to communicates with all backends even if that's not
> required, because libxl works this way and it's hard to change. (We
> could communicate with QEMU via QMP rather than xenstore for example.)
>
> So I guess either you have a name for your implementation, or something
> generic will do. So "standalone" is fine.

Thanks for the confirmation!


>
> (We probably want to document somewhere that this new type would
> simply mean "only-relying-on-xenstore-data" like Juergen is putting
> it, and isn't blkback or QEMU.)

I will add a comment in libxl_types.idl where "STANDALONE" enum item is 
introduced.



>
> Thanks,
>
-- 
Regards,

Oleksandr Tyshchenko