[v6,09/13] vpci/header: emulate PCI_COMMAND register for guests

Commit Message

Oleksandr Andrushchenko Feb. 4, 2022, 6:34 a.m. UTC

From: Oleksandr Andrushchenko <[e-mail]>

Add basic emulation support for guests. At the moment only emulate
PCI_COMMAND_INTX_DISABLE bit, the rest is not emulated yet and left
as TODO.

Signed-off-by: Oleksandr Andrushchenko <[e-mail]>
---
Since v5:
- add additional check for MSI-X enabled while altering INTX bit
- make sure INTx disabled while guests enable MSI/MSI-X
Since v3:
- gate more code on CONFIG_HAS_MSI
- removed logic for the case when MSI/MSI-X not enabled
---
 xen/drivers/vpci/header.c | 21 +++++++++++++++++++--
 xen/drivers/vpci/msi.c    |  4 ++++
 xen/drivers/vpci/msix.c   |  4 ++++
 3 files changed, 27 insertions(+), 2 deletions(-)

Comments

Jan Beulich Feb. 4, 2022, 2:25 p.m. UTC | #1

On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -454,6 +454,22 @@ 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. */
> +
> +#ifdef CONFIG_HAS_PCI_MSI
> +    if ( pdev->vpci->msi->enabled || pdev->vpci->msix->enabled )
> +    {
> +        /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */
> +        cmd |= PCI_COMMAND_INTX_DISABLE;
> +    }
> +#endif
> +
> +    cmd_write(pdev, reg, cmd, data);
> +}

It's not really clear to me whether the TODO warrants this being a
separate function. Personally I'd find it preferable if the logic
was folded into cmd_write().

With this and ...

> --- a/xen/drivers/vpci/msi.c
> +++ b/xen/drivers/vpci/msi.c
> @@ -70,6 +70,10 @@ static void control_write(const struct pci_dev *pdev, unsigned int reg,
>  
>          if ( vpci_msi_arch_enable(msi, pdev, vectors) )
>              return;
> +
> +        /* Make sure guest doesn't enable INTx while enabling MSI. */
> +        if ( !is_hardware_domain(pdev->domain) )
> +            pci_intx(pdev, false);
>      }
>      else
>          vpci_msi_arch_disable(msi, pdev);
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -92,6 +92,10 @@ static void control_write(const struct pci_dev *pdev, unsigned int reg,
>          for ( i = 0; i < msix->max_entries; i++ )
>              if ( !msix->entries[i].masked && msix->entries[i].updated )
>                  update_entry(&msix->entries[i], pdev, i);
> +
> +        /* Make sure guest doesn't enable INTx while enabling MSI-X. */
> +        if ( !is_hardware_domain(pdev->domain) )
> +            pci_intx(pdev, false);
>      }
>      else if ( !new_enabled && msix->enabled )
>      {

... this done (as requested) behind the back of the guest, what's the
idea wrt the guest reading the command register? That continues to be
wired to vpci_hw_read16() (and hence accesses the underlying hardware
value irrespective of what patch 4 did).

Jan

Oleksandr Andrushchenko Feb. 8, 2022, 8:13 a.m. UTC | #2

On 04.02.22 16:25, Jan Beulich wrote:
> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -454,6 +454,22 @@ 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. */
>> +
>> +#ifdef CONFIG_HAS_PCI_MSI
>> +    if ( pdev->vpci->msi->enabled || pdev->vpci->msix->enabled )
>> +    {
>> +        /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */
>> +        cmd |= PCI_COMMAND_INTX_DISABLE;
>> +    }
>> +#endif
>> +
>> +    cmd_write(pdev, reg, cmd, data);
>> +}
> It's not really clear to me whether the TODO warrants this being a
> separate function. Personally I'd find it preferable if the logic
> was folded into cmd_write().
Not sure cmd_write needs to have guest's logic. And what's the
profit? Later on, when we decide how PCI_COMMAND can be emulated
this code will live in guest_cmd_write anyways
>
> With this and ...
>
>> --- a/xen/drivers/vpci/msi.c
>> +++ b/xen/drivers/vpci/msi.c
>> @@ -70,6 +70,10 @@ static void control_write(const struct pci_dev *pdev, unsigned int reg,
>>   
>>           if ( vpci_msi_arch_enable(msi, pdev, vectors) )
>>               return;
>> +
>> +        /* Make sure guest doesn't enable INTx while enabling MSI. */
>> +        if ( !is_hardware_domain(pdev->domain) )
>> +            pci_intx(pdev, false);
>>       }
>>       else
>>           vpci_msi_arch_disable(msi, pdev);
>> --- a/xen/drivers/vpci/msix.c
>> +++ b/xen/drivers/vpci/msix.c
>> @@ -92,6 +92,10 @@ static void control_write(const struct pci_dev *pdev, unsigned int reg,
>>           for ( i = 0; i < msix->max_entries; i++ )
>>               if ( !msix->entries[i].masked && msix->entries[i].updated )
>>                   update_entry(&msix->entries[i], pdev, i);
>> +
>> +        /* Make sure guest doesn't enable INTx while enabling MSI-X. */
>> +        if ( !is_hardware_domain(pdev->domain) )
>> +            pci_intx(pdev, false);
>>       }
>>       else if ( !new_enabled && msix->enabled )
>>       {
> ... this done (as requested) behind the back of the guest, what's the
> idea wrt the guest reading the command register? That continues to be
> wired to vpci_hw_read16() (and hence accesses the underlying hardware
> value irrespective of what patch 4 did).
Yes, good point. We need to add guest_cmd_read counterpart,
so we can also implement the same logic as in guest_cmd_write
wrt to INTx bit.
>
> Jan
>
Thank you,
Oleksandr

Jan Beulich Feb. 8, 2022, 9:33 a.m. UTC | #3

On 08.02.2022 09:13, Oleksandr Andrushchenko wrote:
> On 04.02.22 16:25, Jan Beulich wrote:
>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
>>> --- a/xen/drivers/vpci/header.c
>>> +++ b/xen/drivers/vpci/header.c
>>> @@ -454,6 +454,22 @@ 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. */
>>> +
>>> +#ifdef CONFIG_HAS_PCI_MSI
>>> +    if ( pdev->vpci->msi->enabled || pdev->vpci->msix->enabled )
>>> +    {
>>> +        /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */
>>> +        cmd |= PCI_COMMAND_INTX_DISABLE;
>>> +    }
>>> +#endif
>>> +
>>> +    cmd_write(pdev, reg, cmd, data);
>>> +}
>> It's not really clear to me whether the TODO warrants this being a
>> separate function. Personally I'd find it preferable if the logic
>> was folded into cmd_write().
> Not sure cmd_write needs to have guest's logic. And what's the
> profit? Later on, when we decide how PCI_COMMAND can be emulated
> this code will live in guest_cmd_write anyways

Why "will"? There's nothing conceptually wrong with putting all the
emulation logic into cmd_write(), inside an if(!hwdom) conditional.
If and when we gain CET-IBT support on the x86 side (and I'm told
there's an Arm equivalent of this), then to make this as useful as
possible it is going to be desirable to limit the number of functions
called through function pointers. You may have seen Andrew's huge
"x86: Support for CET Indirect Branch Tracking" series. We want to
keep down the number of such annotations; the vast part of the series
is about adding of such.

Jan

Oleksandr Andrushchenko Feb. 8, 2022, 9:38 a.m. UTC | #4

On 08.02.22 11:33, Jan Beulich wrote:
> On 08.02.2022 09:13, Oleksandr Andrushchenko wrote:
>> On 04.02.22 16:25, Jan Beulich wrote:
>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
>>>> --- a/xen/drivers/vpci/header.c
>>>> +++ b/xen/drivers/vpci/header.c
>>>> @@ -454,6 +454,22 @@ 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. */
>>>> +
>>>> +#ifdef CONFIG_HAS_PCI_MSI
>>>> +    if ( pdev->vpci->msi->enabled || pdev->vpci->msix->enabled )
>>>> +    {
>>>> +        /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */
>>>> +        cmd |= PCI_COMMAND_INTX_DISABLE;
>>>> +    }
>>>> +#endif
>>>> +
>>>> +    cmd_write(pdev, reg, cmd, data);
>>>> +}
>>> It's not really clear to me whether the TODO warrants this being a
>>> separate function. Personally I'd find it preferable if the logic
>>> was folded into cmd_write().
>> Not sure cmd_write needs to have guest's logic. And what's the
>> profit? Later on, when we decide how PCI_COMMAND can be emulated
>> this code will live in guest_cmd_write anyways
> Why "will"? There's nothing conceptually wrong with putting all the
> emulation logic into cmd_write(), inside an if(!hwdom) conditional.
> If and when we gain CET-IBT support on the x86 side (and I'm told
> there's an Arm equivalent of this), then to make this as useful as
> possible it is going to be desirable to limit the number of functions
> called through function pointers. You may have seen Andrew's huge
> "x86: Support for CET Indirect Branch Tracking" series. We want to
> keep down the number of such annotations; the vast part of the series
> is about adding of such.
Well, while I see nothing bad with that, from the code organization
it would look a bit strange: we don't differentiate hwdom in vpci
handlers, but instead provide one for hwdom and one for guests.
While I understand your concern I still think that at the moment
it will be more in line with the existing code if we provide a dedicated
handler.

Once we are all set with the handlers we may want performing a refactoring
with limiting the number of register handlers.

@Roger, what's your view on this?
> Jan
>
>
Thank you,
Oleksandr

Jan Beulich Feb. 8, 2022, 9:52 a.m. UTC | #5

On 08.02.2022 10:38, Oleksandr Andrushchenko wrote:
> 
> 
> On 08.02.22 11:33, Jan Beulich wrote:
>> On 08.02.2022 09:13, Oleksandr Andrushchenko wrote:
>>> On 04.02.22 16:25, Jan Beulich wrote:
>>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
>>>>> --- a/xen/drivers/vpci/header.c
>>>>> +++ b/xen/drivers/vpci/header.c
>>>>> @@ -454,6 +454,22 @@ 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. */
>>>>> +
>>>>> +#ifdef CONFIG_HAS_PCI_MSI
>>>>> +    if ( pdev->vpci->msi->enabled || pdev->vpci->msix->enabled )
>>>>> +    {
>>>>> +        /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */
>>>>> +        cmd |= PCI_COMMAND_INTX_DISABLE;
>>>>> +    }
>>>>> +#endif
>>>>> +
>>>>> +    cmd_write(pdev, reg, cmd, data);
>>>>> +}
>>>> It's not really clear to me whether the TODO warrants this being a
>>>> separate function. Personally I'd find it preferable if the logic
>>>> was folded into cmd_write().
>>> Not sure cmd_write needs to have guest's logic. And what's the
>>> profit? Later on, when we decide how PCI_COMMAND can be emulated
>>> this code will live in guest_cmd_write anyways
>> Why "will"? There's nothing conceptually wrong with putting all the
>> emulation logic into cmd_write(), inside an if(!hwdom) conditional.
>> If and when we gain CET-IBT support on the x86 side (and I'm told
>> there's an Arm equivalent of this), then to make this as useful as
>> possible it is going to be desirable to limit the number of functions
>> called through function pointers. You may have seen Andrew's huge
>> "x86: Support for CET Indirect Branch Tracking" series. We want to
>> keep down the number of such annotations; the vast part of the series
>> is about adding of such.
> Well, while I see nothing bad with that, from the code organization
> it would look a bit strange: we don't differentiate hwdom in vpci
> handlers, but instead provide one for hwdom and one for guests.
> While I understand your concern I still think that at the moment
> it will be more in line with the existing code if we provide a dedicated
> handler.

The existing code only deals with Dom0, and hence doesn't have any
pairs of handlers. FTAOD what I said above applies equally to other
separate guest read/write handlers you may be introducing. The
exception being when e.g. a hardware access handler is put in place
for Dom0 (for obvious reasons, I think).

Jan

Oleksandr Andrushchenko Feb. 8, 2022, 9:58 a.m. UTC | #6

On 08.02.22 11:52, Jan Beulich wrote:
> On 08.02.2022 10:38, Oleksandr Andrushchenko wrote:
>>
>> On 08.02.22 11:33, Jan Beulich wrote:
>>> On 08.02.2022 09:13, Oleksandr Andrushchenko wrote:
>>>> On 04.02.22 16:25, Jan Beulich wrote:
>>>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
>>>>>> --- a/xen/drivers/vpci/header.c
>>>>>> +++ b/xen/drivers/vpci/header.c
>>>>>> @@ -454,6 +454,22 @@ 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. */
>>>>>> +
>>>>>> +#ifdef CONFIG_HAS_PCI_MSI
>>>>>> +    if ( pdev->vpci->msi->enabled || pdev->vpci->msix->enabled )
>>>>>> +    {
>>>>>> +        /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */
>>>>>> +        cmd |= PCI_COMMAND_INTX_DISABLE;
>>>>>> +    }
>>>>>> +#endif
>>>>>> +
>>>>>> +    cmd_write(pdev, reg, cmd, data);
>>>>>> +}
>>>>> It's not really clear to me whether the TODO warrants this being a
>>>>> separate function. Personally I'd find it preferable if the logic
>>>>> was folded into cmd_write().
>>>> Not sure cmd_write needs to have guest's logic. And what's the
>>>> profit? Later on, when we decide how PCI_COMMAND can be emulated
>>>> this code will live in guest_cmd_write anyways
>>> Why "will"? There's nothing conceptually wrong with putting all the
>>> emulation logic into cmd_write(), inside an if(!hwdom) conditional.
>>> If and when we gain CET-IBT support on the x86 side (and I'm told
>>> there's an Arm equivalent of this), then to make this as useful as
>>> possible it is going to be desirable to limit the number of functions
>>> called through function pointers. You may have seen Andrew's huge
>>> "x86: Support for CET Indirect Branch Tracking" series. We want to
>>> keep down the number of such annotations; the vast part of the series
>>> is about adding of such.
>> Well, while I see nothing bad with that, from the code organization
>> it would look a bit strange: we don't differentiate hwdom in vpci
>> handlers, but instead provide one for hwdom and one for guests.
>> While I understand your concern I still think that at the moment
>> it will be more in line with the existing code if we provide a dedicated
>> handler.
> The existing code only deals with Dom0, and hence doesn't have any
> pairs of handlers.
This is fair
>   FTAOD what I said above applies equally to other
> separate guest read/write handlers you may be introducing. The
> exception being when e.g. a hardware access handler is put in place
> for Dom0 (for obvious reasons, I think).
@Roger, what's your preference here?
>
> Jan
>
Thank you,
Oleksandr

Roger Pau Monne Feb. 8, 2022, 11:11 a.m. UTC | #7

On Tue, Feb 08, 2022 at 09:58:40AM +0000, Oleksandr Andrushchenko wrote:
> 
> 
> On 08.02.22 11:52, Jan Beulich wrote:
> > On 08.02.2022 10:38, Oleksandr Andrushchenko wrote:
> >>
> >> On 08.02.22 11:33, Jan Beulich wrote:
> >>> On 08.02.2022 09:13, Oleksandr Andrushchenko wrote:
> >>>> On 04.02.22 16:25, Jan Beulich wrote:
> >>>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
> >>>>>> --- a/xen/drivers/vpci/header.c
> >>>>>> +++ b/xen/drivers/vpci/header.c
> >>>>>> @@ -454,6 +454,22 @@ 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. */
> >>>>>> +
> >>>>>> +#ifdef CONFIG_HAS_PCI_MSI
> >>>>>> +    if ( pdev->vpci->msi->enabled || pdev->vpci->msix->enabled )
> >>>>>> +    {
> >>>>>> +        /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */
> >>>>>> +        cmd |= PCI_COMMAND_INTX_DISABLE;
> >>>>>> +    }
> >>>>>> +#endif
> >>>>>> +
> >>>>>> +    cmd_write(pdev, reg, cmd, data);
> >>>>>> +}
> >>>>> It's not really clear to me whether the TODO warrants this being a
> >>>>> separate function. Personally I'd find it preferable if the logic
> >>>>> was folded into cmd_write().
> >>>> Not sure cmd_write needs to have guest's logic. And what's the
> >>>> profit? Later on, when we decide how PCI_COMMAND can be emulated
> >>>> this code will live in guest_cmd_write anyways
> >>> Why "will"? There's nothing conceptually wrong with putting all the
> >>> emulation logic into cmd_write(), inside an if(!hwdom) conditional.
> >>> If and when we gain CET-IBT support on the x86 side (and I'm told
> >>> there's an Arm equivalent of this), then to make this as useful as
> >>> possible it is going to be desirable to limit the number of functions
> >>> called through function pointers. You may have seen Andrew's huge
> >>> "x86: Support for CET Indirect Branch Tracking" series. We want to
> >>> keep down the number of such annotations; the vast part of the series
> >>> is about adding of such.
> >> Well, while I see nothing bad with that, from the code organization
> >> it would look a bit strange: we don't differentiate hwdom in vpci
> >> handlers, but instead provide one for hwdom and one for guests.
> >> While I understand your concern I still think that at the moment
> >> it will be more in line with the existing code if we provide a dedicated
> >> handler.
> > The existing code only deals with Dom0, and hence doesn't have any
> > pairs of handlers.
> This is fair
> >   FTAOD what I said above applies equally to other
> > separate guest read/write handlers you may be introducing. The
> > exception being when e.g. a hardware access handler is put in place
> > for Dom0 (for obvious reasons, I think).
> @Roger, what's your preference here?
> >

The newly introduced handler ends up calling the existing one, so in
this case it might make sense to expand cmd_write to also cater for
the domU case?

I think we need to be sensible here in that we don't want to end up
with handlers like:

register_read(...)
{
   if ( is_hardware_domain() )
       ....
   else
       ...
}

If there's shared code it's IMO better to not create as guest specific
handler.

It's also more risky to use the same handlers for dom0 and domU, as a
change intended to dom0 only might end up leaking in the domU path and
that could easily become a security issue.

Thanks, Roger.

Oleksandr Andrushchenko Feb. 8, 2022, 11:29 a.m. UTC | #8

On 08.02.22 13:11, Roger Pau Monné wrote:
> On Tue, Feb 08, 2022 at 09:58:40AM +0000, Oleksandr Andrushchenko wrote:
>>
>> On 08.02.22 11:52, Jan Beulich wrote:
>>> On 08.02.2022 10:38, Oleksandr Andrushchenko wrote:
>>>> On 08.02.22 11:33, Jan Beulich wrote:
>>>>> On 08.02.2022 09:13, Oleksandr Andrushchenko wrote:
>>>>>> On 04.02.22 16:25, Jan Beulich wrote:
>>>>>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
>>>>>>>> --- a/xen/drivers/vpci/header.c
>>>>>>>> +++ b/xen/drivers/vpci/header.c
>>>>>>>> @@ -454,6 +454,22 @@ 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. */
>>>>>>>> +
>>>>>>>> +#ifdef CONFIG_HAS_PCI_MSI
>>>>>>>> +    if ( pdev->vpci->msi->enabled || pdev->vpci->msix->enabled )
>>>>>>>> +    {
>>>>>>>> +        /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */
>>>>>>>> +        cmd |= PCI_COMMAND_INTX_DISABLE;
>>>>>>>> +    }
>>>>>>>> +#endif
>>>>>>>> +
>>>>>>>> +    cmd_write(pdev, reg, cmd, data);
>>>>>>>> +}
>>>>>>> It's not really clear to me whether the TODO warrants this being a
>>>>>>> separate function. Personally I'd find it preferable if the logic
>>>>>>> was folded into cmd_write().
>>>>>> Not sure cmd_write needs to have guest's logic. And what's the
>>>>>> profit? Later on, when we decide how PCI_COMMAND can be emulated
>>>>>> this code will live in guest_cmd_write anyways
>>>>> Why "will"? There's nothing conceptually wrong with putting all the
>>>>> emulation logic into cmd_write(), inside an if(!hwdom) conditional.
>>>>> If and when we gain CET-IBT support on the x86 side (and I'm told
>>>>> there's an Arm equivalent of this), then to make this as useful as
>>>>> possible it is going to be desirable to limit the number of functions
>>>>> called through function pointers. You may have seen Andrew's huge
>>>>> "x86: Support for CET Indirect Branch Tracking" series. We want to
>>>>> keep down the number of such annotations; the vast part of the series
>>>>> is about adding of such.
>>>> Well, while I see nothing bad with that, from the code organization
>>>> it would look a bit strange: we don't differentiate hwdom in vpci
>>>> handlers, but instead provide one for hwdom and one for guests.
>>>> While I understand your concern I still think that at the moment
>>>> it will be more in line with the existing code if we provide a dedicated
>>>> handler.
>>> The existing code only deals with Dom0, and hence doesn't have any
>>> pairs of handlers.
>> This is fair
>>>    FTAOD what I said above applies equally to other
>>> separate guest read/write handlers you may be introducing. The
>>> exception being when e.g. a hardware access handler is put in place
>>> for Dom0 (for obvious reasons, I think).
>> @Roger, what's your preference here?
> The newly introduced handler ends up calling the existing one,
But before doing so it implements guest specific logic which will be
extended as we add more bits of emulation
>   so in
> this case it might make sense to expand cmd_write to also cater for
> the domU case?
So, from the above I thought is was ok to have a dedicated handler
>
> I think we need to be sensible here in that we don't want to end up
> with handlers like:
>
> register_read(...)
> {
>     if ( is_hardware_domain() )
>         ....
>     else
>         ...
> }
>
> If there's shared code it's IMO better to not create as guest specific
> handler.
>
> It's also more risky to use the same handlers for dom0 and domU, as a
> change intended to dom0 only might end up leaking in the domU path and
> that could easily become a security issue.
So, just for your justification: BARs. Is this something we also want
to be kept separate or we want if (is_hwdom)?
I guess the former.
>
> Thanks, Roger.
Thank you,
Oleksandr

Roger Pau Monne Feb. 8, 2022, 2:09 p.m. UTC | #9

On Tue, Feb 08, 2022 at 11:29:07AM +0000, Oleksandr Andrushchenko wrote:
> 
> 
> On 08.02.22 13:11, Roger Pau Monné wrote:
> > On Tue, Feb 08, 2022 at 09:58:40AM +0000, Oleksandr Andrushchenko wrote:
> >>
> >> On 08.02.22 11:52, Jan Beulich wrote:
> >>> On 08.02.2022 10:38, Oleksandr Andrushchenko wrote:
> >>>> On 08.02.22 11:33, Jan Beulich wrote:
> >>>>> On 08.02.2022 09:13, Oleksandr Andrushchenko wrote:
> >>>>>> On 04.02.22 16:25, Jan Beulich wrote:
> >>>>>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
> >>>>>>>> --- a/xen/drivers/vpci/header.c
> >>>>>>>> +++ b/xen/drivers/vpci/header.c
> >>>>>>>> @@ -454,6 +454,22 @@ 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. */
> >>>>>>>> +
> >>>>>>>> +#ifdef CONFIG_HAS_PCI_MSI
> >>>>>>>> +    if ( pdev->vpci->msi->enabled || pdev->vpci->msix->enabled )
> >>>>>>>> +    {
> >>>>>>>> +        /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */
> >>>>>>>> +        cmd |= PCI_COMMAND_INTX_DISABLE;
> >>>>>>>> +    }
> >>>>>>>> +#endif
> >>>>>>>> +
> >>>>>>>> +    cmd_write(pdev, reg, cmd, data);
> >>>>>>>> +}
> >>>>>>> It's not really clear to me whether the TODO warrants this being a
> >>>>>>> separate function. Personally I'd find it preferable if the logic
> >>>>>>> was folded into cmd_write().
> >>>>>> Not sure cmd_write needs to have guest's logic. And what's the
> >>>>>> profit? Later on, when we decide how PCI_COMMAND can be emulated
> >>>>>> this code will live in guest_cmd_write anyways
> >>>>> Why "will"? There's nothing conceptually wrong with putting all the
> >>>>> emulation logic into cmd_write(), inside an if(!hwdom) conditional.
> >>>>> If and when we gain CET-IBT support on the x86 side (and I'm told
> >>>>> there's an Arm equivalent of this), then to make this as useful as
> >>>>> possible it is going to be desirable to limit the number of functions
> >>>>> called through function pointers. You may have seen Andrew's huge
> >>>>> "x86: Support for CET Indirect Branch Tracking" series. We want to
> >>>>> keep down the number of such annotations; the vast part of the series
> >>>>> is about adding of such.
> >>>> Well, while I see nothing bad with that, from the code organization
> >>>> it would look a bit strange: we don't differentiate hwdom in vpci
> >>>> handlers, but instead provide one for hwdom and one for guests.
> >>>> While I understand your concern I still think that at the moment
> >>>> it will be more in line with the existing code if we provide a dedicated
> >>>> handler.
> >>> The existing code only deals with Dom0, and hence doesn't have any
> >>> pairs of handlers.
> >> This is fair
> >>>    FTAOD what I said above applies equally to other
> >>> separate guest read/write handlers you may be introducing. The
> >>> exception being when e.g. a hardware access handler is put in place
> >>> for Dom0 (for obvious reasons, I think).
> >> @Roger, what's your preference here?
> > The newly introduced handler ends up calling the existing one,
> But before doing so it implements guest specific logic which will be
> extended as we add more bits of emulation
> >   so in
> > this case it might make sense to expand cmd_write to also cater for
> > the domU case?
> So, from the above I thought is was ok to have a dedicated handler

Given the current proposal where you are only dealing with INTx I don't
think it makes much sense to have a separate handler because you end
up calling cmd_write anyway, so what's added there could very well be
added at the top of cmd_write.

> >
> > I think we need to be sensible here in that we don't want to end up
> > with handlers like:
> >
> > register_read(...)
> > {
> >     if ( is_hardware_domain() )
> >         ....
> >     else
> >         ...
> > }
> >
> > If there's shared code it's IMO better to not create as guest specific
> > handler.
> >
> > It's also more risky to use the same handlers for dom0 and domU, as a
> > change intended to dom0 only might end up leaking in the domU path and
> > that could easily become a security issue.
> So, just for your justification: BARs. Is this something we also want
> to be kept separate or we want if (is_hwdom)?
> I guess the former.

I think BAR access handling is sufficiently different between dom0 and
domU that we want separate handlers.

Thanks, Roger.

Oleksandr Andrushchenko Feb. 8, 2022, 2:13 p.m. UTC | #10

On 08.02.22 16:09, Roger Pau Monné wrote:
> On Tue, Feb 08, 2022 at 11:29:07AM +0000, Oleksandr Andrushchenko wrote:
>>
>> On 08.02.22 13:11, Roger Pau Monné wrote:
>>> On Tue, Feb 08, 2022 at 09:58:40AM +0000, Oleksandr Andrushchenko wrote:
>>>> On 08.02.22 11:52, Jan Beulich wrote:
>>>>> On 08.02.2022 10:38, Oleksandr Andrushchenko wrote:
>>>>>> On 08.02.22 11:33, Jan Beulich wrote:
>>>>>>> On 08.02.2022 09:13, Oleksandr Andrushchenko wrote:
>>>>>>>> On 04.02.22 16:25, Jan Beulich wrote:
>>>>>>>>> On 04.02.2022 07:34, Oleksandr Andrushchenko wrote:
>>>>>>>>>> --- a/xen/drivers/vpci/header.c
>>>>>>>>>> +++ b/xen/drivers/vpci/header.c
>>>>>>>>>> @@ -454,6 +454,22 @@ 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. */
>>>>>>>>>> +
>>>>>>>>>> +#ifdef CONFIG_HAS_PCI_MSI
>>>>>>>>>> +    if ( pdev->vpci->msi->enabled || pdev->vpci->msix->enabled )
>>>>>>>>>> +    {
>>>>>>>>>> +        /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */
>>>>>>>>>> +        cmd |= PCI_COMMAND_INTX_DISABLE;
>>>>>>>>>> +    }
>>>>>>>>>> +#endif
>>>>>>>>>> +
>>>>>>>>>> +    cmd_write(pdev, reg, cmd, data);
>>>>>>>>>> +}
>>>>>>>>> It's not really clear to me whether the TODO warrants this being a
>>>>>>>>> separate function. Personally I'd find it preferable if the logic
>>>>>>>>> was folded into cmd_write().
>>>>>>>> Not sure cmd_write needs to have guest's logic. And what's the
>>>>>>>> profit? Later on, when we decide how PCI_COMMAND can be emulated
>>>>>>>> this code will live in guest_cmd_write anyways
>>>>>>> Why "will"? There's nothing conceptually wrong with putting all the
>>>>>>> emulation logic into cmd_write(), inside an if(!hwdom) conditional.
>>>>>>> If and when we gain CET-IBT support on the x86 side (and I'm told
>>>>>>> there's an Arm equivalent of this), then to make this as useful as
>>>>>>> possible it is going to be desirable to limit the number of functions
>>>>>>> called through function pointers. You may have seen Andrew's huge
>>>>>>> "x86: Support for CET Indirect Branch Tracking" series. We want to
>>>>>>> keep down the number of such annotations; the vast part of the series
>>>>>>> is about adding of such.
>>>>>> Well, while I see nothing bad with that, from the code organization
>>>>>> it would look a bit strange: we don't differentiate hwdom in vpci
>>>>>> handlers, but instead provide one for hwdom and one for guests.
>>>>>> While I understand your concern I still think that at the moment
>>>>>> it will be more in line with the existing code if we provide a dedicated
>>>>>> handler.
>>>>> The existing code only deals with Dom0, and hence doesn't have any
>>>>> pairs of handlers.
>>>> This is fair
>>>>>     FTAOD what I said above applies equally to other
>>>>> separate guest read/write handlers you may be introducing. The
>>>>> exception being when e.g. a hardware access handler is put in place
>>>>> for Dom0 (for obvious reasons, I think).
>>>> @Roger, what's your preference here?
>>> The newly introduced handler ends up calling the existing one,
>> But before doing so it implements guest specific logic which will be
>> extended as we add more bits of emulation
>>>    so in
>>> this case it might make sense to expand cmd_write to also cater for
>>> the domU case?
>> So, from the above I thought is was ok to have a dedicated handler
> Given the current proposal where you are only dealing with INTx I don't
> think it makes much sense to have a separate handler because you end
> up calling cmd_write anyway, so what's added there could very well be
> added at the top of cmd_write.
Good
>
>>> I think we need to be sensible here in that we don't want to end up
>>> with handlers like:
>>>
>>> register_read(...)
>>> {
>>>      if ( is_hardware_domain() )
>>>          ....
>>>      else
>>>          ...
>>> }
>>>
>>> If there's shared code it's IMO better to not create as guest specific
>>> handler.
>>>
>>> It's also more risky to use the same handlers for dom0 and domU, as a
>>> change intended to dom0 only might end up leaking in the domU path and
>>> that could easily become a security issue.
>> So, just for your justification: BARs. Is this something we also want
>> to be kept separate or we want if (is_hwdom)?
>> I guess the former.
> I think BAR access handling is sufficiently different between dom0 and
> domU that we want separate handlers.
Makes sense
> Thanks, Roger.
Thank you,
Oleksandr

Patch

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 88ca1ad8211d..33d8c15ae6e8 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -454,6 +454,22 @@  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. */
+
+#ifdef CONFIG_HAS_PCI_MSI
+    if ( pdev->vpci->msi->enabled || pdev->vpci->msix->enabled )
+    {
+        /* Guest wants to enable INTx. It can't be enabled if MSI/MSI-X enabled. */
+        cmd |= PCI_COMMAND_INTX_DISABLE;
+    }
+#endif
+
+    cmd_write(pdev, reg, cmd, data);
+}
+
 static void bar_write(const struct pci_dev *pdev, unsigned int reg,
                       uint32_t val, void *data)
 {
@@ -661,8 +677,9 @@  static int init_bars(struct pci_dev *pdev)
     }
 
     /* Setup a handler for the command register. */
