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

Avi Kivity
2011-06-28T09:10:57Z

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