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