Re: [Qemu-devel] [PATCH] pci: Length-align config space accesses

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2011-07-20 16:22, Michael S. Tsirkin wrote:
> On Tue, Jul 19, 2011 at 11:39:02PM +0200, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Introduce pci_config_read/write helpers to split up config space
>> accesses that are not length-aligned. This particularly avoids that each
>> and every device needs to check for config space overruns. Also move the
>> access length assertion to the new helpers.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Nice, this is possible now that everyone uses pci_host.
> Some comments below.
> 
>> ---
>>
>> This will also simplify my cap refactorings for the device-assignment
>> code in qemu-kvm.
>>
>>  hw/pci.c       |   43 +++++++++++++++++++++++++++++++++++++++----
>>  hw/pci.h       |    5 +++++
>>  hw/pci_host.c  |   14 ++++++--------
>>  hw/pcie_host.c |   12 ++++++------
>>  4 files changed, 56 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/pci.c b/hw/pci.c
>> index b904a4e..6957ece 100644
>> --- a/hw/pci.c
>> +++ b/hw/pci.c
>> @@ -1104,12 +1104,48 @@ static void pci_update_irq_disabled(PCIDevice *d, int was_irq_disabled)
>>      }
>>  }
>>  
>> +void pci_config_write(PCIDevice *pci_dev, uint32_t addr, uint32_t addr_mask,
>> +                      uint32_t val, uint32_t len)
> 
> Let's put these in pci_host.h/pci_host.c, and prefix in a way
> so we don't want devices to use this mistakenly.

I didn't find direct relations between pcie_host.c and pci_host.c in the
current code, so went for pci.c - but no problem to change.

pci_host_config_read/write?

> Also please add a comment here saying what this does.
> Maybe also note how, if some system device ever does
> need misaligned accesses, it will need some special hacks.

OK.

> 
>> +{
>> +    assert(len == 1 || len == 2 || len == 4);
>> +
>> +    addr &= addr_mask;
> 
> A 4 byte access at address 255 would wrap around to 0 with this,
> while previously we ignored high bits on write and returned
> 0 on read.

The question is rather what the spec or real hw demand from us. Do you
have any insights on this? I didn't find hints which behavior is more
compliant. It looks like such access patterns are deep into bug land anyway.

> 
> I'd say let's check config size. With some more comments
> this gets:
> 
> 	if (addr >= pci_config_size(pci_dev)) {
> 		return;
> 	}
> 
> 	if (likely(!(addr & (len - 1)))) {
> 	       pci_dev->config_write(pci_dev, addr, val, len);
> 	       return;
> 	}
> 
> 	/* Handle unaligned access: split it to two accesses each
> 	 * half the length of the original one. */
> 	len = len / 2;
> 	pci_host_config_write(pci_dev, addr, addr_mask, val, len);
> 	pci_host_config_write(pci_dev, addr + len, config_size, val >> (len * 8), len);
> 
> I am kind of unhappy with adding recursion here but
> it does seem to be the least of evils, and
> it is rare (and can't happen on the pc).

+ recursion depth is naturally limited.

> 
>> +
>> +    if (addr & (len - 1)) {
> 
> Let's add an unlikely here.
> 
>> +        len >>= 1;
> 
> I know it's the same but I prefer len /= 2;

OK for both.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux