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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 08.09.21 17:46, Jan Beulich wrote:
> On 08.09.2021 15:33, Oleksandr Andrushchenko wrote:
>> On 08.09.21 13:03, Jan Beulich wrote:
>>> On 08.09.2021 11:43, Oleksandr Andrushchenko wrote:
>>>> On 08.09.21 12:27, Jan Beulich wrote:
>>>>> 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.
>>>> But in case of BAR sizing I need to actually return BAR size.
>>>> So, the comparison is the way to tell if the guest wants to read
>>>> actual (configured) BAR value or it tries to determine BAR's size.
>>>> This is why I compare and use the result as the answer to what needs
>>>> to be supplied to the guest. So, if I don't compare with 0xffffffff for the
>>>> hi part and 0xfffffff0 for the low then how do I know when to return
>>>> configured BAR or return the size?
>>> Well, but that's the common misunderstanding that I've been trying
>>> to point out: There's no difference between these two forms of
>>> reads. The BARs are simply registers with some r/o bits. There's
>>> no hidden 2nd register recording what was last written. When you
>>> write 0xffffffff, all you do is set all writable bits to 1. When
>>> you read back from the register, you will find all r/o bits
>>> unchanged (which in particular means all lower address bits are
>>> zero, thus allowing you to determine the size).
>>>
>>> When the spec says to write 0xffffffff for sizing purposes, OSes
>>> should follow that, yes. This doesn't preclude them to use other
>>> forms of writes for whichever purpose. Hence you do not want to
>>> special case sizing, but instead you want to emulate correctly
>>> all forms of writes, including subsequent reads to uniformly
>>> return the intended / expected values.
>>>
>>> Just to give an example (perhaps a little contrived): To size a
>>> 64-bit BAR, in principle you'd first need to write 0xffffffff to
>>> both halves. But there's nothing wrong with doing this in a
>>> different order: Act on the low half alone first, and then act
>>> on the high half. The acting on the high half could even be
>>> skipped if the low half sizing produced at least bit 31 set. Now
>>> if you were to special case seeing ffffffff:fffffff? as the
>>> last written pair of values, you'd break that (imo legitimate)
>>> alternative process of sizing.
>> How about:
> Yes, that's what I was after. Just one nit right away:
>
>> 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;
>>           val |= bar->type == VPCI_BAR_MEM32 ? PCI_BASE_ADDRESS_MEM_TYPE_32
>>                                              : PCI_BASE_ADDRESS_MEM_TYPE_64;
>>           val |= bar->prefetchable ? PCI_BASE_ADDRESS_MEM_PREFETCH : 0;
>>       }
>>
>>       bar->guest_addr &= ~(0xffffffffull << (hi ? 32 : 0));

Do you think this needs to be 0xfffffffful, not 0xffffffffull?

e.g. s/ull/ul

>>       bar->guest_addr |= (uint64_t)val << (hi ? 32 : 0);
>>
>>       bar->guest_addr &= ~(bar->size - 1) | ~PCI_BASE_ADDRESS_MEM_MASK;
>> }
>>
>> static uint32_t guest_bar_read(const struct pci_dev *pdev, unsigned int reg,
>>                                  void *data)
>> {
>>       struct vpci_bar *bar = data;
> const please.
Sure
>
> Jan
>
Thank you for helping with this!!

Oleksandr