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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Roger,

> On 15 Oct 2021, at 10:53, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> 
> On Fri, Oct 15, 2021 at 07:53:38AM +0000, Bertrand Marquis wrote:
>>> On 15 Oct 2021, at 08:44, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>>> On Thu, Oct 14, 2021 at 03:49:49PM +0100, Bertrand Marquis wrote:
>>>> @@ -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;
>>>> -
>>>> -    /*
>>>> -     * 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);
>>> 
>>> I think it would be better for vpci_ecam_mmio_read to just return the
>>> read value, or ~0 in case of error, at least that interface would be
>>> simpler and suitable for x86.
>> 
>> I am not quite sure on this as on absolute to read ~0 is possible so the
>> caller cannot distinguish between properly reading ~0 or an access allowed 
>> error.
> 
> How do you report an access allowed error on Arm for the PCI config
> space?

This does not go that far. In the current case this is actually an unaligned
access to mmio which will trigger and exception before reaching the PCI config 
space.

> 
> At least on x86 I don't think we currently have a way to propagate
> such errors, neither a plan to do so that I'm aware.

The behaviour on x86 will not change as we will ignore this.

Regards
Bertrand

> 
> Thanks, Roger.