Re: [PATCH v3] vpci/msix: handle accesses adjacent to the MSI-X table

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.03.2023 18:08, Roger Pau Monné wrote:
> On Wed, Mar 22, 2023 at 06:05:55PM +0100, Roger Pau Monné wrote:
>> On Wed, Mar 22, 2023 at 04:14:54PM +0100, Jan Beulich wrote:
>>> On 22.03.2023 15:30, Roger Pau Monne wrote:
>>>> Changes since v2:
>>>>  - Slightly adjust VMSIX_ADDR_SAME_PAGE().
>>>>  - Use IS_ALIGNED and unlikely for the non-aligned access checking.
>>>>  - Move the check for the page mapped before the aligned one.
>>>>  - Remove cast of data to uint8_t and instead use a mask in order to
>>>>    avoid undefined behaviour when shifting.
>>>>  - Remove Xen maps of the MSIX related regions when memory decoding
>>>>    for the device is enabled by dom0, in order to purge stale maps.
>>>
>>> I'm glad you thought of this. The new code has issues, though:
>>>
>>>> @@ -182,93 +187,201 @@ static struct vpci_msix_entry *get_entry(struct 
>>>> vpci_msix *msix,
>>>>      return &msix->entries[(addr - start) / PCI_MSIX_ENTRY_SIZE];
>>>>  }
>>>>  
>>>> -static void __iomem *get_pba(struct vpci *vpci)
>>>> +static void __iomem *get_table(struct vpci *vpci, unsigned int slot)
>>>>  {
>>>>      struct vpci_msix *msix = vpci->msix;
>>>>      /*
>>>> -     * PBA will only be unmapped when the device is deassigned, so access 
>>>> it
>>>> -     * without holding the vpci lock.
>>>> +     * Regions will only be unmapped when the device is deassigned, so 
>>>> access
>>>> +     * them without holding the vpci lock.
>>>
>>> The first part of the sentence is now stale, and the second part is in
>>> conflict ...
>>>
>>>> @@ -482,6 +641,26 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
>>>>          }
>>>>      }
>>>>  
>>>> +    if ( is_hardware_domain(d) )
>>>> +    {
>>>> +        unsigned int i;
>>>> +
>>>> +        /*
>>>> +         * For the hardware domain only remove any hypervisor mappings of 
>>>> the
>>>> +         * MSIX or PBA related areas, as dom0 is capable of moving the 
>>>> position
>>>> +         * of the BARs in the host address space.
>>>> +         *
>>>> +         * We rely on being called with the vPCI lock held in order to 
>>>> not race
>>>> +         * with get_table().
>>>
>>> ... with what you say (and utilize) here. Furthermore this comment also 
>>> wants
>>> clarifying that apply_map() -> modify_decoding() not (afaics) holding the 
>>> lock
>>> when calling here is not a problem, as no mapping can exist yet that may 
>>> need
>>> tearing down. (I first wondered whether you wouldn't want to assert that the
>>> lock is being held. You actually could, but only after finding a non-NULL
>>> table entry.)
>>
>> Oh, yes, sorry, I should update those comments.  vpci_make_msix_hole()
>> gets called before bars[].enabled gets set, so there should be no
>> users of the mappings at that time because we don't handle accesses
>> when the BAR is not mapped.
>>
>> Not sure whether we should consider an access from when the BAR was
>> actually enabled by a different thread could still continue while on
>> another thread the BAR has been disabled and enabled again (and thus
>> the mapping removed).  It's a theoretical race, so I guess I will look
>> into making sure we cannot hit it.
> 
> Hm, maybe it doesn't matter much because such kind of trace could only
> be triggered by the hardware domain anyway, and it has plenty of other
> ways to mess with Xen.

Preferably we should get things to use proper locking. If that turns out
too hard, properly justified such an exception for Dom0 might be
acceptable.

Jan