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

[Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>]

On 15.11.23 20:33, Julien Grall wrote:
> Hi Oleksandr,

Hello Julien


> 
> On 15/11/2023 18:14, Oleksandr Tyshchenko wrote:
>> On 15.11.23 19:31, Julien Grall wrote:
>>> On 15/11/2023 16:51, Oleksandr Tyshchenko wrote:
>>>> On 15.11.23 14:33, Julien Grall wrote:
>>>>> 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.
>>
>>
>> Hmm, sounds surprising I would say. Are you saying that unmodified Qemu
>> will work if we drop #5?
> 
> I don't know if an unmodified QEMU will work. My point is I don't view 
> the patch in Xen necessary. You should be able to tell QEMU "here is the 
> ECAM region, please emulate an hostbridge". QEMU will then register the 
> region to Xen and all the accesses will be forwarded. >
> In the future we may need a patch similar to #5 if we want to have 
> multiple DM using the same hostbridge. But this is a different 
> discussion and the patch would need some rework.


ok

> 
> The ioreq.c code was always meant to be generic and is always for every 
> emulated MMIO. So you want to limit any change in it. Checking the MMIO 
> region belongs to the hostbridge and doing the translation is IMHO not a 
> good idea to do in ioreq.c. Instead you want to do the conversion from 
> MMIO to (sbdf, offset) in virtio_pci_mmio{read, write}(). So the job of 
> ioreq.c is to simply find the correct Device Model and forward it.



Are you about virtio_pci_ioreq_server_get_addr() called from 
arch_ioreq_server_get_type_addr()? If so and if I am not mistaken the 
x86 also check what PCI device is targeted there.

But, I am not against the suggestion, I agree with it.


> 
> I also don't see why the feature is gated by has_vcpi(). They are two 
> distinct features (at least in your current model).

yes, you are correct. In #5 virtio-pci mmio handlers are still 
registered in domain_vpci_init() (which is gated by has_vcpi()), etc


> 
> Cheers,
>