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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.01.2022 14:41, Oleksandr Andrushchenko wrote:
> On 31.01.22 15:36, Jan Beulich wrote:
>> On 31.01.2022 14:30, Oleksandr Andrushchenko wrote:
>>> 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. */
>> Except that like for the "tailing gap" you also need to avoid the
>> "heading gap" ending up in a read of the underlying hardware
>> register. Effectively you want to deal properly with all
>> vpci_read_hw() invocations (including the one when no pdev was
>> found, which for a DomU may simply mean domain_crash()).
> Yes. And with the above patch I can now remove the "TODO patch" then?
> Because it is saying that we allow access to the registers, but it is not 
> safe.
> And now, if we disable that access, then TODO should be about the need to
> implement emulation for all the registers which are not yet handled which is
> obvious.

Yes, I think that other patch then should have no use anymore. (To be
honest I don't recall such a patch anyway.)

Jan