Re: [PATCH v6 1/3] xen/vpci: Move ecam access functions to common code

[Jan Beulich <jbeulich@suse.com>]

On 14.10.2021 16:49, Bertrand Marquis wrote:
> @@ -305,7 +291,7 @@ static int vpci_portio_read(const struct hvm_io_handler *handler,
>  
>      reg = hvm_pci_decode_addr(cf8, addr, &sbdf);
>  
> -    if ( !vpci_access_allowed(reg, size) )
> +    if ( !vpci_ecam_access_allowed(reg, size) )
>          return X86EMUL_OKAY;
>  
>      *data = vpci_read(sbdf, reg, size);
> @@ -335,7 +321,7 @@ static int vpci_portio_write(const struct hvm_io_handler *handler,
>  
>      reg = hvm_pci_decode_addr(cf8, addr, &sbdf);
>  
> -    if ( !vpci_access_allowed(reg, size) )
> +    if ( !vpci_ecam_access_allowed(reg, size) )
>          return X86EMUL_OKAY;
>  
>      vpci_write(sbdf, reg, size, data);

Why would port I/O functions call an ECAM helper? And in how far is
that helper actually ECAM-specific?

> @@ -434,25 +420,8 @@ static int vpci_mmcfg_read(struct vcpu *v, unsigned long addr,
>      reg = vpci_mmcfg_decode_addr(mmcfg, addr, &sbdf);
>      read_unlock(&d->arch.hvm.mmcfg_lock);
>  
> -    if ( !vpci_access_allowed(reg, len) ||
> -         (reg + len) > PCI_CFG_SPACE_EXP_SIZE )
> -        return X86EMUL_OKAY;

While I assume this earlier behavior is the reason for ...

> -    /*
> -     * According to the PCIe 3.1A specification:
> -     *  - Configuration Reads and Writes must usually be DWORD or smaller
> -     *    in size.
> -     *  - Because Root Complex implementations are not required to support
> -     *    accesses to a RCRB that cross DW boundaries [...] software
> -     *    should take care not to cause the generation of such accesses
> -     *    when accessing a RCRB unless the Root Complex will support the
> -     *    access.
> -     *  Xen however supports 8byte accesses by splitting them into two
> -     *  4byte accesses.
> -     */
> -    *data = vpci_read(sbdf, reg, min(4u, len));
> -    if ( len == 8 )
> -        *data |= (uint64_t)vpci_read(sbdf, reg + 4, 4) << 32;
> +    /* Ignore return code */
> +    vpci_ecam_mmio_read(sbdf, reg, len, data);

... the commented-upon ignoring of the return value, I don't think
that's a good way to deal with things anymore. Instead I think
*data should be written to ~0 upon failure, unless it is intended
for vpci_ecam_mmio_read() to take care of that case (in which case
I'm not sure I would see why it needs to return an error indicator
in the first place).

> @@ -476,13 +445,8 @@ static int vpci_mmcfg_write(struct vcpu *v, unsigned long addr,
>      reg = vpci_mmcfg_decode_addr(mmcfg, addr, &sbdf);
>      read_unlock(&d->arch.hvm.mmcfg_lock);
>  
> -    if ( !vpci_access_allowed(reg, len) ||
> -         (reg + len) > PCI_CFG_SPACE_EXP_SIZE )
> -        return X86EMUL_OKAY;
> -
> -    vpci_write(sbdf, reg, min(4u, len), data);
> -    if ( len == 8 )
> -        vpci_write(sbdf, reg + 4, 4, data >> 32);
> +    /* Ignore return code */
> +    vpci_ecam_mmio_write(sbdf, reg, len, data);

Here ignoring is fine imo, but if you feel it is important to
comment on this, then I think you need to prefer "why" over "what".

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -478,6 +478,66 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
>      spin_unlock(&pdev->vpci->lock);
>  }
>  
> +/* Helper function to check an access size and alignment on vpci space. */
> +bool vpci_ecam_access_allowed(unsigned int reg, unsigned int len)
> +{
> +    /*
> +     * Check access size.
> +     *
> +     * On arm32 or for 32bit guests on arm, 64bit accesses should be forbidden
> +     * but as for those platform ISV register, which gives the access size,
> +     * cannot have a value 3, checking this would just harden the code.
> +     */
> +    if ( len != 1 && len != 2 && len != 4 && len != 8 )
> +        return false;

I'm not convinced talking about Arm specifically here is
warranted, unless there's something there that's clearly
different from all other architectures. Otherwise the comment
should imo be written in more general terms.

> +int vpci_ecam_mmio_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int len,
> +                         unsigned long data)
> +{
> +    if ( !vpci_ecam_access_allowed(reg, len) ||
> +         (reg + len) > PCI_CFG_SPACE_EXP_SIZE )
> +        return 0;
> +
> +    vpci_write(sbdf, reg, min(4u, len), data);
> +    if ( len == 8 )
> +        vpci_write(sbdf, reg + 4, 4, data >> 32);
> +
> +    return 1;
> +}
> +
> +int vpci_ecam_mmio_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len,
> +                        unsigned long *data)
> +{
> +    if ( !vpci_ecam_access_allowed(reg, len) ||
> +         (reg + len) > PCI_CFG_SPACE_EXP_SIZE )
> +        return 0;
> +
> +    /*
> +     * According to the PCIe 3.1A specification:
> +     *  - Configuration Reads and Writes must usually be DWORD or smaller
> +     *    in size.
> +     *  - Because Root Complex implementations are not required to support
> +     *    accesses to a RCRB that cross DW boundaries [...] software
> +     *    should take care not to cause the generation of such accesses
> +     *    when accessing a RCRB unless the Root Complex will support the
> +     *    access.
> +     *  Xen however supports 8byte accesses by splitting them into two
> +     *  4byte accesses.
> +     */
> +    *data = vpci_read(sbdf, reg, min(4u, len));
> +    if ( len == 8 )
> +        *data |= (uint64_t)vpci_read(sbdf, reg + 4, 4) << 32;
> +
> +    return 1;
> +}

Why do these two functions return int/0/1 instead of
bool/false/true (assuming, as per above, that them returning non-
void is warranted at all)?

Also both of these functions will silently misbehave on 32-bit due to
the use of unsigned long in the parameter types. I think you want e.g.
CONFIG_64BIT conditionals here as well as in vpci_access_allowed()
(omitting the questionable "ecam" part of the name) to reject len == 8
there in that case.

Finally, to me, having both "ecam" and "mmio" in the names feels
redundant - the PCI spec doesn't mention any non-MMIO mechanism there
afaics.

Jan