Re: [PATCH v6 10/13] vpci/header: reset the command register when adding devices

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 07.02.22 14:54, Jan Beulich wrote:
> On 07.02.2022 13:51, Oleksandr Andrushchenko wrote:
>>
>> On 07.02.22 14:38, Jan Beulich wrote:
>>> On 07.02.2022 12:27, Oleksandr Andrushchenko wrote:
>>>> On 07.02.22 09:29, Jan Beulich wrote:
>>>>> On 04.02.2022 15:37, Oleksandr Andrushchenko wrote:
>>>>>> On 04.02.22 16:30, Jan Beulich wrote:
>>>>>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
>>>>>>>> Reset the command register when assigning a PCI device to a guest:
>>>>>>>> according to the PCI spec the PCI_COMMAND register is typically all 0's
>>>>>>>> after reset.
>>>>>>> It's not entirely clear to me whether setting the hardware register to
>>>>>>> zero is okay. What wants to be zero is the value the guest observes
>>>>>>> initially.
>>>>>> "the PCI spec says the PCI_COMMAND register is typically all 0's after 
>>>>>> reset."
>>>>>> Why wouldn't it be ok? What is the exact concern here?
>>>>> The concern is - as voiced is similar ways before, perhaps in other
>>>>> contexts - that you need to consider bit-by-bit whether overwriting
>>>>> with 0 what is currently there is okay. Xen and/or Dom0 may have put
>>>>> values there which they expect to remain unaltered. I guess
>>>>> PCI_COMMAND_SERR is a good example: While the guest's view of this
>>>>> will want to be zero initially, the host having set it to 1 may not
>>>>> easily be overwritten with 0, or else you'd effectively imply giving
>>>>> the guest control of the bit.
>>>> We have already discussed in great detail PCI_COMMAND emulation [1].
>>>> At the end you wrote [1]:
>>>> "Well, in order for the whole thing to be security supported it needs to
>>>> be explained for every bit why it is safe to allow the guest to drive it.
>>>> Until you mean vPCI to reach that state, leaving TODO notes in the code
>>>> for anything not investigated may indeed be good enough.
>>>>
>>>> Jan"
>>>>
>>>> So, this is why I left a TODO in the PCI_COMMAND emulation for now and only
>>>> care about INTx which is honored with the code in this patch.
>>> Right. The issue I see is that the description does not have any
>>> mention of this, but instead talks about simply writing zero.
>> How do you want that mentioned? Extended commit message or
>> just a link to the thread [1]?
> What I'd like you to describe is what the change does without
> fundamentally implying it'll end up being zero which gets written
> to the register. Stating as a conclusion that for the time being
> this means writing zero is certainly fine (and likely helpful if
> made explicit).
Xen and/or Dom0 may have put values in PCI_COMMAND which they expect
to remain unaltered. PCI_COMMAND_SERR bit is a good example: while the
guest's view of this will want to be zero initially, the host having set
it to 1 may not easily be overwritten with 0, or else we'd effectively
imply giving the guest control of the bit. Thus, PCI_COMMAND register needs
proper emulation in order to honor host's settings.

There are examples of emulators [1], [2] which already deal with PCI_COMMAND
register emulation and it seems that at most they care about the only INTX
bit (besides IO/memory enable and bus muster which are write through).
It could be because in order to properly emulate the PCI_COMMAND register
we need to know about the whole PCI topology, e.g. if any setting in device's
command register is aligned with the upstream port etc.
This makes me think that because of this complexity others just ignore that.
Neither I think this can be easily done in Xen case.

According to "PCI LOCAL BUS SPECIFICATION, REV. 3.0", section "6.2.2
Device Control" says that the reset state of the command register is
typically 0, so reset the command register when assigning a PCI device
to a guest t all 0's and for now only make sure INTx bit is set according
to if MSI/MSI-X enabled.

[1] https://github.com/qemu/qemu/blob/master/hw/xen/xen_pt_config_init.c#L310
[2] 
https://github.com/projectacrn/acrn-hypervisor/blob/master/hypervisor/hw/pci.c#L336

Will the above description be enough?

It also seems to be a good move to squash the following patches:
[PATCH v6 09/13] vpci/header: emulate PCI_COMMAND register for guests
[PATCH v6 10/13] vpci/header: reset the command register when adding devices

as they implement a single piece of functionality now.
>> With the above done, do you think that writing 0's is an acceptable
>> approach as of now?
> Well, yes, provided we have a sufficiently similar understanding
> of what "acceptable" here means.
>
> Jan
>
Thank you,
Oleksandr