Re: [PATCH v3 kvmtool 24/32] pci: Limit configuration transaction size to 32 bits

[Alexandru Elisei <alexandru.elisei@xxxxxxx>]

Hi,

On 5/4/20 2:00 PM, Alexandru Elisei wrote:
> Hi,
>
> On 4/2/20 9:34 AM, André Przywara wrote:
>> On 26/03/2020 15:24, Alexandru Elisei wrote:
>>> From PCI Local Bus Specification Revision 3.0. section 3.8 "64-Bit Bus
>>> Extension":
>>>
>>> "The bandwidth requirements for I/O and configuration transactions cannot
>>> justify the added complexity, and, therefore, only memory transactions
>>> support 64-bit data transfers".
>>>
>>> Further down, the spec also describes the possible responses of a target
>>> which has been requested to do a 64-bit transaction. Limit the transaction
>>> to the lower 32 bits, to match the second accepted behaviour.
>> That looks like a reasonable behaviour.
>> AFAICS there is one code path from powerpc/spapr_pci.c which isn't
>> covered by those limitations (rtas_ibm_write_pci_config() ->
>> pci__config_wr() -> cfg_ops.write() -> vfio_pci_cfg_write()).
>> Same for read.
> The code compares the access size to 1, 2 and 4, so I think powerpc doesn't expect
> 64 bit accesses either. The change looks straightforward, I'll do it for consistency.
>
Read the code more carefully and powerpc already limits the access size to 4 bytes:

static void rtas_ibm_read_pci_config(struct kvm_cpu *vcpu,
                     uint32_t token, uint32_t nargs,
                     target_ulong args,
                     uint32_t nret, target_ulong rets)
{
    [..]
    if (buid != phb.buid || !dev || (size > 4)) {
        phb_dprintf("- cfgRd buid 0x%lx cfg addr 0x%x size %d not found\n",
                buid, addr.w, size);

        rtas_st(vcpu->kvm, rets, 0, -1);
        return;
    }
    pci__config_rd(vcpu->kvm, addr, &val, size);
    [..]
}

It's the same for all the functions where pci__config_{rd,wr} are called directly.
So no changes are needed.

Thanks,
Alex