Re: [PATCH 12/13] pci-assign: Generic config space access management

[Avi Kivity <avi@redhat.com>]

On 06/28/2011 11:51 AM, Michael S. Tsirkin wrote:
> On Mon, Jun 27, 2011 at 08:19:55PM +0200, Jan Kiszka wrote:
> >  -/* There can be multiple VNDR capabilities per device, we need to find the
> >  - * one that starts closet to the given address without going over. */
> >  -static uint8_t find_vndr_start(PCIDevice *pci_dev, uint32_t address)
> >  +static uint32_t assigned_dev_pci_read_config(PCIDevice *pci_dev,
> >  +                                             uint32_t address, int len)
> >   {
> >  -    uint8_t cap, pos;
> >  +    AssignedDevice *assigned_dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
> >  +    uint32_t virt_val = pci_default_read_config(pci_dev, address, len);
> >  +    uint32_t real_val, direct_mask;
> >
> >  -    for (cap = pos = 0;
> >  -         (pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_VNDR, pos));
> >  -         pos += PCI_CAP_LIST_NEXT) {
> >  -        if (pos<= address) {
> >  -            cap = MAX(pos, cap);
> >  -        }
> >  +    switch (len) {
> >  +    case 4:
> >  +        direct_mask =
> >  +            pci_get_long(assigned_dev->direct_config_read + address);
> >  +        break;
> >  +    case 2:
> >  +        direct_mask =
> >  +            pci_get_word(assigned_dev->direct_config_read + address);
> >  +        break;
> >  +    case 1:
> >  +        direct_mask =
> >  +            pci_get_byte(assigned_dev->direct_config_read + address);
> >  +        break;
> >  +    default:
> >  +        abort();
> >       }
>
> One small issue here is that I think read config is not guaranteed to
> get a length aligned address, while pci_get_XXX assume length
> alignment. Further, because of alignment you can get beyond
> the end of array.
>
> I don't think it ever happens in practice on the PC,

It cannot happen with CF8/CFC configuration (bits 1:0 of CF8 are 
reserved), but it can happen with mmconfig.

> so we can
> add an assert, but it's might be easier to look at how
> pci_default_read_config handles all these issues:
>
>      uint32_t val = 0;
>      assert(len == 1 || len == 2 || len == 4);
>      len = MIN(len, pci_config_size(d) - address);
>      memcpy(&val, d->config + address, len);
>      return le32_to_cpu(val);
>
> You can replace config with direct access, and check full access
> with:
>
> 	allones = ~0x0;
> 	memcmp(&allones,&val, len)
> and full emulation:
> 	zero = 0x0;
> 	memcmp(&zero,&val, len)
>
> This is not done correctly by current code btw, I think.


I think this should be done by the bus code.

On the original Pentium, a 4-byte write to address 7 would be issued as 
a write to address 0 with #BE7 (byte enable 7) asserted, then a write to 
address 0 with #BE0, #BE1, and #B2 asserted.  That is, a one-byte write 
followed by a 3-byte write.

The memory API has support for this (only at the device level at 
present, but shouldn't be to difficult to implement at the bus level as 
well).

-- 
error compiling committee.c: too many arguments to function