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?
Jan
