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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

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: yes, this doesn't
cover dom0less etc. so we do rely on some entity before the guest to set the
PCI_COMMAND correctly.
This is how it is implemented in the patch.
Please also see the discussion we had before [1].

What is not now covered is that if there is a hardware domain and the same PCI
device is first passed to one of the guests and then assigned to another. In 
this case:

hwdom (or any other entity) programs PCI_COMMAND
assign domU1
deassign domU1
*assign domIO*
assign domU2

So in this scenario the host assigned value is lost after assigning to domU1
and domU2 will use the value used by domU1.
So, it seems that this is the only use-case not covered by the patch.

Jan [1]:
"In the absence of Dom0 controlling the device, I think we ought to take
Xen's view as the "host" one. Which will want the bit set at least as
long as either MSI or MSI-X is enabled for the device."

So, for the PCI_COMMAND register we might want to have a reference value
to be stored so we can restore it while assigning the PCI device to a guest.
For the current implementation the best I can probably do is to read this value
in init_bars when it is called for the hardware domain:

if ( is_hardware_domain(d) )
   vpci->pci_command_reference = pci_read(PCI_COMMAND)

And when I want to reset PCI_COMMAND while assigning to a guest I will
use it instead of 0 as it is now.
>> Jan
>>
[1] 
https://lore.kernel.org/xen-devel/20210903100831.177748-9-andr2000@xxxxxxxxx/