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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 3391 bytes --]

On 2011-06-28 10:30, Michael S. Tsirkin wrote:
> 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.

OK, that sounds reasonable.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]