Re: [PATCH v3 08/11] vpci/header: Emulate PCI_COMMAND register for guests

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 03.11.21 11:39, Roger Pau Monné wrote:
> On Wed, Nov 03, 2021 at 09:18:03AM +0000, Oleksandr Andrushchenko wrote:
>>
>> On 03.11.21 11:11, Jan Beulich wrote:
>>> On 03.11.2021 09:53, Oleksandr Andrushchenko wrote:
>>>> On 02.11.21 16:10, Oleksandr Andrushchenko wrote:
>>>>> On 02.11.21 15:54, Jan Beulich wrote:
>>>>>> On 02.11.2021 12:50, Roger Pau Monné wrote:
>>>>>>> On Tue, Nov 02, 2021 at 12:19:13PM +0100, Jan Beulich wrote:
>>>>>>>> On 26.10.2021 12:52, Roger Pau Monné wrote:
>>>>>>>>> On Thu, Sep 30, 2021 at 10:52:20AM +0300, Oleksandr Andrushchenko 
>>>>>>>>> wrote:
>>>>>>>>>> --- a/xen/drivers/vpci/header.c
>>>>>>>>>> +++ b/xen/drivers/vpci/header.c
>>>>>>>>>> @@ -451,6 +451,32 @@ static void cmd_write(const struct pci_dev 
>>>>>>>>>> *pdev, unsigned int reg,
>>>>>>>>>>              pci_conf_write16(pdev->sbdf, reg, cmd);
>>>>>>>>>>      }
>>>>>>>>>>      
>>>>>>>>>> +static void guest_cmd_write(const struct pci_dev *pdev, unsigned 
>>>>>>>>>> int reg,
>>>>>>>>>> +                            uint32_t cmd, void *data)
>>>>>>>>>> +{
>>>>>>>>>> +    /* TODO: Add proper emulation for all bits of the command 
>>>>>>>>>> register. */
>>>>>>>>>> +
>>>>>>>>>> +    if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 )
>>>>>>>>>> +    {
>>>>>>>>>> +        /*
>>>>>>>>>> +         * Guest wants to enable INTx. It can't be enabled if:
>>>>>>>>>> +         *  - host has INTx disabled
>>>>>>>>>> +         *  - MSI/MSI-X enabled
>>>>>>>>>> +         */
>>>>>>>>>> +        if ( pdev->vpci->msi->enabled )
>>>>>>>>>> +            cmd |= PCI_COMMAND_INTX_DISABLE;
>>>>>>>>>> +        else
>>>>>>>>>> +        {
>>>>>>>>>> +            uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg);
>>>>>>>>>> +
>>>>>>>>>> +            if ( current_cmd & PCI_COMMAND_INTX_DISABLE )
>>>>>>>>>> +                cmd |= PCI_COMMAND_INTX_DISABLE;
>>>>>>>>>> +        }
>>>>>>>>> This last part should be Arm specific. On other architectures we
>>>>>>>>> likely want the guest to modify INTx disable in order to select the
>>>>>>>>> interrupt delivery mode for the device.
>>>>>>>> We cannot allow a guest to clear the bit when it has MSI / MSI-X
>>>>>>>> enabled - only one of the three is supposed to be active at a time.
>>>>>>>> (IOW similarly we cannot allow a guest to enable MSI / MSI-X when
>>>>>>>> the bit is clear.)
>>>>>>> Sure, but this code is making the bit sticky, by not allowing
>>>>>>> INTX_DISABLE to be cleared once set. We do not want that behavior on
>>>>>>> x86, as a guest can decide to use MSI or INTx. The else branch needs
>>>>>>> to be Arm only.
>>>>>> Isn't the "else" part questionable even on Arm?
>>>>> It is. Once fixed I can't see anything Arm specific here
>>>> Well, I have looked at the code one more time and everything seems to
>>>> be ok wrt that sticky bit: we have 2 handlers which are cmd_write and
>>>> guest_cmd_write. The former is used for the hardware domain and has
>>>> *no restrictions* on writing PCI_COMMAND register contents and the later
>>>> is only used for guests and which does have restrictions applied in
>>>> emulate_cmd_reg function.
>>>>
>>>> So, for the hardware domain, there is no "sticky" bit possible and for the
>>>> guest domains if the physical contents of the PCI_COMMAND register
>>>> has PCI_COMMAND_INTX_DISABLE bit set then the guest is enforced to
>>>> use PCI_COMMAND_INTX_DISABLE bit set.
>>>>
>>>> So, from hardware domain POV, this should not be a problem, but from
>>>> guests view it can. Let's imagine that the hardware domain can handle
>>>> all types of interrupts, e.g. INTx, MSI, MSI-X. In this case the hardware
>>>> domain can decide what can be used for the interrupt source (again, no
>>>> restriction here) and program PCI_COMMAND accordingly.
>>>> Guest domains need to align with this configuration, e.g. if INTx was 
>>>> disabled
>>>> by the hardware domain then INTx cannot be enabled for guests
>>> Why? It's the DomU that's in control of the device, so it ought to
>>> be able to pick any of the three. I don't think Dom0 is involved in
>>> handling of interrupts from the device, and hence its own "dislike"
>>> of INTx ought to only extend to the period of time where Dom0 is
>>> controlling the device. This would be different if Xen's view was
>>> different, but as we seem to agree Xen's role here is solely to
>>> prevent invalid combinations getting established in hardware.
>> On top of a PCI device there is a physical host bridge and
>> physical bus topology which may impose restrictions from
>> Dom0 POV on that particular device. So, every PCI device
>> being passed through to a DomU may have different INTx
>> settings which do depend on Dom0 in our case.
> Hm, it's kind of weird. What happens if you play with this bit and the
> bridge doesn't support it?
For that reason I think it is enough to relay on some reference value
which shows if INTx can be used. For that I suggest we depend on
Dom0 for now and read this reference PCI_COMMAND value while
in init_bars + is_hardware_domain. Then this can be used to feed
the initial value of the PCI_COMMAND for guests.
This way Dom0 solves the problem "what is supported for this
PCI device with respect to the bus topology and host bridge"
>
> Also note that your current code would allow a domU to set the bit if
> previously unset, but it then won't allow the domU to clear it, which
> doesn't seem to be exactly what you are aiming for.
That was noted before. If we use the reference value and use it
as an initial value of the PCI_COMMAND for the guests (remember
I use 0 in the patch which resets PCI_COMMAND for the guests
and check the real PCI_COMMAND contents to decide on INTx).
So, this reference value can be used in checks:
         if ( pdev->vpci->msi->enabled )
             cmd |= PCI_COMMAND_INTX_DISABLE;
         else
         {
             if ( pdev->cmd_ref_value & PCI_COMMAND_INTX_DISABLE )
                  ^^^^^^^^^^^^^^
                 cmd |= PCI_COMMAND_INTX_DISABLE;
         }

init_bars:
if (hwdom)
  pdev->cmd_ref_value = read(PCI_COMMAND)
>
> Thanks, Roger.
>