Re: [Xen-devel] [PATCH v2] vpci: honor read-only devices

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.09.2019 11:28, Roger Pau Monné  wrote:
> On Tue, Sep 03, 2019 at 11:09:09AM +0200, Jan Beulich wrote:
>> On 02.09.2019 17:30, Roger Pau Monne wrote:
>>> --- a/xen/drivers/vpci/vpci.c
>>> +++ b/xen/drivers/vpci/vpci.c
>>> @@ -418,13 +418,21 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, 
>>> unsigned int size,
>>>          return;
>>>      }
>>>  
>>> -    /*
>>> -     * Find the PCI dev matching the address.
>>> -     * Passthrough everything that's not trapped.
>>> -     */
>>> +    /* Find the PCI dev matching the address. */
>>>      pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
>>>      if ( !pdev )
>>>      {
>>> +        const unsigned long *ro_map = pci_get_ro_map(sbdf.seg);
>>> +
>>> +        if ( ro_map && test_bit(sbdf.bdf, ro_map) )
>>> +            /* Ignore writes to read-only devices. */
>>> +            return;
>>> +
>>> +        /*
>>> +         * Let the hardware domain access config space regions for 
>>> non-existent
>>> +         * devices.
>>> +         * TODO: revisit for domU support.
>>> +         */
>>>          vpci_write_hw(sbdf, reg, size, data);
>>>          return;
>>>      }
>>>
>>
>> In principle I'm okay with the change, but I have two more things
>> to be considered:
>>
>> 1) I'd prefer if the check was independent of the return value of
>> pci_get_pdev_by_domain(), to be more robust against the r/o map
>> having got updated but the owner still being hwdom.
> 
> So the RO check would be done ahead of the owner check?
> 
> I can do that, but it seems like a bodge for the locking issues (or
> lack of it) we have in the handling of PCI devices. I assume having a
> RO device assigned to a domain different than dom_xen is not possible.

It ought not be possible. Hence me saying "more robust" (i.e. in
case the "ought not" somehow gets broken). And no, the comment
wasn't really related to the (lack of) locking here - that's an
orthogonal issue.

>> 2) Wouldn't it be better to move the check into the callers of
>> vpci_write(), to avoid the duplicate lookup in the qword-MCFG-
>> write case? The main questionable point here is where, for DomU
>> support, the SBDF translation is going to live.
> 
> So I have a series I'm going to send quite soon in order to integrate
> vPCI with ioreq, as a first step in order to make it available to
> domUs.
> 
> The SBDF translation there is going to be performed by the ioreq code
> (ie: hvm_select_ioreq_server), but checking against the RO map there
> would be wrong, as ioreq doesn't know whether the underlying handler
> is for an emulated device or for a passthrough one. I think the RO
> check needs to be in the vPCI code itself.

Oh, sure. The question then simply converts to "Where can it be done
the earliest?" I.e. when/where do we have the physical SBDF in our
hands?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel