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

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jun 28, 2011 at 10:18:20AM +0200, Jan Kiszka wrote:
> > Is this an optimization? What harm does it do to virtualize always?
> 
> Just replicating the old behavior,

Oh, I absolutely agree with that.
It's better to do more cleanups in separate patches,
this one is huge so it's better if bisect can lead us to
a specific change.

> but I can virtualize it unconditionally.


Just pointing out follow-up
cleanups that become possible.

> > 
> >>  
> >>      return 0;
> >> @@ -1694,6 +1571,26 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
> >>          return -1;
> >>      }
> >>  
> >> +    /*
> >> +     * Set up basic config space access control. Will be further refined during
> >> +     * device initialization.
> >> +     *
> >> +     * Direct read/write access is grangted for:
> > 
> > typo
> > 
> >> +     *  - status & command registers, revision ID & class code, cacheline size,
> >> +     *    latency timer, header type, BIST
> >> +     *  - cardbus CIS pointer, subsystem vendor & ID
> >> +     *  - reserved fields
> >> +     *  - Min_Gnt & Max_Lat
> >> +     */
> >> +    memset(dev->direct_config_read + PCI_COMMAND, 0xff, 12);
> >> +    memset(dev->direct_config_read + PCI_CARDBUS_CIS, 0xff, 8);
> >> +    memset(dev->direct_config_read + PCI_CAPABILITY_LIST + 1, 0xff, 7);
> >> +    memset(dev->direct_config_read + PCI_MIN_GNT, 0xff, 2);
> >> +    memset(dev->direct_config_write + PCI_COMMAND, 0xff, 12);
> >> +    memset(dev->direct_config_write + PCI_CARDBUS_CIS, 0xff, 8);
> >> +    memset(dev->direct_config_write + PCI_CAPABILITY_LIST + 1, 0xff, 7);
> >> +    memset(dev->direct_config_write + PCI_MIN_GNT, 0xff, 2);
> >> +
> >>      if (get_real_device(dev, dev->host.seg, dev->host.bus,
> >>                          dev->host.dev, dev->host.func)) {
> >>          error_report("pci-assign: Error: Couldn't get real device (%s)!",
> >> diff --git a/hw/device-assignment.h b/hw/device-assignment.h
> >> index 3d20207..ed5defb 100644
> >> --- a/hw/device-assignment.h
> >> +++ b/hw/device-assignment.h
> >> @@ -104,12 +104,13 @@ typedef struct AssignedDevice {
> >>  #define ASSIGNED_DEVICE_MSIX_MASKED (1 << 2)
> >>          uint32_t state;
> >>      } cap;
> >> +    uint8_t direct_config_read[PCI_CONFIG_SPACE_SIZE];
> >> +    uint8_t direct_config_write[PCI_CONFIG_SPACE_SIZE];
> > 
> > What direct means isn't obvious. Add a comment?
> 
> Ok.
> 
> > 
> > We could revert the logic, have bits for
> > emulate_config_read/emulate_config_write.  I think that most of the
> > registers can safely be read/written directly, so protecting relevant
> > bits will be less work. Is that true?
> 
> I intentionally chose whitelisting to avoid accidentally granting access
> to forgotten fields. I don't think it's a good idea to switch to
> blacklisting.
> 
> Jan
> 

At least in theory iommu protects us from most harm device can do
(besides downstream transactions, which is why we must protect the BAR).
Even if we change things, it probably should be a separate patch since
whitelisting is what existing code had.
However, I think emulate_xx is just a better name for the field.
You can preset them to all ones if you think it's safer.

-- 
MST