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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 29.10.21 10:33, Roger Pau Monné wrote:
> On Thu, Oct 28, 2021 at 05:55:25PM +0000, Oleksandr Andrushchenko wrote:
>>
>> 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.
> It's possible on x86 to have multiple ECAM regions, whether that means
> multiple host bridges, or host bridges having multiple ECAM regions is
> unknown to me. It's all reported in the MCFG ACPI table (see PCI
> Firmware document for the detailed description of MCFG) using the
> "Configuration Space Base Address Allocation Structure", and there can
> be multiple of those structures.
As we are currently supporting generic ECAM host bridge which
has a single ECAM region I think the existing code we have and
about to upstream is ok as is for now.
I own a bridge which has 2 ECAM regions, so I will work towards
adding its support soon.
>
>> 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
> Indeed. Without fixing that passthrough to domUs is completely unsafe,
> as you allow domUs full access to registers not explicitly handled by
> current vPCI code.
Well, my understanding is: we can let the guest access whatever
registers it wants with the following exceptions:
- "special" registers we already trap in vPCI, e.g. command, BARs
- we must not let the guest go out of the configuration space of a
specific PCI device, e.g. prevent it from accessing configuration
spaces of other devices.
The rest accesses seem to be ok to me as we do not really want:
- have handlers and emulate all possible registers
- we do not want the guest to fail if it accesses a valid register which
we do not emulate.
>
> Regards, Roger.
>
Thanks,
Oleksandr