Re: [PATCH V8 2/2] libxl: Introduce basic virtio-mmio support on Arm
On 20.05.22 18:22, Anthony PERARD wrote:
Hello Anthony
On Thu, May 19, 2022 at 08:16:16PM +0300, Oleksandr wrote:
On 18.05.22 14:05, Anthony PERARD wrote:
On Tue, May 03, 2022 at 08:26:03PM +0300, Oleksandr Tyshchenko wrote:
+for (i = 0; i < d_config->num_disks; i++) {
+libxl_device_disk *disk = &d_config->disks[i];
+
+if (disk->specification == LIBXL_DISK_SPECIFICATION_VIRTIO) {
+disk->base = alloc_virtio_mmio_base(gc, &virtio_mmio_base);
+if (!disk->base)
+return ERROR_FAIL;
+
+disk->irq = alloc_virtio_mmio_irq(gc, &virtio_mmio_irq);
+if (!disk->irq)
+return ERROR_FAIL;
+
+if (virtio_irq < disk->irq)
+virtio_irq = disk->irq;
+virtio_enabled = true;
+
+LOG(DEBUG, "Allocate Virtio MMIO params for Vdev %s: IRQ %u BASE
0x%"PRIx64,
+disk->vdev, disk->irq, disk->base);
+}
+}
+
+if (virtio_enabled)
+nr_spis += (virtio_irq - 32) + 1;
Is it possible to update "nr_spis" inside the loop?
yes, but ...
The added value
seems to be "number of virtio device + 1",
... not really ...
so updating "nr_spis" and
adding +1 after the loop could work, right?
... from my understanding, we cannot just increment nr_spis by "one"
inside a loop, we need to calculate it.
Something like that (not tested):
uint32_t spi;
...
spi = irq - 32;
if (nr_spis <= spi)
nr_spis = spi + 1;
Shall I update "nr_spis" inside the loop?
Are you asking because of eliminating "virtio_enabled" and/or "virtio_irq"
locals? They are used down the code.
I'm asking because the calculation doesn't really make sense to me yet. At the
moment "virtio_irq-32+1" happen to be the "number of disk + 1" and we
have "nr_spis += " which I don't think makes sense with the "+1".
I see
Doesn't "nr_spis" only need to be the highest irq value for the devices
we're adding? (Maybe with +1) (also -32 because I think I understand
what 32 stand for now) (also, the "num_irqs" loop just after this loop
seems to do exactly that)
I also think the same, the "nr_spis" needs to cover the highest SPI.
But I think what this line of code needs the most is a comment.
ok
Also, what is "32"? Is it "GUEST_VIRTIO_MMIO_SPI_FIRST - 1" ?
Although currently "GUEST_VIRTIO_MMIO_SPI_FIRST - 1" = "32", we cannot rely
on this (I mean to use "GUEST_VIRTIO_MMIO_SPI_FIRST - 1"
instead of "32"), because "32" has yet another meaning. This is an offset
for SPI, the values before 32 are for private IRQs (PPI, SGI).
Do you think it could be possible to name that value? I've seen other
use of 32 in the same function that have probably the same meaning.
yes, all uses of "32" within current function have the same meaning.
But
if you don't have a good name, I guess we can also live a bit longer
with a plain "32".
Except here in toolstack, this plain "32" is used in a few places in
hypervisor code also.
I don't have a plan to convert this value into appropriate #define
everywhere.
But, I can introduce local to this file #define NR_LOCAL_IRQS 32 and
change in current function.
Shall I?
Cheers,
--
Regards,
Oleksandr Tyshchenko
Re: [PATCH V8 2/2] libxl: Introduce basic virtio-mmio support on Arm
On Thu, May 19, 2022 at 08:16:16PM +0300, Oleksandr wrote:
> On 18.05.22 14:05, Anthony PERARD wrote:
> > On Tue, May 03, 2022 at 08:26:03PM +0300, Oleksandr Tyshchenko wrote:
> > > +for (i = 0; i < d_config->num_disks; i++) {
> > > +libxl_device_disk *disk = &d_config->disks[i];
> > > +
> > > +if (disk->specification == LIBXL_DISK_SPECIFICATION_VIRTIO) {
> > > +disk->base = alloc_virtio_mmio_base(gc, &virtio_mmio_base);
> > > +if (!disk->base)
> > > +return ERROR_FAIL;
> > > +
> > > +disk->irq = alloc_virtio_mmio_irq(gc, &virtio_mmio_irq);
> > > +if (!disk->irq)
> > > +return ERROR_FAIL;
> > > +
> > > +if (virtio_irq < disk->irq)
> > > +virtio_irq = disk->irq;
> > > +virtio_enabled = true;
> > > +
> > > +LOG(DEBUG, "Allocate Virtio MMIO params for Vdev %s: IRQ %u
> > > BASE 0x%"PRIx64,
> > > +disk->vdev, disk->irq, disk->base);
> > > +}
> > > +}
> > > +
> > > +if (virtio_enabled)
> > > +nr_spis += (virtio_irq - 32) + 1;
> > Is it possible to update "nr_spis" inside the loop?
>
> yes, but ...
>
>
> > The added value
> > seems to be "number of virtio device + 1",
>
> ... not really ...
>
>
> > so updating "nr_spis" and
> > adding +1 after the loop could work, right?
>
> ... from my understanding, we cannot just increment nr_spis by "one"
> inside a loop, we need to calculate it.
>
>
> Something like that (not tested):
>
> uint32_t spi;
>
> ...
>
> spi = irq - 32;
> if (nr_spis <= spi)
> nr_spis = spi + 1;
>
>
> Shall I update "nr_spis" inside the loop?
>
> Are you asking because of eliminating "virtio_enabled" and/or "virtio_irq"
> locals? They are used down the code.
I'm asking because the calculation doesn't really make sense to me yet. At the
moment "virtio_irq-32+1" happen to be the "number of disk + 1" and we
have "nr_spis += " which I don't think makes sense with the "+1".
Doesn't "nr_spis" only need to be the highest irq value for the devices
we're adding? (Maybe with +1) (also -32 because I think I understand
what 32 stand for now) (also, the "num_irqs" loop just after this loop
seems to do exactly that)
But I think what this line of code needs the most is a comment.
> > Also, what is "32"? Is it "GUEST_VIRTIO_MMIO_SPI_FIRST - 1" ?
>
> Although currently "GUEST_VIRTIO_MMIO_SPI_FIRST - 1" = "32", we cannot rely
> on this (I mean to use "GUEST_VIRTIO_MMIO_SPI_FIRST - 1"
>
> instead of "32"), because "32" has yet another meaning. This is an offset
> for SPI, the values before 32 are for private IRQs (PPI, SGI).
Do you think it could be possible to name that value? I've seen other
use of 32 in the same function that have probably the same meaning. But
if you don't have a good name, I guess we can also live a bit longer
with a plain "32".
Cheers,
--
Anthony PERARD
Re: [PATCH V8 2/2] libxl: Introduce basic virtio-mmio support on Arm
On 18.05.22 14:05, Anthony PERARD wrote:
Hello Anthony
On Tue, May 03, 2022 at 08:26:03PM +0300, Oleksandr Tyshchenko wrote:
From: Julien Grall
This patch introduces helpers to allocate Virtio MMIO params
(IRQ and memory region) and create specific device node in
the Guest device-tree with allocated params. In order to deal
with multiple Virtio devices, reserve corresponding ranges.
For now, we reserve 1MB for memory regions and 10 SPIs.
As these helpers should be used for every Virtio device attached
to the Guest, call them for Virtio disk(s).
Please note, with statically allocated Virtio IRQs there is
a risk of a clash with a physical IRQs of passthrough devices.
For the first version, it's fine, but we should consider allocating
the Virtio IRQs automatically. Thankfully, we know in advance which
IRQs will be used for passthrough to be able to choose non-clashed
ones.
Signed-off-by: Julien Grall
Signed-off-by: Oleksandr Tyshchenko
---
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index eef1de0..37403a2 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -8,6 +8,46 @@
#include
#include
+/*
+ * There is no clear requirements for the total size of Virtio MMIO region.
+ * The size of control registers is 0x100 and device-specific configuration
+ * registers starts at the offset 0x100, however it's size depends on the
device
+ * and the driver. Pick the biggest known size at the moment to cover most
+ * of the devices (also consider allowing the user to configure the size via
+ * config file for the one not conforming with the proposed value).
+ */
+#define VIRTIO_MMIO_DEV_SIZE xen_mk_ullong(0x200)
+
+static uint64_t alloc_virtio_mmio_base(libxl__gc *gc, uint64_t
*virtio_mmio_base)
+{
+uint64_t base = *virtio_mmio_base;
+
+/* Make sure we have enough reserved resources */
+if ((base + VIRTIO_MMIO_DEV_SIZE >
+GUEST_VIRTIO_MMIO_BASE + GUEST_VIRTIO_MMIO_SIZE)) {
Could you remove the second set of parentheses? I'd like the compiler to
warn us if there's an assignment.
ok
@@ -26,8 +66,10 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
{
uint32_t nr_spis = 0;
unsigned int i;
-uint32_t vuart_irq;
-bool vuart_enabled = false;
+uint32_t vuart_irq, virtio_irq = 0;
+bool vuart_enabled = false, virtio_enabled = false;
+uint64_t virtio_mmio_base = GUEST_VIRTIO_MMIO_BASE;
+uint32_t virtio_mmio_irq = GUEST_VIRTIO_MMIO_SPI_FIRST;
/*
* If pl011 vuart is enabled then increment the nr_spis to allow
allocation
@@ -39,6 +81,30 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
vuart_enabled = true;
}
+for (i = 0; i < d_config->num_disks; i++) {
+libxl_device_disk *disk = &d_config->disks[i];
+
+if (disk->specification == LIBXL_DISK_SPECIFICATION_VIRTIO) {
+disk->base = alloc_virtio_mmio_base(gc, &virtio_mmio_base);
+if (!disk->base)
+return ERROR_FAIL;
+
+disk->irq = alloc_virtio_mmio_irq(gc, &virtio_mmio_irq);
+if (!disk->irq)
+return ERROR_FAIL;
+
+if (virtio_irq < disk->irq)
+virtio_irq = disk->irq;
+virtio_enabled = true;
+
+LOG(DEBUG, "Allocate Virtio MMIO params for Vdev %s: IRQ %u BASE
0x%"PRIx64,
+disk->vdev, disk->irq, disk->base);
+}
+}
+
+if (virtio_enabled)
+nr_spis += (virtio_irq - 32) + 1;
Is it possible to update "nr_spis" inside the loop?
yes, but ...
The added value
seems to be "number of virtio device + 1",
... not really ...
so updating "nr_spis" and
adding +1 after the loop could work, right?
... from my understanding, we cannot just increment nr_spis by "one"
inside a loop, we need to calculate it.
Something like that (not tested):
uint32_t spi;
...
spi = irq - 32;
if (nr_spis <= spi)
nr_spis = spi + 1;
Shall I update "nr_spis" inside the loop?
Are you asking because of eliminating "virtio_enabled" and/or
"virtio_irq" locals? They are used down the code.
Also, what is "32"? Is it "GUEST_VIRTIO_MMIO_SPI_FIRST - 1" ?
Although currently "GUEST_VIRTIO_MMIO_SPI_FIRST - 1" = "32", we cannot
rely on this (I mean to use "GUEST_VIRTIO_MMIO_SPI_FIRST - 1"
instead of "32"), because "32" has yet another meaning. This is an
offset for SPI, the values before 32 are for private IRQs (PPI, SGI).
+
for (i = 0; i < d_config->b_info.num_irqs; i++) {
uint32_t irq = d_config->b_info.irqs[i];
uint32_t spi;
@@ -787,6 +860,39 @@ static int make_vpci_node(libxl__gc *gc, void *fdt,
return 0;
}
+
+static int make_virtio_mmio_node(libxl__gc *gc, void *fdt,
+ uint64_t base, uint32_t irq)
+{
+int res;
+gic_interrupt intr;
+/* Placeholder for virtio@ + a 64-bit number + \0 */
+
Re: [PATCH V8 2/2] libxl: Introduce basic virtio-mmio support on Arm
On Tue, May 03, 2022 at 08:26:03PM +0300, Oleksandr Tyshchenko wrote:
> From: Julien Grall
>
> This patch introduces helpers to allocate Virtio MMIO params
> (IRQ and memory region) and create specific device node in
> the Guest device-tree with allocated params. In order to deal
> with multiple Virtio devices, reserve corresponding ranges.
> For now, we reserve 1MB for memory regions and 10 SPIs.
>
> As these helpers should be used for every Virtio device attached
> to the Guest, call them for Virtio disk(s).
>
> Please note, with statically allocated Virtio IRQs there is
> a risk of a clash with a physical IRQs of passthrough devices.
> For the first version, it's fine, but we should consider allocating
> the Virtio IRQs automatically. Thankfully, we know in advance which
> IRQs will be used for passthrough to be able to choose non-clashed
> ones.
>
> Signed-off-by: Julien Grall
> Signed-off-by: Oleksandr Tyshchenko
> ---
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index eef1de0..37403a2 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -8,6 +8,46 @@
> #include
> #include
>
> +/*
> + * There is no clear requirements for the total size of Virtio MMIO region.
> + * The size of control registers is 0x100 and device-specific configuration
> + * registers starts at the offset 0x100, however it's size depends on the
> device
> + * and the driver. Pick the biggest known size at the moment to cover most
> + * of the devices (also consider allowing the user to configure the size via
> + * config file for the one not conforming with the proposed value).
> + */
> +#define VIRTIO_MMIO_DEV_SIZE xen_mk_ullong(0x200)
> +
> +static uint64_t alloc_virtio_mmio_base(libxl__gc *gc, uint64_t
> *virtio_mmio_base)
> +{
> +uint64_t base = *virtio_mmio_base;
> +
> +/* Make sure we have enough reserved resources */
> +if ((base + VIRTIO_MMIO_DEV_SIZE >
> +GUEST_VIRTIO_MMIO_BASE + GUEST_VIRTIO_MMIO_SIZE)) {
Could you remove the second set of parentheses? I'd like the compiler to
warn us if there's an assignment.
> @@ -26,8 +66,10 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> {
> uint32_t nr_spis = 0;
> unsigned int i;
> -uint32_t vuart_irq;
> -bool vuart_enabled = false;
> +uint32_t vuart_irq, virtio_irq = 0;
> +bool vuart_enabled = false, virtio_enabled = false;
> +uint64_t virtio_mmio_base = GUEST_VIRTIO_MMIO_BASE;
> +uint32_t virtio_mmio_irq = GUEST_VIRTIO_MMIO_SPI_FIRST;
>
> /*
> * If pl011 vuart is enabled then increment the nr_spis to allow
> allocation
> @@ -39,6 +81,30 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> vuart_enabled = true;
> }
>
> +for (i = 0; i < d_config->num_disks; i++) {
> +libxl_device_disk *disk = &d_config->disks[i];
> +
> +if (disk->specification == LIBXL_DISK_SPECIFICATION_VIRTIO) {
> +disk->base = alloc_virtio_mmio_base(gc, &virtio_mmio_base);
> +if (!disk->base)
> +return ERROR_FAIL;
> +
> +disk->irq = alloc_virtio_mmio_irq(gc, &virtio_mmio_irq);
> +if (!disk->irq)
> +return ERROR_FAIL;
> +
> +if (virtio_irq < disk->irq)
> +virtio_irq = disk->irq;
> +virtio_enabled = true;
> +
> +LOG(DEBUG, "Allocate Virtio MMIO params for Vdev %s: IRQ %u BASE
> 0x%"PRIx64,
> +disk->vdev, disk->irq, disk->base);
> +}
> +}
> +
> +if (virtio_enabled)
> +nr_spis += (virtio_irq - 32) + 1;
Is it possible to update "nr_spis" inside the loop? The added value
seems to be "number of virtio device + 1", so updating "nr_spis" and
adding +1 after the loop could work, right?
Also, what is "32"? Is it "GUEST_VIRTIO_MMIO_SPI_FIRST - 1" ?
> +
> for (i = 0; i < d_config->b_info.num_irqs; i++) {
> uint32_t irq = d_config->b_info.irqs[i];
> uint32_t spi;
> @@ -787,6 +860,39 @@ static int make_vpci_node(libxl__gc *gc, void *fdt,
> return 0;
> }
>
> +
> +static int make_virtio_mmio_node(libxl__gc *gc, void *fdt,
> + uint64_t base, uint32_t irq)
> +{
> +int res;
> +gic_interrupt intr;
> +/* Placeholder for virtio@ + a 64-bit number + \0 */
> +char buf[24];
> +
> +snprintf(buf, sizeof(buf), "virtio@%"PRIx64, base);
Could you use GCSPRINTF() here instead of using a buffer of a static
size calculated by hand which is potentially wrong? Also, the return
value of snprintf isn't checked so the string could be truncated without
warning. So I think GCSPRINTF is better than a static buffer.
The rest of the patch looks fine.
Thanks,
--
Anthony PERARD
Re: [PATCH V8 2/2] libxl: Introduce basic virtio-mmio support on Arm
On Tue, 3 May 2022, Oleksandr Tyshchenko wrote:
> From: Julien Grall
>
> This patch introduces helpers to allocate Virtio MMIO params
> (IRQ and memory region) and create specific device node in
> the Guest device-tree with allocated params. In order to deal
> with multiple Virtio devices, reserve corresponding ranges.
> For now, we reserve 1MB for memory regions and 10 SPIs.
>
> As these helpers should be used for every Virtio device attached
> to the Guest, call them for Virtio disk(s).
>
> Please note, with statically allocated Virtio IRQs there is
> a risk of a clash with a physical IRQs of passthrough devices.
> For the first version, it's fine, but we should consider allocating
> the Virtio IRQs automatically. Thankfully, we know in advance which
> IRQs will be used for passthrough to be able to choose non-clashed
> ones.
>
> Signed-off-by: Julien Grall
> Signed-off-by: Oleksandr Tyshchenko
Reviewed-by: Stefano Stabellini
> ---
> Please note, this is a split/cleanup/hardening of Julien's PoC:
> "Add support for Guest IO forwarding to a device emulator"
>
> Changes RFC -> V1:
>- was squashed with:
> "[RFC PATCH V1 09/12] libxl: Handle virtio-mmio irq in more correct way"
> "[RFC PATCH V1 11/12] libxl: Insert "dma-coherent" property into
> virtio-mmio device node"
> "[RFC PATCH V1 12/12] libxl: Fix duplicate memory node in DT"
>- move VirtIO MMIO #define-s to xen/include/public/arch-arm.h
>
> Changes V1 -> V2:
>- update the author of a patch
>
> Changes V2 -> V3:
>- no changes
>
> Changes V3 -> V4:
>- no changes
>
> Changes V4 -> V5:
>- split the changes, change the order of the patches
>- drop an extra "virtio" configuration option
>- update patch description
>- use CONTAINER_OF instead of own implementation
>- reserve ranges for Virtio MMIO params and put them
> in correct location
>- create helpers to allocate Virtio MMIO params, add
> corresponding sanity-сhecks
>- add comment why MMIO size 0x200 is chosen
>- update debug print
>- drop Wei's T-b
>
> Changes V5 -> V6:
>- rebase on current staging
>
> Changes V6 -> V7:
>- rebase on current staging
>- add T-b and R-b tags
>- update according to the recent changes to
> "libxl: Add support for Virtio disk configuration"
>
> Changes V7 -> V8:
>- drop T-b and R-b tags
>- make virtio_mmio_base/irq global variables to be local in
> libxl__arch_domain_prepare_config() and initialize them at
> the beginning of the function, then rework alloc_virtio_mmio_base/irq()
> to take a pointer to virtio_mmio_base/irq variables as an argument
>- update according to the recent changes to
> "libxl: Add support for Virtio disk configuration"
> ---
> tools/libs/light/libxl_arm.c | 118
> +-
> xen/include/public/arch-arm.h | 7 +++
> 2 files changed, 123 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index eef1de0..37403a2 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -8,6 +8,46 @@
> #include
> #include
>
> +/*
> + * There is no clear requirements for the total size of Virtio MMIO region.
> + * The size of control registers is 0x100 and device-specific configuration
> + * registers starts at the offset 0x100, however it's size depends on the
> device
> + * and the driver. Pick the biggest known size at the moment to cover most
> + * of the devices (also consider allowing the user to configure the size via
> + * config file for the one not conforming with the proposed value).
> + */
> +#define VIRTIO_MMIO_DEV_SIZE xen_mk_ullong(0x200)
> +
> +static uint64_t alloc_virtio_mmio_base(libxl__gc *gc, uint64_t
> *virtio_mmio_base)
> +{
> +uint64_t base = *virtio_mmio_base;
> +
> +/* Make sure we have enough reserved resources */
> +if ((base + VIRTIO_MMIO_DEV_SIZE >
> +GUEST_VIRTIO_MMIO_BASE + GUEST_VIRTIO_MMIO_SIZE)) {
> +LOG(ERROR, "Ran out of reserved range for Virtio MMIO BASE
> 0x%"PRIx64"\n",
> +base);
> +return 0;
> +}
> +*virtio_mmio_base += VIRTIO_MMIO_DEV_SIZE;
> +
> +return base;
> +}
> +
> +static uint32_t alloc_virtio_mmio_irq(libxl__gc *gc, uint32_t
> *virtio_mmio_irq)
> +{
> +uint32_t irq = *virtio_mmio_irq;
> +
> +/* Make sure we have enough reserved resources */
> +if (irq > GUEST_VIRTIO_MMIO_SPI_LAST) {
> +LOG(ERROR, "Ran out of reserved range for Virtio MMIO IRQ %u\n",
> irq);
> +return 0;
> +}
> +(*virtio_mmio_irq)++;
> +
> +return irq;
> +}
> +
> static const char *gicv_to_string(libxl_gic_version gic_version)
> {
> switch (gic_version) {
> @@ -26,8 +66,10 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> {
> uint32_t nr_spis = 0;
> unsigned int i;
> -uint32_t vuart_irq;
> -bool vuart_enabled = false;
> +uint32_t vuar
[PATCH V8 2/2] libxl: Introduce basic virtio-mmio support on Arm
From: Julien Grall
This patch introduces helpers to allocate Virtio MMIO params
(IRQ and memory region) and create specific device node in
the Guest device-tree with allocated params. In order to deal
with multiple Virtio devices, reserve corresponding ranges.
For now, we reserve 1MB for memory regions and 10 SPIs.
As these helpers should be used for every Virtio device attached
to the Guest, call them for Virtio disk(s).
Please note, with statically allocated Virtio IRQs there is
a risk of a clash with a physical IRQs of passthrough devices.
For the first version, it's fine, but we should consider allocating
the Virtio IRQs automatically. Thankfully, we know in advance which
IRQs will be used for passthrough to be able to choose non-clashed
ones.
Signed-off-by: Julien Grall
Signed-off-by: Oleksandr Tyshchenko
---
Please note, this is a split/cleanup/hardening of Julien's PoC:
"Add support for Guest IO forwarding to a device emulator"
Changes RFC -> V1:
- was squashed with:
"[RFC PATCH V1 09/12] libxl: Handle virtio-mmio irq in more correct way"
"[RFC PATCH V1 11/12] libxl: Insert "dma-coherent" property into
virtio-mmio device node"
"[RFC PATCH V1 12/12] libxl: Fix duplicate memory node in DT"
- move VirtIO MMIO #define-s to xen/include/public/arch-arm.h
Changes V1 -> V2:
- update the author of a patch
Changes V2 -> V3:
- no changes
Changes V3 -> V4:
- no changes
Changes V4 -> V5:
- split the changes, change the order of the patches
- drop an extra "virtio" configuration option
- update patch description
- use CONTAINER_OF instead of own implementation
- reserve ranges for Virtio MMIO params and put them
in correct location
- create helpers to allocate Virtio MMIO params, add
corresponding sanity-сhecks
- add comment why MMIO size 0x200 is chosen
- update debug print
- drop Wei's T-b
Changes V5 -> V6:
- rebase on current staging
Changes V6 -> V7:
- rebase on current staging
- add T-b and R-b tags
- update according to the recent changes to
"libxl: Add support for Virtio disk configuration"
Changes V7 -> V8:
- drop T-b and R-b tags
- make virtio_mmio_base/irq global variables to be local in
libxl__arch_domain_prepare_config() and initialize them at
the beginning of the function, then rework alloc_virtio_mmio_base/irq()
to take a pointer to virtio_mmio_base/irq variables as an argument
- update according to the recent changes to
"libxl: Add support for Virtio disk configuration"
---
tools/libs/light/libxl_arm.c | 118 +-
xen/include/public/arch-arm.h | 7 +++
2 files changed, 123 insertions(+), 2 deletions(-)
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index eef1de0..37403a2 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -8,6 +8,46 @@
#include
#include
+/*
+ * There is no clear requirements for the total size of Virtio MMIO region.
+ * The size of control registers is 0x100 and device-specific configuration
+ * registers starts at the offset 0x100, however it's size depends on the
device
+ * and the driver. Pick the biggest known size at the moment to cover most
+ * of the devices (also consider allowing the user to configure the size via
+ * config file for the one not conforming with the proposed value).
+ */
+#define VIRTIO_MMIO_DEV_SIZE xen_mk_ullong(0x200)
+
+static uint64_t alloc_virtio_mmio_base(libxl__gc *gc, uint64_t
*virtio_mmio_base)
+{
+uint64_t base = *virtio_mmio_base;
+
+/* Make sure we have enough reserved resources */
+if ((base + VIRTIO_MMIO_DEV_SIZE >
+GUEST_VIRTIO_MMIO_BASE + GUEST_VIRTIO_MMIO_SIZE)) {
+LOG(ERROR, "Ran out of reserved range for Virtio MMIO BASE
0x%"PRIx64"\n",
+base);
+return 0;
+}
+*virtio_mmio_base += VIRTIO_MMIO_DEV_SIZE;
+
+return base;
+}
+
+static uint32_t alloc_virtio_mmio_irq(libxl__gc *gc, uint32_t *virtio_mmio_irq)
+{
+uint32_t irq = *virtio_mmio_irq;
+
+/* Make sure we have enough reserved resources */
+if (irq > GUEST_VIRTIO_MMIO_SPI_LAST) {
+LOG(ERROR, "Ran out of reserved range for Virtio MMIO IRQ %u\n", irq);
+return 0;
+}
+(*virtio_mmio_irq)++;
+
+return irq;
+}
+
static const char *gicv_to_string(libxl_gic_version gic_version)
{
switch (gic_version) {
@@ -26,8 +66,10 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
{
uint32_t nr_spis = 0;
unsigned int i;
-uint32_t vuart_irq;
-bool vuart_enabled = false;
+uint32_t vuart_irq, virtio_irq = 0;
+bool vuart_enabled = false, virtio_enabled = false;
+uint64_t virtio_mmio_base = GUEST_VIRTIO_MMIO_BASE;
+uint32_t virtio_mmio_irq = GUEST_VIRTIO_MMIO_SPI_FIRST;
/*
* If pl011 vuart is enabled then increment the nr_spis to allow allocation
@@ -39,6 +81,30 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
