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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan