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

[Oleksandr <olekstysh@gmail.com>]

On 14.12.21 18:03, Anthony PERARD wrote:

Hi Anthony


> On Wed, Dec 08, 2021 at 06:59:43PM +0200, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> This patch adds basic support for configuring and assisting 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 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)
>>   - 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.
> How backend are intended to be created? Is there something listening on
> xenstore?
>
> You mention QEMU as been the backend, do you intend to have QEMU
> listening on xenstore to create a virtio backend? Or maybe it is on the
> command line? There is QMP as well, but it's probably a lot more
> complicated as I think libxl needs refactoring for that.


No, QEMU is not involved there. The backend is a standalone application,
it is launched from the command line. The backend reads the Xenstore to get
the configuration and to detect when guest with the frontend is 
created/destroyed.


>
>> Besides introducing new disk backend type (LIBXL_DISK_BACKEND_VIRTIO)
>> introduce new device kind (LIBXL__DEVICE_KIND_VIRTIO_DISK) as current
>> one (LIBXL__DEVICE_KIND_VBD) doesn't fit into Virtio disk model.
>>
>> In order to inform the toolstack that Virtio disk needs to be used
>> extend "disk" configuration by introducing new "virtio" flag.
>> An example of domain configuration:
>> disk = [ 'backend=DomD, phy:/dev/mmcblk1p3, xvda1, rw, virtio' ]
> This new "virtio" flags feels strange. Would having something like
> "backendtype=virtio" works?


IIRC I considered "backendtype=virtio" option, but decided to go "an 
extra virtio flag" route, however I don't remember what exactly affected 
my decision.
But, I see your point and agree, I will analyze whether we can use 
"backendtype=virtio", I hope that we can, but need to make sure.


>
>> 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 feels like the patches are in the wrong order. I don't think it is
> a good idea to allow to create broken configuration until a follow-up
> patch fixes things.

Yes, I also think this is not an ideal splitting, so I decided to write 
a few sentences to draw reviewer's attention to this.
The problem is that second patch adds Arm bits which are local to 
libs/light/libxl_arm.c and if I put it before the current one I will 
break the bisectability
as there will be no callers yet.


>
>> diff --git a/tools/xl/xl_block.c b/tools/xl/xl_block.c
>> index 70eed43..50a4d45 100644
>> --- a/tools/xl/xl_block.c
>> +++ b/tools/xl/xl_block.c
>> @@ -50,6 +50,11 @@ int main_blockattach(int argc, char **argv)
>>           return 0;
>>       }
>>   
>> +    if (disk.virtio) {
>> +        fprintf(stderr, "block-attach is not supported for Virtio device\n");
>> +        return 1;
>> +    }
> This might not be the right place. libxl might want to prevent hotplug
> instead.

Could you please point me the right place where to prevent hotplug?


Thank you.


>
> Thanks,
>
-- 
Regards,

Oleksandr Tyshchenko