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

From: Alexandru Elisei <alexandru.elisei@arm.com>
To: "André Przywara" <andre.przywara@arm.com>, kvm@vger.kernel.org
Cc: will@kernel.org, julien.thierry.kdev@gmail.com,
	sami.mujawar@arm.com, lorenzo.pieralisi@arm.com
Subject: Re: [PATCH v3 kvmtool 24/32] pci: Limit configuration transaction size to 32 bits
Date: Mon, 4 May 2020 14:00:35 +0100	[thread overview]
Message-ID: <30df4164-f7db-c00d-e67c-5d2cdb696354@arm.com> (raw)
In-Reply-To: <c1f0db1a-5df1-492b-8d91-a972fe2a4d42@arm.com>

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.

>
> I am not sure we really care, maybe you can fix it if you like.
>
> Either way this patch seems right, so:
>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Thanks!

Alex

>
> Cheers,
> Andre
>
>> ---
>>  pci.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/pci.c b/pci.c
>> index 7399c76c0819..611e2c0bf1da 100644
>> --- a/pci.c
>> +++ b/pci.c
>> @@ -119,6 +119,9 @@ static bool pci_config_data_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16
>>  {
>>  	union pci_config_address pci_config_address;
>>  
>> +	if (size > 4)
>> +		size = 4;
>> +
>>  	pci_config_address.w = ioport__read32(&pci_config_address_bits);
>>  	/*
>>  	 * If someone accesses PCI configuration space offsets that are not
>> @@ -135,6 +138,9 @@ static bool pci_config_data_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16
>>  {
>>  	union pci_config_address pci_config_address;
>>  
>> +	if (size > 4)
>> +		size = 4;
>> +
>>  	pci_config_address.w = ioport__read32(&pci_config_address_bits);
>>  	/*
>>  	 * If someone accesses PCI configuration space offsets that are not
>> @@ -248,6 +254,9 @@ static void pci_config_mmio_access(struct kvm_cpu *vcpu, u64 addr, u8 *data,
>>  	cfg_addr.w		= (u32)addr;
>>  	cfg_addr.enable_bit	= 1;
>>  
>> +	if (len > 4)
>> +		len = 4;
>> +
>>  	if (is_write)
>>  		pci__config_wr(kvm, cfg_addr, data, len);
>>  	else
>>