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

[Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>]

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. 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.

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.

Please note, I might miss some bits since this enabling work.


> 
>>
>> For that purpose, reserve separate virtio-pci resources (memory and 
>> SPI range
>> for Legacy PCI interrupts) up to 8 possible PCI hosts (to be aligned with
> 
> Do you mean host bridge rather than host?

yes


> 
>> MAX_NR_IOREQ_SERVERS) and allocate a host per backend domain. The PCI 
>> host
>> details including its host_id to be written to dedicated Xenstore node 
>> for
>> the device-model to retrieve.
> 
> So which with approach, who is decide which BDF will be used for a given 
> virtio PCI device?


toolstack (via configuration file)

> 
>>
>> 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.


> 
>> + */
>> +#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*"):


> root@h3ulcb:~# dmesg | grep pci
> [    0.444163] pci-host-generic 33000000.pcie: host bridge /pcie@33000000 
> ranges:
> [    0.444257] pci-host-generic 33000000.pcie:      MEM 
> 0x0034000000..0x00347fffff -> 0x0034000000
> [    0.444304] pci-host-generic 33000000.pcie:      MEM 
> 0x003a000000..0x003a7fffff -> 0x003a000000
> [    0.444396] pci-host-generic 33000000.pcie: ECAM at [mem 
> 0x33000000-0x331fffff] for [bus 00-01]
> [    0.444595] pci-host-generic 33000000.pcie: PCI host bridge to bus 0000:00
> [    0.444635] pci_bus 0000:00: root bus resource [bus 00-01]
> [    0.444662] pci_bus 0000:00: root bus resource [mem 0x34000000-0x347fffff]
> [    0.444692] pci_bus 0000:00: root bus resource [mem 0x3a000000-0x3a7fffff 
> pref]
> [    0.445193] pci 0000:00:00.0: [1b36:0008] type 00 class 0x060000
> [    0.449493] pci 0000:00:08.0: [1af4:1042] type 00 class 0x010000           
>                                                                               
>                                                
> [    0.451760] pci 0000:00:08.0: reg 0x20: [mem 0x00000000-0x00003fff 64bit 
> pref]                                                                         
>                                                  
> [    0.455985] pci 0000:00:08.0: BAR 4: assigned [mem 0x3a000000-0x3a003fff 
> 64bit pref]   
> [    0.456678] pci-host-generic 33200000.pcie: host bridge /pcie@33200000 
> ranges:
> [    0.456748] pci-host-generic 33200000.pcie:      MEM 
> 0x0034800000..0x0034ffffff -> 0x0034800000
> [    0.456793] pci-host-generic 33200000.pcie:      MEM 
> 0x003a800000..0x003affffff -> 0x003a800000
> [    0.456879] pci-host-generic 33200000.pcie: ECAM at [mem 
> 0x33200000-0x333fffff] for [bus 00-01]
> [    0.457038] pci-host-generic 33200000.pcie: PCI host bridge to bus 0001:00
> [    0.457079] pci_bus 0001:00: root bus resource [bus 00-01]
> [    0.457106] pci_bus 0001:00: root bus resource [mem 0x34800000-0x34ffffff]
> [    0.457136] pci_bus 0001:00: root bus resource [mem 0x3a800000-0x3affffff 
> pref]
> [    0.457808] pci 0001:00:00.0: [1b36:0008] type 00 class 0x060000
> [    0.461401] pci 0001:00:01.0: [1af4:1041] type 00 class 0x020000
> [    0.463537] pci 0001:00:01.0: reg 0x20: [mem 0x00000000-0x00003fff 64bit 
> pref]
> [    0.468135] pci 0001:00:02.0: [1af4:1042] type 00 class 0x010000
> [    0.470185] pci 0001:00:02.0: reg 0x20: [mem 0x00000000-0x00003fff 64bit 
> pref]
> [    0.474575] pci 0001:00:03.0: [1af4:1050] type 00 class 0x038000
> [    0.476534] pci 0001:00:03.0: reg 0x20: [mem 0x00000000-0x00003fff 64bit 
> pref]
> [    0.480942] pci 0001:00:04.0: [1af4:1052] type 00 class 0x090000
> [    0.483096] pci 0001:00:04.0: reg 0x20: [mem 0x00000000-0x00003fff 64bit 
> pref]
> [    0.487631] pci 0001:00:06.0: [1af4:1053] type 00 class 0x078000
> [    0.489693] pci 0001:00:06.0: reg 0x20: [mem 0x00000000-0x00003fff 64bit 
> pref]
> [    0.493669] pci 0001:00:01.0: BAR 4: assigned [mem 0x3a800000-0x3a803fff 
> 64bit pref]                                                                   
>                                                  
> [    0.495840] pci 0001:00:02.0: BAR 4: assigned [mem 0x3a804000-0x3a807fff 
> 64bit pref]                                                                   
>                                                  
> [    0.496656] pci 0001:00:03.0: BAR 4: assigned [mem 0x3a808000-0x3a80bfff 
> 64bit pref]                                                                   
>                                                  
> [    0.497671] pci 0001:00:04.0: BAR 4: assigned [mem 0x3a80c000-0x3a80ffff 
> 64bit pref]                                                                   
>                                                  
> [    0.498320] pci 0001:00:06.0: BAR 4: assigned [mem 0x3a810000-0x3a813fff 
> 64bit pref]      
> [    0.500732] virtio-pci 0000:00:08.0: enabling device (0000 -> 0002)
> [    0.509728] virtio-pci 0001:00:01.0: enabling device (0000 -> 0002)
> [    0.529757] virtio-pci 0001:00:02.0: enabling device (0000 -> 0002)
> [    0.550208] virtio-pci 0001:00:03.0: enabling device (0000 -> 0002)
> [    0.571012] virtio-pci 0001:00:04.0: enabling device (0000 -> 0002)
> [    0.591042] virtio-pci 0001:00:06.0: enabling device (0000 -> 0002)



> 
>> +#define GUEST_VIRTIO_PCI_ADDR_TYPE_PREFETCH_MEM    
>> xen_mk_ullong(0x42000000)
>> +#define GUEST_VIRTIO_PCI_PREFETCH_MEM_ADDR         
>> xen_mk_ullong(0x3a000000)
>> +#define GUEST_VIRTIO_PCI_PREFETCH_MEM_SIZE         
>> xen_mk_ullong(0x04000000)
>> +
>>   #define GUEST_RAM_BANKS   2
>>   /*
>> @@ -515,6 +533,9 @@ typedef uint64_t xen_callback_t;
>>   #define GUEST_VIRTIO_MMIO_SPI_FIRST   33
>>   #define GUEST_VIRTIO_MMIO_SPI_LAST    43
>> +#define GUEST_VIRTIO_PCI_SPI_FIRST   44
>> +#define GUEST_VIRTIO_PCI_SPI_LAST    76
> 
> Just to confirm this is 4 interrupts per PCI host bridge? If so, can 
> this be clarified in a comment?

yes (INTA - INTD)


> 
> Also, above you said that the host ID will be written to Xenstore. But 
> who will decide which SPI belongs to which host bridge?


toolstack, it in charge of PCI host bridge resources allocation (as we 
need to both properly create PCI Host bridge device tree node and inform 
device emulator about PCI host bridge details). Please take a look at:

https://patchwork.kernel.org/project/xen-devel/patch/20231115112611.3865905-4-Sergiy_Kibrik@xxxxxxxx/

> 
> Cheers,
>