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

[Julien Grall <julien@xxxxxxx>]

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?
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.
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.
When there is a data abort in Xen, you should get a stack trace from 
where this comes from. Can you paste it here?
> Fix this by adjusting the gpa with respect to the host bridge base address
> in a way as it is done for x86.
>
> Fixes: d59168dc05a5 ("xen/arm: Enable the existing x86 virtual PCI support for 
> ARM")
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> ---
>   xen/arch/arm/vpci.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
> index 8f40a0dec6d2..23f45386f4b3 100644
> --- a/xen/arch/arm/vpci.c
> +++ b/xen/arch/arm/vpci.c
> @@ -24,7 +24,7 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>       unsigned long data;
>   
>       /* We ignore segment part and always handle segment 0 */
> -    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa);
> +    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE);
Looking at the rest of the rest, it seems that
 1) the issue is latent as the bits 0-27 are clear
 2) this will need to be modified to take into account dom0.

So I would prefer if the base address is passed differently (maybe in 
priv?) from the start. This will avoid extra modification that you 
already plan to have in a follow-up series.
>   
>       if ( vpci_ecam_read(sbdf, ECAM_REG_OFFSET(info->gpa),
>                           1U << info->dabt.size, &data) )
> @@ -44,7 +44,7 @@ static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
>       pci_sbdf_t sbdf;
>   
>       /* We ignore segment part and always handle segment 0 */
> -    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa);
> +    sbdf.sbdf = VPCI_ECAM_BDF(info->gpa - GUEST_VPCI_ECAM_BASE);
>   
>       return vpci_ecam_write(sbdf, ECAM_REG_OFFSET(info->gpa),
>                              1U << info->dabt.size, r);
Cheers,

--
Julien Grall