Re: [PATCH v5 06/14] vpci/header: implement guest BAR register handlers

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 31.01.22 13:39, Jan Beulich wrote:
> On 31.01.2022 12:23, Oleksandr Andrushchenko wrote:
>> On 31.01.22 13:10, Roger Pau Monné wrote:
>>> Right (see my previous reply to this comment). I think it would be
>>> easier (and cleaner) if you switched the default behavior regarding
>>> unhandled register access for domUs at the start of the series (drop
>>> writes, reads returns ~0), and then you won't need to add all those
>>> dummy handler to drop writes and return ~0 for reads.
>>>
>>> It's going to be more work initially as you would need to support
>>> passthrough of more registers, but it's the right approach that we
>>> need implementation wise.
>> While I agree in general, this effectively means that I'll need to provide
>> handling for all PCIe registers and capabilities from the very start.
>> Otherwise no guest be able to properly initialize a PCI device without that.
>> Of course, we may want starting from stubs instead of proper emulation,
>> which will direct the access to real HW and later on we add proper emulation.
>> But, again, this is going to be a rather big piece of code where we need
>> to explicitly handle every possible capability.
> Since the two sub-threads are now about exactly the same topic, I'm
> answering here instead of there.
>
> No, you are not going to need to emulate all possible capabilities.
> We (or really qemu) don't do this on x86 either. Certain capabilities
> may be a must, but not everything. There are also device specific
> registers not covered by any capability structures - what to do with
> those is even more of a question.
>
> Furthermore for some of the fields justification why access to the
> raw hardware value is fine is going to be easy: r/o fields like
> vendor and device ID, for example. But every bit you allow direct
> access to needs to come with justification.
>
>> At the moment we are not going to claim that vPCI provides all means to
>> pass through a PCI device safely with this respect and this is why the 
>> feature
>> itself won't even be a tech preview yet. For that reason I think we can still
>> have implemented only crucial set of handlers and still allow the rest to
>> be read/write directly without emulation.
> I think you need to separate what you need for development from what
> goes upstream: For dev purposes you can very well invert the policy
> from white- to black-listing. But if we accepted the latter into the
> main tree, the risk would be there that something gets missed at the
> time where the permission model gets changed around.
>
> You could even have a non-default mode operating the way you want it
> (along the lines of pciback's permissive mode), allowing you to get
> away without needing to carry private patches. Things may also
> initially only work in that mode. But the default should be a mode
> which is secure (and which perhaps initially offers only very limited
> functionality).
Ok, so to make it clear:
1. We do not allow unhandled access for guests: for that I will create a
dedicated patch which will implement such restrictions. Something like
the below (for both vPCI read and write):

diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index c5e67491c24f..9ef2a1b5af58 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -347,6 +347,7 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, 
unsigned int size)
      const struct vpci_register *r;
      unsigned int data_offset = 0;
      uint32_t data = ~(uint32_t)0;
+    bool handled = false;

      if ( !size )
      {
@@ -405,6 +406,8 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, 
unsigned int size)
          if ( cmp > 0 )
              continue;

+        handled = true; /* Found the handler for this access. */
+
          if ( emu.offset < r->offset )
          {
              /* Heading gap, read partial content from hardware. */
@@ -432,6 +435,10 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, 
unsigned int size)
      }
      spin_unlock(&pdev->vpci_lock);

+    /* All unhandled guest requests return all 1's. */
+    if ( !is_hardware_domain(d) && !handled )
+        return ~(uint32_t)0;
+
      if ( data_offset < size )
      {
          /* Tailing gap, read the remaining. */

@Roger: does the above work for you?

2. For the time being, while only a reduced set of registers is emulated,
everyone who wants to test vPCI for guests should either create their own
patch with those handlers implemented or overcome that in any other
suitable way, e.g. with a hack that removes HW register access protection
or by any other means.

>
>> Another question is what needs to be done for vendor specific capabilities?
>> How these are going to be emulated?
> By vendor specific code, I'm afraid. Assuming these capabilities
> really need exposing in the first place.
I have a feeling this is not going to happen...
>
> Jan
>

Thank you,
Oleksandr