[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH 2/6] xen/public: arch-arm: reserve resources for virtio-pci



Hi Oleksandr,

On 15/11/2023 16:51, Oleksandr Tyshchenko wrote:


On 15.11.23 14:33, Julien Grall wrote:
Hi,


Hello Julien

Let me please try to explain some bits.



Thanks for adding support for virtio-pci in Xen. I have some questions.

On 15/11/2023 11:26, Sergiy Kibrik wrote:
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>

In order to enable more use-cases such as having multiple
device-models (Qemu) running in different backend domains which provide
virtio-pci devices for the same guest, we allocate and expose one
PCI host bridge for every virtio backend domain for that guest.

OOI, why do you need to expose one PCI host bridge for every stubdomain?

In fact looking at the next patch, it seems you are handling some of the
hostbridge request in Xen. This is adds a bit more confusion.

I was expecting the virtual PCI device would be in the vPCI and each
Device emulator would advertise which BDF they are covering.


This patch series only covers use-cases where the device emulator
handles the *entire* PCI Host bridge and PCI (virtio-pci) devices behind
it (i.e. Qemu). Also this patch series doesn't touch vPCI/PCI
pass-through resources, handling, accounting, nothing.

I understood you want to one Device Emulator to handle the entire PCI host bridge. But...

From the
hypervisor we only need a help to intercept the config space accesses
happen in a range [GUEST_VIRTIO_PCI_ECAM_BASE ...
GUEST_VIRTIO_PCI_ECAM_BASE + GUEST_VIRTIO_PCI_TOTAL_ECAM_SIZE] and
forward them to the linked device emulator (if any), that's all.

... I really don't see why you need to add code in Xen to trap the region. If QEMU is dealing with the hostbridge, then it should be able to register the MMIO region and then do the translation.


It is not possible (with current series) to run device emulators what
emulate only separate PCI (virtio-pci) devices. For it to be possible, I
think, much more changes are required than current patch series does.
There at least should be special PCI Host bridge emulation in Xen (or
reuse vPCI) for the integration. Also Xen should be in charge of forming
resulting PCI interrupt based on each PCI device level signaling (if we
use legacy interrupts), some kind of x86's XEN_DMOP_set_pci_intx_level,
etc. Please note, I am not saying this is not possible in general,
likely it is possible, but initial patch series doesn't cover these
use-cases)

We expose one PCI host bridge per virtio backend domain. This is a
separate PCI host bridge to combine all virtio-pci devices running in
the same backend domain (in the same device emulator currently).
The examples:
- if only one domain runs Qemu which servers virtio-blk, virtio-net,
virtio-console devices for DomU - only single PCI Host bridge will be
exposed for DomU
- if we add another domain to run Qemu to serve additionally virtio-gpu,
virtio-input and virtio-snd for the *same* DomU - we expose second PCI
Host bridge for DomU

I am afraid, we cannot end up exposing only single PCI Host bridge with
current model (if we use device emulators running in different domains
that handles the *entire* PCI Host bridges), this won't work.

That makes sense and it is fine. But see above, I think only the #2 is necessary for the hypervisor. Patch #5 should not be necessary at all.

[...]

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@xxxxxxxx>
---
   xen/include/public/arch-arm.h | 21 +++++++++++++++++++++
   1 file changed, 21 insertions(+)

diff --git a/xen/include/public/arch-arm.h
b/xen/include/public/arch-arm.h
index a25e87dbda..e6c9cd5335 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -466,6 +466,19 @@ typedef uint64_t xen_callback_t;
   #define GUEST_VPCI_MEM_ADDR                 xen_mk_ullong(0x23000000)
   #define GUEST_VPCI_MEM_SIZE                 xen_mk_ullong(0x10000000)
+/*
+ * 16 MB is reserved for virtio-pci configuration space based on
calculation
+ * 8 bridges * 2 buses x 32 devices x 8 functions x 4 KB = 16 MB

Can you explain how youd ecided the "2"?

good question, we have a limited free space available in memory layout
(we had difficulties to find a suitable holes) also we don't expect a
lot of virtio-pci devices, so "256" used vPCI would be too much. It was
decided to reduce significantly, but select maximum to fit into free
space, with having "2" buses we still fit into the chosen holes.

If you don't expect a lot of virtio devices, then why do you need two buses? Wouldn't one be sufficient?




+ */
+#define GUEST_VIRTIO_PCI_ECAM_BASE          xen_mk_ullong(0x33000000)
+#define GUEST_VIRTIO_PCI_TOTAL_ECAM_SIZE    xen_mk_ullong(0x01000000)
+#define GUEST_VIRTIO_PCI_HOST_ECAM_SIZE     xen_mk_ullong(0x00200000)
+
+/* 64 MB is reserved for virtio-pci memory */
+#define GUEST_VIRTIO_PCI_ADDR_TYPE_MEM    xen_mk_ullong(0x02000000)
+#define GUEST_VIRTIO_PCI_MEM_ADDR         xen_mk_ullong(0x34000000)
+#define GUEST_VIRTIO_PCI_MEM_SIZE         xen_mk_ullong(0x04000000)
+
   /*
    * 16MB == 4096 pages reserved for guest to use as a region to map its
    * grant table in.
@@ -476,6 +489,11 @@ typedef uint64_t xen_callback_t;
   #define GUEST_MAGIC_BASE  xen_mk_ullong(0x39000000)
   #define GUEST_MAGIC_SIZE  xen_mk_ullong(0x01000000)
+/* 64 MB is reserved for virtio-pci Prefetch memory */

This doesn't seem a lot depending on your use case. Can you details how
you can up with "64 MB"?

the same calculation as it was done configuration space. It was observed
that only 16K is used per virtio-pci device (maybe it can be bigger for
usual PCI device, I don't know). Please look at the example of DomU log
below (to strings that contain "*BAR 4: assigned*"):

What about virtio-gpu? I would expect a bit more memory is necessary for that use case.

Any case, what I am looking for is for some explanation in the commit message of the limits. I don't particularly care about the exact limit because this is not part of a stable ABI.

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.