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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.03.2023 14:54, Roger Pau Monné wrote:
> On Tue, Mar 14, 2023 at 12:56:33PM +0100, Jan Beulich wrote:
>> On 14.03.2023 11:13, Roger Pau Monne wrote:
>>> --- a/xen/drivers/vpci/msix.c
>>> +++ b/xen/drivers/vpci/msix.c
>>> @@ -27,6 +27,13 @@
>>>      ((addr) >= vmsix_table_addr(vpci, nr) &&                              \
>>>       (addr) < vmsix_table_addr(vpci, nr) + vmsix_table_size(vpci, nr))
>>>  
>>> +#define VMSIX_ADDR_ADJACENT(addr, vpci, nr)                               \
>>> +    ((PFN_DOWN(addr) == PFN_DOWN(vmsix_table_addr(vpci, nr)) &&           \
>>> +      (addr) < vmsix_table_addr(vpci, nr)) ||                             \
>>> +     (PFN_DOWN(addr) == PFN_DOWN(vmsix_table_addr(vpci, nr) +             \
>>> +                                 vmsix_table_size(vpci, nr) - 1) &&       \
>>> +      (addr) >= vmsix_table_addr(vpci, nr) + vmsix_table_size(vpci, nr)))
>>
>> While I realize this may impact performance a little, did you consider
>> using !VMSIX_ADDR_IN_RANGE() instead of open-coding it kind of? Then
>> again there's only a single use of the macro, and that's in code where
>> VMSIX_ADDR_IN_RANGE() was already checked (by the earlier invocation
>> of msix_find()), so the re-checking of the MSI-X table bounds isn't
>> strictly necessary anyway.
> 
> I didn't want to rely on the order of execution of the MMIO handlers,
> so I would rather make sure that the newly added one would work
> correctly if it turns to be checked for before the MSIX table handling
> one.
> 
> I could indeed use !VMSIX_ADDR_IN_RANGE() if that is clearer.

I think it would make the source easier to follow, even if the generated
code is slightly worse.

>>> @@ -438,6 +369,145 @@ static const struct hvm_mmio_ops vpci_msix_table_ops 
>>> = {
>>>      .write = msix_write,
>>>  };
>>>  
>>> +const static struct vpci_msix *adjacent_find(const struct domain *d,
>>> +                                             unsigned long addr)
>>> +{
>>> +    const struct vpci_msix *msix;
>>> +
>>> +    list_for_each_entry ( msix, &d->arch.hvm.msix_tables, next )
>>> +        /*
>>> +         * So far vPCI only traps accesses to the MSIX table, but not the 
>>> PBA
>>> +         * explicitly, and hence we only need to check for the hole 
>>> created by
>>> +         * the MSIX table.
>>> +         *
>>> +         * If the PBA table is also trapped, the check here should be 
>>> expanded
>>> +         * to take it into account.
>>> +         */
>>> +        if ( VMSIX_ADDR_ADJACENT(addr, msix->pdev->vpci, VPCI_MSIX_TABLE) )
>>> +            return msix;
>>
>> Is the comment really correct when considering that you don't change
>> vpci_make_msix_hole()?
> 
> Urg, I was really confused then, as I didn't remember (and didn't
> check) that we also punch a hole for the PBA.  That's not really
> needed for dom0, as we allow both reads and writes on that case anyway.

Well, until now we restricted the kinds of writes. But ...

>>> +    switch ( len )
>>> +    {
>>> +    case 1:
>>> +        *data = readb(mem + PAGE_OFFSET(addr));
>>> +        break;
>>> +
>>> +    case 2:
>>> +        *data = readw(mem + PAGE_OFFSET(addr));
>>> +        break;
>>> +
>>> +    case 4:
>>> +        *data = readl(mem + PAGE_OFFSET(addr));
>>> +        break;
>>> +
>>> +    case 8:
>>> +        *data = readq(mem + PAGE_OFFSET(addr));
>>> +        break;
>>
>> So far we have allowed only aligned 4- and 8-byte accesses to the PBA.
>> Shouldn't we continue to do so?
> 
> I've read the spec about this and wasn't able to find a reference
> about having to access the PBA using 4 and 8 byte accesses.  There's
> one for the MSI-X table, but it's my understanding it only applies to
> the table.

... you make a good point here: Perhaps we were too restrictive. Such
a change in behavior wants calling out in the description, though.

>> I'm also concerned of misaligned accesses: While we can't keep the
>> guest from doing such on pages we don't intercept, depending on the kind
>> of anomalies such may cause the effects there may be contained to that
>> guest. When doing the accesses from the hypervisor, bad effects could
>> affect the entire system. (FTAOD I don't mean to constrain guests, but I
>> do think we need to consider splitting misaligned accesses.)
> 
> I was also wondering about misaligned accesses.  Should be allow dom0
> any kind of access, while limiting domUs to aligned only?

I guess the code would be simpler we we treated both equally. As said,
my goal is not to refuse misaligned accesses, but to break them up. To
keep things simple we might even use purely byte accesses (and then
perhaps simply REP MOVSB). Special casing Dom0 would only add code for
no real gain.

Jan