Re: [PATCH V7 1/2] libxl: Add support for Virtio disk configuration

[Oleksandr <olekstysh@gmail.com>]

On 25.04.22 15:09, Juergen Gross wrote:

Hello Juergen

> On 25.04.22 14:02, Oleksandr wrote:
>>
>> On 25.04.22 10:43, Juergen Gross wrote:
>>
>>
>> Hello Juergen
>>
>>
>> Thank you for the feedback.
>>
>>> On 08.04.22 20:21, Oleksandr Tyshchenko wrote:
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>
>>>> This patch adds basic support for configuring and assisting 
>>>> virtio-mmio
>>>> based virtio-disk backend (emualator) 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 (the vdev is not
>>>>     passed to the frontend)
>>>
>>> I thought about the future support on x86.
>>>
>>> There we don't have a device tree (and I don't want to introduce it),
>>> so the only ways to specify the backend domain id would be to:
>>>
>>> - add some information to ACPI tables
>>> - use boot parameters
>>> - use Xenstore
>>
>> I understand that, and agree
>>
>>
>>> Thinking further of hotplugging virtio devices, Xenstore seems to be 
>>> the
>>> only real suitable alternative. Using virtio mechanisms doesn't seem
>>> appropriate, as such information should be retrieved in "platform
>>> specific" ways (see e.g. specifying an "endpoint" in the virtio IOMMU
>>> device [1], [2]). I think the Xenstore information for that purpose
>>> could be rather minimal and it should be device-type agnostic. Having
>>> just a directory with endpoints and associated backend domids would
>>> probably be enough (not needed in this series, of course).
>>
>> Just to make it clear, we are speaking about the possible ways to 
>> communicate backend domid for another series [1], so about the x86's 
>> alternative of device-tree bindings "xen,dev-domid" [2].
>> I was thinking we could avoid using Xenstore at the guest side for 
>> that purpose, but I didn't think about hotplug...
>> I assume, all Xenstore bits wouldn't go the outside Xen grant 
>> DMA-mapping layer (grant-dma-ops.c)?
>
> I think it would be another driver under drivers/xen/ without the need to
> touch any other frontend related file or Xen-related architecture 
> specific
> code.


ok


>
>
>>> And with the hotplug option in mind I start to feel unueasy with naming
>>> the new Xenstore node "protocol", as the frontend disk nodes for 
>>> "normal"
>>> disks already have a "protocol" entry specifying 64- or 32-bit 
>>> protocol.
>>
>>
>> I noticed the "protocol" node at the frontend side for traditional 
>> Xen PV block device which handles yet another purpose, but I didn't 
>> think much about it
>> since the new "protocol" node in only for the backend's use. If we 
>> start thinking of frontend's Xenstore nodes, then yes, will clash...
>>
>>>
>>>
>>> Maybe we should really name it "transport" instead?
>>
>> ... For me the "transport" name is associated with virtio transports: 
>> mmio, pci, ccw. But, I would be ok with that name. Another possible 
>> name could be "specification".
>
> Yeah, looking at the virtio spec this makes sense.
>
> So I would be fine with "specification".


ok, thank you for the confirmation.


>
>>>> - 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, 
>>>> protocol=virtio-mmio']
>>>
>>> With Roger's feedback this would then be "transport=virtio", the "mmio"
>>> part should then be something like "adapter=mmio" (in contrast to
>>> "adapter=pci"), and "adapter" only needed in case of a device tree and
>>> PCI being available.
>>
>> ok, will rename. Can we add "adapter" (or whenever the name would be) 
>> option later, when there is a real need? For now, I mean within the 
>> current series which adds only virtio-mmio bits on Arm, we can assume 
>> that "transport=virtio" implies using virtio-mmio.
>
> Yes, we should add it only when needed.


ok, good


>
>>
>> BTW, if we named the main option "specification", the secondary 
>> option "transport" would good fit from my PoV.
>> For example:
>> disk = [ 'phy:/dev/mmcblk0p3, xvda1, backendtype=other, 
>> specification=virtio, transport=mmio']
>
> Fine with me.


perfect


>
>
> Juergen

-- 
Regards,

Oleksandr Tyshchenko