Re: [PATCH] xen/arm: fix SBDF calculation for vPCI MMIO handlers

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 28.10.21 19:03, Roger Pau Monné wrote:
> On Thu, Oct 28, 2021 at 02:23:34PM +0000, Oleksandr Andrushchenko wrote:
>>
>> On 28.10.21 16:36, Roger Pau Monné wrote:
>>> On Thu, Oct 28, 2021 at 12:09:23PM +0000, Oleksandr Andrushchenko wrote:
>>>> Hi, Julien!
>>>>
>>>> On 27.10.21 20:35, Julien Grall wrote:
>>>>> Hi Oleksandr,
>>>>>
>>>>> On 27/10/2021 09:25, Oleksandr Andrushchenko wrote:
>>>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>>>>>
>>>>>> While in vPCI MMIO trap handlers for the guest PCI host bridge it is not
>>>>>> enough for SBDF translation to simply call VPCI_ECAM_BDF(info->gpa) as
>>>>>> the base address may not be aligned in the way that the translation
>>>>>> always work. If not adjusted with respect to the base address it may not 
>>>>>> be
>>>>>> able to properly convert SBDF and crashes:
>>>>>>
>>>>>> (XEN) vpci_mmio_read 0000:65:1a.0 reg 8bc gpa e65d08bc
>>>>> I can't find a printk() that may output this message. Where does this 
>>>>> comes from?
>>>> That was a debug print. I shouldn't have used that in the patch 
>>>> description, but
>>>> probably after "---" to better explain what's happening
>>>>> Anyway, IIUC the guest physical address is 0xe65d08bc which, if I am not 
>>>>> mistaken, doesn't belong to the range advertised for GUEST_VPCI_ECAM.
>>>> This is from dom0 I am working on now.
>>>>> IMHO, the stack trace should come from usptream Xen or need some 
>>>>> information to explain how this was reproduced.
>>>>>
>>>>>> (XEN) Data Abort Trap. Syndrome=0x6
>>>>>> (XEN) Walking Hypervisor VA 0x467a28bc on CPU0 via TTBR 
>>>>>> 0x00000000481d5000
>>>>> I can understnad that if we don't substract GUEST_VPCI_ECAM, we would (in 
>>>>> theory) not get the correct BDF. But... I don't understand how this would 
>>>>> result to a data abort in the hypervisor.
>>>>>
>>>>> In fact, I think the vPCI code should be resilient enough to not crash if 
>>>>> we pass the wrong BDF.
>>>> Well, there is no (?) easy way to validate SBDF. And this could be a 
>>>> problem if we have a misbehaving
>>>> guest which may force Xen to access the memory beyond that of PCI host 
>>>> bridge
>>> How could that be? The ECAM region exposed to the guest you should be
>>> the same as the physical one for dom0?
>> Ok, I have a Designware PCI hist which has 2 ECAM regions (I am starting to
>> implement the driver for it, so I can be wrong here):
>> - Root Complex ECAM area ("dbi"), it is something like 0x3000 bytes long
>> - "Client" ECAM area ("config")
>> So from Dom0 POV we have 2 ECAM regions and for the guest
>> we always emulate a single big region:
> You need support for multiple ECAM regions. That's how we do it on x86
> PVH dom0. See register_vpci_mmcfg_handler and related machinery.
Is it common for a PCI host bridge to have multiple ECAM regions?
Currently on Arm we were about to support "pci-host-ecam-generic" [1],
e.g. generic ECAM host bridge which normally (?) has a single ECAM
region [2]. But the host bridge I want to support has multiple, so
strictly speaking it is not the one that we implement.

Arm folks, do we want this generalization at this moment to align with x86
with this respect?

We can live with the current approach and when I have my driver implemented
I can send patches to make that generalization.
>
>> /*
>>    * 256 MB is reserved for VPCI configuration space based on calculation
>>    * 256 buses x 32 devices x 8 functions x 4 KB = 256 MB
>>    */
>> #define GUEST_VPCI_ECAM_BASE    xen_mk_ullong(0x10000000)
>> #define GUEST_VPCI_ECAM_SIZE    xen_mk_ullong(0x10000000)
>>
>> So, we have the base address and size of the emulated ECAM space
>> not connected to the real host bridge
>>> And for domUs you really need to fix vpci_{read,write} to not
>>> passthrough accesses not explicitly handled.
>> Do you mean that we need to validate SBDFs there?
>> This can be tricky if we have a use-case when a PCI device being
>> passed through if not put at 0000:00:0.0, but requested to be, for
>> example, 0000:0d:0.0. So, we need to go over the list of virtual
>> devices and see if SBDF the guest is trying to access is a valid SBDF.
>> Is this what you mean?
> No, you need to prevent accesses to registers not explicitly handled
> by vpci. Ie: do not forward unhandled accesses to
> vpci_{read,wrie}_hw).
I see, so those which have no handlers are not passed to the hardware.
I need to see how to do that
>
> Regards, Roger.
Thank you,
Oleksandr

[1] 
https://elixir.bootlin.com/linux/v5.15-rc7/source/drivers/pci/controller/pci-host-generic.c
[2] 
https://elixir.bootlin.com/linux/v5.15-rc7/source/drivers/pci/controller/pci-host-common.c#L23