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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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?

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.

Thanks, Roger.