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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jan,

> On 14 Oct 2021, at 17:06, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> 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?

The function was global before.

> 
>> @@ -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 ...

Yes :-)

> 
>> -    /*
>> -     * 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).

I am not sure in the first place why this is actually ignored and just
returning a -1 value.
If an access is not right, an exception should be generated to the
Guest instead.
When we do that on arm the function is returning an error to the upper
layer in that case, that’s why I did keep a generic function informing the
caller.

So I think it is right for the function to return an error if the access is not 
allowed but I agree the comment on x86 could get a better justification.
@Roger: could you help finding one here as I do not quite understand why it is 
ok to ignore this case ?

> 
>> @@ -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".

Agree I would just need some help on the why.
Now there was no comment before to explain why so I could also
remove the comment altogether.

> 
>> --- 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.

Other architectures might allow this case. So this is specific to Arm.

> 
>> +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)?

This is what the mmio handlers should return to say that an access
was ok or not so the function stick to this standard.

> 
> 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.

Right using CONFIG_64BIT is a good idea here in fact.
I can do that in the next version.

> 
> 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.

I was thinking the mmio here stands more for “mmio handler” as to function
handling emulation through mmio access.
I have no objection to remove the mmio part though.

Regards
Bertrand

> 
> Jan
>