Re: [Qemu-devel] [PATCH] try to implement complete pci-to-pci bridge emulator.

[Isaku Yamahata]

Thank you for review.

On Fri, May 01, 2009 at 08:17:50AM -0600, M. Warner Losh wrote:
> In message: <address@hidden>
>             Isaku Yamahata <address@hidden> writes:
> : Signed-off-by: Isaku Yamahata <address@hidden>
> : ---
> :  hw/pci.c      |  175 
> +++++++++++++++++++++++++++++++++++++++------------------
> :  hw/pci.h      |   30 ++++++++++
> :  hw/unin_pci.c |    2 +-
> :  3 files changed, 151 insertions(+), 56 deletions(-)
> : 
> : diff --git a/hw/pci.c b/hw/pci.c
> : index 0be3662..a1123cb 100644
> : --- a/hw/pci.c
> : +++ b/hw/pci.c
> ...
> : +        case 0x01:
> : +        case 0x02:
> : +        case 0x03:
> : +        case 0x06:
> : +        case 0x07:
> : +        case 0x08:
> : +        case 0x09:
> : +        case 0x0a:
> : +        case 0x0b:
> : +        case 0x0e:
> : +        case 0x10 ... 0x27: /* base */
> 
> This is wrong.  BARs are writable, and must be writable to determine
> their size.

BAR/ROM case is handled by the if clause specially, so the case
doesn't make sense.
I didn't changed the original logic. Yes, these switch is confusing. 


> : +        case 0x2c ... 0x2f: /* read-only subsystem ID & vendor ID */
> : +        case 0x30 ... 0x33: /* rom */
> 
> This is wrong too.  You have to be able to turn on and off this
> mapping by writing bit 0.

ditto.

Hmm, your comment makes me think of whole rewriting PCI configuration
space write emulation.
Would it be a good idea? or is it overkill because the current code is
just enough to make OSes work?

I'm thinking of something of the following (pseudo) code.
What do you think? I want feed back before digging deep into PCI
spec details and coding. Maybe the special treatment of BAR/ROM
registers can be removed with this logic.

struct pci_config_reg {
       uint8 writable_mask;
       void (*changed)(struct pci_dev* dev);
};

struct pci_config_reg regs[] = {
        /* register 0 */
       { .writable_mask = 0 /* read only */
         .changed = NULL 
       },
       ...
};

pci_default_write_config()
        for (...) {
            d->config[addr] =
             (regs[addr].writable_mask & val) |
             (~regs[addr].writable_mask & d->config[addr]);
             if (regs[addr].changed)
                (regs[addr].changed)(d)


> : +        case 0x3d:
> : +            can_write = 0;
> :              break;
> 
> General comment: hard coded magic numbers like the above are very hard
> to read.

So as the original code is. Okay, then I'll introduce more PCI
related constants and use them...
-- 
yamahata