Re: [PATCH 5/9] vpci/header: Implement guest BAR register handlers

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.09.2021 19:39, Oleksandr Andrushchenko wrote:
> On 07.09.21 19:30, Jan Beulich wrote:
>> On 07.09.2021 15:33, Oleksandr Andrushchenko wrote:
>>> On 06.09.21 17:31, Jan Beulich wrote:
>>>> On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
>>>>> --- a/xen/drivers/vpci/header.c
>>>>> +++ b/xen/drivers/vpci/header.c
>>>>> @@ -400,12 +400,72 @@ static void bar_write(const struct pci_dev *pdev, 
>>>>> unsigned int reg,
>>>>>    static void guest_bar_write(const struct pci_dev *pdev, unsigned int 
>>>>> reg,
>>>>>                                uint32_t val, void *data)
>>>>>    {
>>>>> +    struct vpci_bar *bar = data;
>>>>> +    bool hi = false;
>>>>> +
>>>>> +    if ( bar->type == VPCI_BAR_MEM64_HI )
>>>>> +    {
>>>>> +        ASSERT(reg > PCI_BASE_ADDRESS_0);
>>>>> +        bar--;
>>>>> +        hi = true;
>>>>> +    }
>>>>> +    else
>>>>> +        val &= PCI_BASE_ADDRESS_MEM_MASK;
>>>>> +    bar->guest_addr &= ~(0xffffffffull << (hi ? 32 : 0));
>>>>> +    bar->guest_addr |= (uint64_t)val << (hi ? 32 : 0);
>>>> What you store here is not the address that's going to be used,
>>>>    as
>>>> you don't mask off the low bits (to account for the BAR's size).
>>>> When a BAR gets written with all ones, all writable bits get these
>>>> ones stored. The address of the BAR, aiui, really changes to
>>>> (typically) close below 4Gb (in the case of a 32-bit BAR), which
>>>> is why memory / I/O decoding should be off while sizing BARs.
>>>> Therefore you shouldn't look for the specific "all writable bits
>>>> are ones" pattern (or worse, as you presently do, the "all bits
>>>> outside of the type specifier are ones" one) on the read path.
>>>> Instead mask the value appropriately here, and simply return back
>>>> the stored value from the read path.
>>> "PCI LOCAL BUS SPECIFICATION, REV. 3.0", "IMPLEMENTATION NOTE
>>>
>>> Sizing a 32-bit Base Address Register Example" says, that
>>>
>>> "Software saves the original value of the Base Address register, writes
>>> 0 FFFF FFFFh to the register, then reads it back."
>>>
>>> The same applies for 64-bit BARs. So what's wrong if I try to catch such
>>> a write when a guest tries to size the BAR? The only difference is that
>>> I compare as
>>>           if ( (val & PCI_BASE_ADDRESS_MEM_MASK_32) == 
>>> PCI_BASE_ADDRESS_MEM_MASK_32 )
>>> which is because val in the question has lower bits cleared.
>> Because while this matches what the spec says, it's not enough to
>> match how hardware behaves.
> But we should implement as the spec says, not like buggy hardware behaves

The behavior I'm describing doesn't violate the spec. There merely is
more to it than what the spec says, or one might also view it the way
that the spec doesn't use the best way of expressing things.

>>   Yet you want to mimic hardware behavior
>> as closely as possible here. There is (iirc) at least one other
>> place in the source tree were we had to adjust a similar initial
>> implementation to be closer to one expected by guests,
> 
> Could you please point me to that code so I can consult and possibly
> re-use the approach?

I only recall the fact; this might have been hvmloader, vpci, or yet
somewhere else. I'm sorry.

>>>>> @@ -522,6 +582,13 @@ static int add_bar_handlers(struct pci_dev *pdev, 
>>>>> bool is_hwdom)
>>>>>                if ( rc )
>>>>>                    return rc;
>>>>>            }
>>>>> +        /*
>>>>> +         * It is neither safe nor secure to initialize guest's view of 
>>>>> the BARs
>>>>> +         * with real values which are used by the hardware domain, so 
>>>>> assign
>>>>> +         * all zeros to guest's view of the BARs, so the guest can 
>>>>> perform
>>>>> +         * proper PCI device enumeration and assign BARs on its own.
>>>>> +         */
>>>>> +        bars[i].guest_addr = 0;
>>>> I'm afraid I don't understand the comment: Without memory decoding
>>>> enabled, the BARs are simple registers (with a few r/o bits).
>>> My first implementation was that bar->guest_addr was initialized with
>>> the value of bar->addr (physical BAR value), but talking on IRC with
>>> Roger he suggested that this might be a security issue to let guest
>>> a hint about physical values, so then I changed the assignment to be 0.
>> Well, yes, that's certainly correct. It's perhaps too unobvious to me
>> why one may want to use the host value here in the first place. It
>> simply has no meaning here.
> Do you want me to remove the comment?

Yes. I wonder whether the assignment is necessary in the first place:
I'd somehow expect the structure to come from xzalloc(). Albeit I
guess this function can be run through more than once without freeing
the structure and then allocating is anew?

Jan