-    rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write, PCI_COMMAND,
-                           2, header);
+    rc = vpci_add_register(pdev->vpci, vpci_hw_read16,
+                           is_hwdom ? cmd_write : guest_cmd_write,
+                           PCI_COMMAND, 2, header);
     if ( rc )
         return rc;
 
diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
index e3ce46869dad..90465dcb4831 100644
--- a/xen/drivers/vpci/msi.c
+++ b/xen/drivers/vpci/msi.c
@@ -70,6 +70,10 @@  static void control_write(const struct pci_dev *pdev, unsigned int reg,
 
         if ( vpci_msi_arch_enable(msi, pdev, vectors) )
             return;
+
+        /* Make sure guest doesn't enable INTx while enabling MSI. */
+        if ( !is_hardware_domain(pdev->domain) )
+            pci_intx(pdev, false);
     }
     else
         vpci_msi_arch_disable(msi, pdev);
diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index d1dbfc6e0ffd..4c0e1836b589 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -92,6 +92,10 @@  static void control_write(const struct pci_dev *pdev, unsigned int reg,
         for ( i = 0; i < msix->max_entries; i++ )
             if ( !msix->entries[i].masked && msix->entries[i].updated )
                 update_entry(&msix->entries[i], pdev, i);
+
+        /* Make sure guest doesn't enable INTx while enabling MSI-X. */
+        if ( !is_hardware_domain(pdev->domain) )
+            pci_intx(pdev, false);
     }
     else if ( !new_enabled && msix->enabled )
     {