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:
signature.asc
Description: OpenPGP digital signature
